netdev - Re: [PATCH net] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <32086239-2fd5-42bd-a554-d0e3c263bb0c@suse.de>
Date: Tue, 21 Oct 2025 20:25:00 +0200
From: Fernando Fernandez Mancera <fmancera@...e.de>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, csmate@....hu, kerneljasonxing@...il.com,
 maciej.fijalkowski@...el.com, bjorn@...nel.org, sdf@...ichev.me,
 jonathan.lemon@...il.com, bpf@...r.kernel.org
Subject: Re: [PATCH net] xsk: avoid data corruption on cq descriptor number



On 10/21/25 6:25 PM, Simon Horman wrote:
> On Tue, Oct 21, 2025 at 05:06:56PM +0200, Fernando Fernandez Mancera wrote:
> 
> ...
> 
>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> 
> ...
> 
>> @@ -774,6 +777,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>   {
>>   	struct net_device *dev = xs->dev;
>>   	struct sk_buff *skb = xs->skb;
>> +	struct page *page;
>>   	int err;
>>   
>>   	if (dev->priv_flags & IFF_TX_SKB_NO_LINEAR) {
>> @@ -791,6 +795,8 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>   		len = desc->len;
>>   
>>   		if (!skb) {
>> +			struct xsk_addr_node *head_addr;
>> +
>>   			hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(dev->needed_headroom));
>>   			tr = dev->needed_tailroom;
>>   			skb = sock_alloc_send_skb(&xs->sk, hr + len + tr, 1, &err);
>> @@ -804,7 +810,13 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>   			if (unlikely(err))
>>   				goto free_err;
>>   
>> -			xsk_skb_init_misc(skb, xs, desc->addr);
>> +			head_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
>> +			if (!head_addr) {
>> +				__free_page(page);
> 
> Hi Fernando,
> 
> Perhaps the page changes to xsk_build_skb() aren't needed
> because page seems to be uninitialised here.
> 
> Flagged by W=1 builds with Clang 21.1.1, and Smatch.
> 

Ah, I don't know know what I was thinking about.. but yes, that page 
handling isn't needed of course as it isn't initialized. (I even needed 
the page struct because it wasn't available in that context).

I will send a V2 patch if the proposed solution is accepted. In additon, 
the performance impact isn't as bad as I thought.. just 4 bytes pero 
skb.. as the cache must be initialized anyway.

>> +				err = -ENOMEM;
>> +				goto free_err;
>> +			}
>> +			xsk_skb_init_misc(skb, xs, head_addr, desc->addr);
>>   			if (desc->options & XDP_TX_METADATA) {
>>   				err = xsk_skb_metadata(skb, buffer, desc,
>>   						       xs->pool, hr);
>> @@ -814,7 +826,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>   		} else {
>>   			int nr_frags = skb_shinfo(skb)->nr_frags;
>>   			struct xsk_addr_node *xsk_addr;
>> -			struct page *page;
>>   			u8 *vaddr;
>>   
>>   			if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc))) {
>> @@ -843,7 +854,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
>>   			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);
>> +			list_add_tail(&xsk_addr->addr_node, &XSK_TX_HEAD(skb)->addr_node);
>>   		}
>>   	}
>>   
>> -- 
>> 2.51.0
>>
>>