xfrm: Force a dst refcount before entering the xfrm type handlers

Crypto requests might return asynchronous. In this case we leave the rcu protected region, so force a refcount on the skb's destination entry before we enter the xfrm type input/output handlers.

This fixes a crash when a route is deleted whilst sending IPsec data that is transformed by an asynchronous algorithm.

---
 net/xfrm/xfrm_input.c  |    2 ++
 net/xfrm/xfrm_output.c |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

index 872065c..341cd11 100644
@@ -190,6 +190,8 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 		XFRM_SKB_CB(skb)->seq.input.low = seq;
 		XFRM_SKB_CB(skb)->seq.input.hi = seq_hi;
 
+		skb_dst_force(skb);
+
 		nexthdr = x->type->input(x, skb);
 
 		if (nexthdr == -EINPROGRESS)

index 1aba03f..8f3f0ee 100644
@@ -78,6 +78,8 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 
 		spin_unlock_bh(&x->lock);
 
+		skb_dst_force(skb);
+
 		err = x->type->output(x, skb);
 		if (err == -EINPROGRESS)
 			goto out_exit;

1.7.0.4

We clone the child entry in skb_dst_pop before we call skb_dst_drop(). Otherwise we might kill the child right before we return it to the caller.

---
 include/net/dst.h      |    2 +-
 net/xfrm/xfrm_output.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 2a46cba..75b95df 100644
@@ -345,7 +345,7 @@ static inline void skb_tunnel_rx(struct sk_buff *skb, struct net_device *dev)
 
 static inline struct dst_entry *skb_dst_pop(struct sk_buff *skb)
 {
-	struct dst_entry *child = skb_dst(skb)->child;
+	struct dst_entry *child = dst_clone(skb_dst(skb)->child);
 
 	skb_dst_drop(skb);
 	return child;

index 8f3f0ee..47bacd8 100644
@@ -96,7 +96,7 @@ resume:
 			err = -EHOSTUNREACH;
 			goto error_nolock;
 		}
-		skb_dst_set(skb, dst_clone(dst));
+		skb_dst_set(skb, dst);
 		x = dst->xfrm;
 	} while (x && !(x->outer_mode->flags & XFRM_MODE_FLAG_TUNNEL));
 

1.7.0.4

Steffen we really need to find another way to fix these problems.

We already make way too many expensive atomic operations in these code paths modified by your 3 patches, we should strive to not add new ones.

Thanks.

I know that it is exspensive, but we have to take a refcount if the crypto layer returns asyncronous. Unfortunately it is too late to take the refcount when the crypto layer notifies us about that as the skb might be already gone.

The second patch just moves the refcount from xfrm_output_one to skb_dst_pop. As xfrm_output_one is the only user of skb_dst_pop, we take the refcont just a bit realier.

The third one makes the socket policy case consistent to the case where we get the destination entry from the flow cache where we take a reference. We can either return the dst entry with or without a refcont in both cases. But we can't return sometimes with and somtimes without a refcount.

I'd be happy to see all these refcounts gone too of course, but that's way beyond a simple bug fix.

Steffen

From: Steffen Klassert <[email protected]>
Date: Wed, 16 Mar 2011 08:43:06 +0100

We can pass around an atomic_t for the crypto layer to bump when it decides to take an async path, bump it on entry and upon event wakeup decrement it.

Actually, a plain atomic_t is a bad idea because we might have to release the object. So callbacks with private void pointer arg is better.