Messages in this thread | | | Subject | Re: [PATCH net v9] skb_expand_head() adjust skb->truesize incorrectly | From | Eric Dumazet <> | Date | Mon, 4 Oct 2021 12:26:02 -0700 |
| |
On 10/4/21 6:00 AM, Vasily Averin wrote: > Christoph Paasch reports [1] about incorrect skb->truesize > after skb_expand_head() call in ip6_xmit. > This may happen because of two reasons: > - skb_set_owner_w() for newly cloned skb is called too early, > before pskb_expand_head() where truesize is adjusted for (!skb-sk) case. > - pskb_expand_head() does not adjust truesize in (skb->sk) case. > In this case sk->sk_wmem_alloc should be adjusted too. > > [1] https://lkml.org/lkml/2021/8/20/1082 > > Fixes: f1260ff15a71 ("skbuff: introduce skb_expand_head()") > Fixes: 2d85a1b31dde ("ipv6: ip6_finish_output2: set sk into newly allocated nskb") > Reported-by: Christoph Paasch <christoph.paasch@gmail.com> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > v9: restored sock_edemux check > v8: clone non-wmem skb > v7 (from kuba@): > shift more magic into helpers, > follow Eric's advice and don't inherit non-wmem skbs for now > v6: fixed delta, > improved comments > v5: fixed else condition, thanks to Eric > reworked update of expanded skb, > added corresponding comments > v4: decided to use is_skb_wmem() after pskb_expand_head() call > fixed 'return (EXPRESSION);' in os_skb_wmem according to Eric Dumazet > v3: removed __pskb_expand_head(), > added is_skb_wmem() helper for skb with wmem-compatible destructors > there are 2 ways to use it: > - before pskb_expand_head(), to create skb clones > - after successfull pskb_expand_head() to change owner on extended > skb. > v2: based on patch version from Eric Dumazet, > added __pskb_expand_head() function, which can be forced > to adjust skb->truesize and sk->sk_wmem_alloc. > --- > include/net/sock.h | 1 + > net/core/skbuff.c | 35 ++++++++++++++++++++++------------- > net/core/sock.c | 8 ++++++++ > 3 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/include/net/sock.h b/include/net/sock.h > index 66a9a90f9558..a547651d46a7 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -1695,6 +1695,7 @@ struct sk_buff *sock_wmalloc(struct sock *sk, unsigned long size, int force, > gfp_t priority); > void __sock_wfree(struct sk_buff *skb); > void sock_wfree(struct sk_buff *skb); > +bool is_skb_wmem(const struct sk_buff *skb); > struct sk_buff *sock_omalloc(struct sock *sk, unsigned long size, > gfp_t priority); > void skb_orphan_partial(struct sk_buff *skb); > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 2170bea2c7de..8cb6d360cda5 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -1804,30 +1804,39 @@ EXPORT_SYMBOL(skb_realloc_headroom); > struct sk_buff *skb_expand_head(struct sk_buff *skb, unsigned int headroom) > { > int delta = headroom - skb_headroom(skb); > + int osize = skb_end_offset(skb); > + struct sock *sk = skb->sk; > > if (WARN_ONCE(delta <= 0, > "%s is expecting an increase in the headroom", __func__)) > return skb; > > - /* pskb_expand_head() might crash, if skb is shared */ > - if (skb_shared(skb)) { > + delta = SKB_DATA_ALIGN(delta); > + /* pskb_expand_head() might crash, if skb is shared. */ > + if (skb_shared(skb) || !is_skb_wmem(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); > - } > + if (unlikely(!nskb)) > + goto fail; > + > + if (sk) > + skb_set_owner_w(nskb, sk); > + consume_skb(skb); > skb = nskb; > } > - if (skb && > - pskb_expand_head(skb, SKB_DATA_ALIGN(delta), 0, GFP_ATOMIC)) { > - kfree_skb(skb); > - skb = NULL; > + if (pskb_expand_head(skb, delta, 0, GFP_ATOMIC)) > + goto fail; > + > + if (sk && skb->destructor != sock_edemux) {
Why not re-using is_skb_wmem() here ? Testing != sock_edemux looks strange. > + delta = skb_end_offset(skb) - osize; > + refcount_add(delta, &sk->sk_wmem_alloc); > + skb->truesize += delta; > } > return skb; > + > +fail: > + kfree_skb(skb); > + return NULL; > } > EXPORT_SYMBOL(skb_expand_head); > > diff --git a/net/core/sock.c b/net/core/sock.c > index 62627e868e03..1932755ae9ba 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -2227,6 +2227,14 @@ void skb_set_owner_w(struct sk_buff *skb, struct sock *sk) > } > EXPORT_SYMBOL(skb_set_owner_w); > > +bool is_skb_wmem(const struct sk_buff *skb) > +{ > + return skb->destructor == sock_wfree || > + skb->destructor == __sock_wfree || > + (IS_ENABLED(CONFIG_INET) && skb->destructor == tcp_wfree); > +} > +EXPORT_SYMBOL(is_skb_wmem); > +
This probably should be inlined.
> static bool can_skb_orphan_partial(const struct sk_buff *skb) > { > #ifdef CONFIG_TLS_DEVICE >
| |