netdev - Re: [PATCH net v4] xsk: avoid data corruption on cq descriptor number

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0485e77a-1566-48e7-a33e-5651a8e4beb5@suse.de>
Date: Thu, 20 Nov 2025 12:04:08 +0100
From: Fernando Fernandez Mancera <fmancera@...e.de>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: netdev@...r.kernel.org, csmate@....hu, maciej.fijalkowski@...el.com,
 bpf@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, horms@...nel.org
Subject: Re: [PATCH net v4] xsk: avoid data corruption on cq descriptor number



On 11/20/25 11:56 AM, Jason Xing wrote:
> On Thu, Nov 20, 2025 at 5:06 PM Fernando Fernandez Mancera
> <fmancera@...e.de> wrote:
>>
>>
>>
>> On 11/20/25 4:07 AM, Jason Xing wrote:
>>> On Tue, Nov 18, 2025 at 8:48 PM Fernando Fernandez Mancera
>>> <fmancera@...e.de> wrote:
>> [...]>> @@ -828,11 +840,20 @@ static struct sk_buff
>> *xsk_build_skb(struct xdp_sock *xs,
>>>>                                   goto free_err;
>>>>                           }
>>>>
>>>> -                       xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
>>>> -                       if (!xsk_addr) {
>>>> -                               __free_page(page);
>>>> -                               err = -ENOMEM;
>>>> -                               goto free_err;
>>>> +                       if (xsk_skb_destructor_is_addr(skb)) {
>>>> +                               xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache,
>>>> +                                                            GFP_KERNEL);
>>>> +                               if (!xsk_addr) {
>>>> +                                       __free_page(page);
>>>> +                                       err = -ENOMEM;
>>>> +                                       goto free_err;
>>>> +                               }
>>>> +
>>>> +                               xsk_addr->num_descs = 1;
>>>> +                               xsk_addr->addrs[0] = xsk_skb_destructor_get_addr(skb);
>>>> +                               skb_shinfo(skb)->destructor_arg = (void *)xsk_addr;
>>>> +                       } else {
>>>> +                               xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
>>>>                           }
>>>>
>>>>                           vaddr = kmap_local_page(page);
>>>> @@ -842,13 +863,11 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>>>                           skb_add_rx_frag(skb, nr_frags, page, 0, len, PAGE_SIZE);
>>>>                           refcount_add(PAGE_SIZE, &xs->sk.sk_wmem_alloc);
>>>>
>>>> -                       xsk_addr->addr = desc->addr;
>>>> -                       list_add_tail(&xsk_addr->addr_node, &XSKCB(skb)->addrs_list);
>>>> +                       xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
>>>> +                       xsk_addr->num_descs++;
>>>
>>> Wait, it's too late to increment it... Please find below.
>>>
>>>>                   }
>>>>           }
>>>>
>>>> -       xsk_inc_num_desc(skb);
>>>> -
>>
>>
>>
>>>>           return skb;
>>>>
>>>>    free_err:
>>>> @@ -857,7 +876,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>>>
>>>>           if (err == -EOVERFLOW) {
>>>>                   /* Drop the packet */
>>>> -               xsk_inc_num_desc(xs->skb);
>>>
>>> Why did you remove this line? The error can occur in the above hidden
>>> snippet[1] without IFF_TX_SKB_NO_LINEAR setting and then we will fail
>>> to increment it by one.
>>>
>>>
>> That is a good catch. Let me fix this logic.. I missed that the
>> -EOVERFLOW is returned in different moments for xsk_build_skb_zerocopy()
>> and xsk_build_skb(). Keeping the increment logic as it was it is better.
> 
> Right. Thanks!
> 
> My new solution based on net-next with your patch is ready now :)
> 

Awesome, thanks!

I just sent out the v5.

@Maciej I dropped your ACK since I changed the code. Feel free to add it 
back and sorry for making you take it a look to it again.

> Thanks,
> Jason
>