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

[Eric Dumazet]

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@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.
> 

I also note that current use of skb_set_owner_w(), forcing skb->destructor to sock_wfree()
is probably breaking TCP Small queues, since original skb->destructor would be tcp_wfree() or __sock_wfree()