Messages in this thread | | | Subject | Re: [PATCH NET-NEXT] ipv6: skb_expand_head() adjust skb->truesize incorrectly | From | Eric Dumazet <> | Date | Mon, 23 Aug 2021 14:51:42 -0700 |
| |
On 8/23/21 2:45 PM, Eric Dumazet wrote: > > > On 8/23/21 10:25 AM, Christoph Paasch wrote: >> Hello, >> >> On Mon, Aug 23, 2021 at 12:56 AM Vasily Averin <vvs@virtuozzo.com> wrote: >>> >>> Christoph Paasch reports [1] about incorrect skb->truesize >>> after skb_expand_head() call in ip6_xmit. >>> This happen because skb_set_owner_w() for newly clone skb is called >>> too early, before pskb_expand_head() where truesize is adjusted for >>> (!skb-sk) case. >>> >>> [1] https://lkml.org/lkml/2021/8/20/1082 >>> >>> Reported-by: Christoph Paasch <christoph.paasch@gmail.com> >>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com> >>> --- >>> net/core/skbuff.c | 24 +++++++++++++----------- >>> 1 file changed, 13 insertions(+), 11 deletions(-) >>> >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>> index f931176..508d5c4 100644 >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c >>> @@ -1803,6 +1803,8 @@ struct sk_buff *skb_realloc_headroom(struct sk_buff *skb, unsigned int headroom) >>> >>> struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom) >>> { >>> + struct sk_buff *oskb = skb; >>> + struct sk_buff *nskb = NULL; >>> int delta = headroom - skb_headroom(skb); >>> >>> if (WARN_ONCE(delta <= 0, >>> @@ -1811,21 +1813,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom) >>> >>> /* pskb_expand_head() might crash, if skb is shared */ >>> if (skb_shared(skb)) { >>> - struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC); >>> - >>> - if (likely(nskb)) { >>> - if (skb->sk) >>> - skb_set_owner_w(nskb, skb->sk); >>> - consume_skb(skb); >>> - } else { >>> - kfree_skb(skb); >>> - } >>> + nskb = skb_clone(skb, GFP_ATOMIC); >>> skb = nskb; >>> } >>> if (skb && >>> - pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) { >>> - kfree_skb(skb); >>> + pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) >>> skb = NULL; >>> + >>> + if (!skb) { >>> + kfree_skb(oskb); >>> + if (nskb) >>> + kfree_skb(nskb); >>> + } else if (nskb) { >>> + if (oskb->sk) >>> + skb_set_owner_w(nskb, oskb->sk); >>> + consume_skb(oskb); >> >> sorry, this does not fix the problem. The syzkaller repro still >> triggers the WARN. >> >> When it happens, the skb in ip6_xmit() is not shared as it comes from >> __tcp_transmit_skb, where it is skb_clone()'d. >> >> > > Old code (in skb_realloc_headroom()) > was first calling skb2 = skb_clone(skb, GFP_ATOMIC); > > At this point, skb2->sk was NULL > So pskb_expand_head(skb2, SKB_DATA_ALIGN(delta), 0, ...) was able to tweak skb2->truesize > > I would try : > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index f9311762cc475bd38d87c33e988d7c983b902e56..326749a8938637b044a616cc33b6a19ed191ac41 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -1804,6 +1804,7 @@ EXPORT_SYMBOL(skb_realloc_headroom); > struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom) > { > int delta = headroom - skb_headroom(skb); > + struct sk_buff *oskb = NULL; > > if (WARN_ONCE(delta <= 0, > "%s is expecting an increase in the headroom", __func__)) > @@ -1813,19 +1814,21 @@ struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom) > if (skb_shared(skb)) { > struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC); > > - if (likely(nskb)) { > - if (skb->sk) > - skb_set_owner_w(nskb, skb->sk); > - consume_skb(skb); > - } else { > + if (unlikely(!nskb)) { > kfree_skb(skb); > + return NULL; > } > + oskb = skb; > skb = nskb; > } > - if (skb && > - pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) { > + if (pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) { > kfree_skb(skb); > - skb = NULL; > + kfree_skb(oskb); > + return NULL; > + } > + if (oskb) { > + skb_set_owner_w(skb, oskb->sk); > + consume_skb(oskb); > } > return skb; > }
Oh well, probably not going to work.
We have to find a way to properly increase skb->truesize, even if skb_clone() is _not_ called.
| |