netdev - Re: [PATCH net 6/8] ice: improve updating ice_{t, r}x_ring::xsk

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: <20240709184524.232b9f57@kernel.org>
Date: Tue, 9 Jul 2024 18:45:24 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
 netdev@...r.kernel.org, Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
 magnus.karlsson@...el.com, aleksander.lobakin@...el.com, ast@...nel.org,
 daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com,
 bpf@...r.kernel.org, Shannon Nelson <shannon.nelson@....com>, Chandan Kumar
 Rout <chandanx.rout@...el.com>
Subject: Re: [PATCH net 6/8] ice: improve updating ice_{t,
 r}x_ring::xsk_pool

On Mon,  8 Jul 2024 15:14:12 -0700 Tony Nguyen wrote:
> @@ -1556,7 +1556,7 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
>  		 * comparison in the irq context instead of many inside the
>  		 * ice_clean_rx_irq function and makes the codebase cleaner.
>  		 */
> -		cleaned = rx_ring->xsk_pool ?
> +		cleaned = READ_ONCE(rx_ring->xsk_pool) ?
>  			  ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
>  			  ice_clean_rx_irq(rx_ring, budget_per_ring);
>  		work_done += cleaned;


> @@ -832,8 +839,8 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
>   */
>  int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
>  {
> +	struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
>  	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> -	struct xsk_buff_pool *xsk_pool = rx_ring->xsk_pool;
>  	u32 ntc = rx_ring->next_to_clean;
>  	u32 ntu = rx_ring->next_to_use;
>  	struct xdp_buff *first = NULL;

This looks suspicious, you need to at least explain why it's correct.
READ_ONCE() means one access per critical section, usually.
You access it at least twice in a single NAPI pool.