Messages in this thread Patch in this message | | | From | Christoph Paasch <> | Date | Wed, 25 Aug 2021 10:49:15 -0700 | Subject | Re: [PATCH NET-NEXT] ipv6: skb_expand_head() adjust skb->truesize incorrectly |
| |
On Tue, Aug 24, 2021 at 10:22 AM Vasily Averin <vvs@virtuozzo.com> wrote: > > On 8/24/21 11:50 AM, Vasily Averin wrote: > > On 8/24/21 1:23 AM, Eric Dumazet wrote: > >> On 8/23/21 2:51 PM, Eric Dumazet wrote: > >>> 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. > > > > Can we adjust truesize outside pskb_expand_head()? > > Could you please explain why it can be not safe? > > Do you mean truesize change should not break balance of sk->sk_wmem_alloc?
AFAICS, that's the problem around adjusting truesize. So, maybe "just" refcount_add the increase of the truesize.
The below does fix the syzkaller bug for me and seems to do the right thing overall. But I honestly think that this is becoming too hacky and not worth it... and who knows what other corner-cases this now exposes...
Maybe a revert is a better course of action?
--- diff --git a/net/core/skbuff.c b/net/core/skbuff.c index f9311762cc47..9cc18a0fdd1c 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -71,6 +71,7 @@ #include <net/mpls.h> #include <net/mptcp.h> #include <net/page_pool.h> +#include <net/tcp.h>
#include <linux/uaccess.h> #include <trace/events/skb.h> @@ -1756,9 +1757,14 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, * For the moment, we really care of rx path, or * when skb is orphaned (not attached to a socket). */ - if (!skb->sk || skb->destructor == sock_edemux) + if (!skb->sk || skb->destructor == sock_edemux || skb->destructor == tcp_wfree) { skb->truesize += size - osize;
+ if (skb->sk && skb->destructor == tcp_wfree) { + refcount_add(size - osize, &skb->sk->sk_wmem_alloc); + } + } + return 0;
nofrags:
| |