Patchwork [net-next,3/3] fix skb_morph to preserve skb->sk and skb->destructor pointers

login
register
mail settings
Submitter Eric Dumazet
Date Nov. 5, 2013, 2:06 p.m.
Message ID <1383660372.4291.134.camel@edumazet-glaptop2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/288532/
State Superseded
Headers show

Comments

Eric Dumazet - Nov. 5, 2013, 2:06 p.m.
On Tue, 2013-11-05 at 12:02 +0100, Jiri Pirko wrote:
> Currently __skb_clone sets skb->sk and skb->destructor to NULL. This is
> not right for skb_morph use case because skb->sk may be previously
> set (e. g. by xt_TPROXY).
> 
> Also, during skb_morph the destructor should not be called. It might be
> previously set, e. g. by xt_TPROXY to sock_edemux, and that would cause
> put sk while skb is still in flight.

truesize alert. You should add some documentation that skb_morph()
must not be used in transmit path.

Its not clear to be how this can happen.

skb_morph() being used only from ipv4 defrag (or ipv6 reassembly).

Maybe the problem could be fixed by doing this defrag _before_ setting
skb->sk ?

Also, I would prefer you find a way to put all this logic inside
skb_morph() instead of adding such complexity in this already complex
code, I fear the compiler will generate slower code with your patch on
fast path.

Maybe something as simple as following (untested) patch ?

Note that the truesize concern might need some care.



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko - Nov. 5, 2013, 2:47 p.m.
Tue, Nov 05, 2013 at 03:06:12PM CET, eric.dumazet@gmail.com wrote:
>On Tue, 2013-11-05 at 12:02 +0100, Jiri Pirko wrote:
>> Currently __skb_clone sets skb->sk and skb->destructor to NULL. This is
>> not right for skb_morph use case because skb->sk may be previously
>> set (e. g. by xt_TPROXY).
>> 
>> Also, during skb_morph the destructor should not be called. It might be
>> previously set, e. g. by xt_TPROXY to sock_edemux, and that would cause
>> put sk while skb is still in flight.
>
>truesize alert. You should add some documentation that skb_morph()
>must not be used in transmit path.
>
>Its not clear to be how this can happen.
>
>skb_morph() being used only from ipv4 defrag (or ipv6 reassembly).

nod

>
>Maybe the problem could be fixed by doing this defrag _before_ setting
>skb->sk ?

skb->sk is set for exmaple in xt_TPROXY that is before reassemly is
done, because reassembly is done after all the netfilter code pass the
skb through. I do not see how this can be changed in the way you are
suggesting.


>
>Also, I would prefer you find a way to put all this logic inside
>skb_morph() instead of adding such complexity in this already complex
>code, I fear the compiler will generate slower code with your patch on
>fast path.
>
>Maybe something as simple as following (untested) patch ?

I had very similar patch prepared. But I did not like it so I chose the
other way. I'm more or less okay with doing this this way though...

>
>Note that the truesize concern might need some care.
>
>diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>index 3735fad5616e..afabfd6ef341 100644
>--- a/net/core/skbuff.c
>+++ b/net/core/skbuff.c
>@@ -793,18 +793,28 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
> 
> /**
>  *	skb_morph	-	morph one skb into another
>- *	@dst: the skb to receive the contents
>+ *	@skb: the skb to receive the contents
>  *	@src: the skb to supply the contents
>  *
>  *	This is identical to skb_clone except that the target skb is
>- *	supplied by the user.
>+ *	supplied by the user, and that we keep target skb destructor in place,
>+ *	meaning this can not be used in transmit path, as skb->truesize might
>+ *	change.
>  *
>  *	The target skb is returned upon exit.
>  */
>-struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
>+struct sk_buff *skb_morph(struct sk_buff *skb, struct sk_buff *src)
> {
>-	skb_release_all(dst);
>-	return __skb_clone(dst, src);
>+	struct sock *save_sk = skb->sk;
>+	void (*save_destructor)(struct sk_buff *) = skb->destructor;
>+
>+	skb->sk = NULL;
>+	skb->destructor = NULL;
>+	skb_release_all(skb);
>+	__skb_clone(skb, src);
>+	skb->sk = save_sk;
>+	skb->destructor = save_destructor;
>+	return skb;
> }
> EXPORT_SYMBOL_GPL(skb_morph);
> 
>
>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3735fad5616e..afabfd6ef341 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -793,18 +793,28 @@  static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 
 /**
  *	skb_morph	-	morph one skb into another
- *	@dst: the skb to receive the contents
+ *	@skb: the skb to receive the contents
  *	@src: the skb to supply the contents
  *
  *	This is identical to skb_clone except that the target skb is
- *	supplied by the user.
+ *	supplied by the user, and that we keep target skb destructor in place,
+ *	meaning this can not be used in transmit path, as skb->truesize might
+ *	change.
  *
  *	The target skb is returned upon exit.
  */
-struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
+struct sk_buff *skb_morph(struct sk_buff *skb, struct sk_buff *src)
 {
-	skb_release_all(dst);
-	return __skb_clone(dst, src);
+	struct sock *save_sk = skb->sk;
+	void (*save_destructor)(struct sk_buff *) = skb->destructor;
+
+	skb->sk = NULL;
+	skb->destructor = NULL;
+	skb_release_all(skb);
+	__skb_clone(skb, src);
+	skb->sk = save_sk;
+	skb->destructor = save_destructor;
+	return skb;
 }
 EXPORT_SYMBOL_GPL(skb_morph);