Re: [PATCH NET-NEXT] ipv6: skb_expand_head() adjust skb->truesize incorrectly

[Eric Dumazet]

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@xxxxxxxxxxxxx> 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@xxxxxxxxx>
>>> Signed-off-by: Vasily Averin <vvs@xxxxxxxxxxxxx>
>>> ---
>>>  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.