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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c7fb0c73-12e9-4a6d-94d9-65f7fc9514ce@suse.de>
Date: Thu, 20 Nov 2025 10:06:01 +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 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.

> [1]: https://elixir.bootlin.com/linux/v6.18-rc6/source/net/xdp/xsk.c#L821
> 
> Thanks,
> Jason