Messages in this thread Patch in this message | | | Subject | Re: [PATCH NET-NEXT] ipv6: skb_expand_head() adjust skb->truesize incorrectly | From | Eric Dumazet <> | Date | Mon, 23 Aug 2021 14:45:56 -0700 |
| |
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; }
| |