netdev - Re: [PATCH net v2] vmxnet3: Fix tx queue race condition with XDP

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: <20250129171501.7d120cae@kernel.org>
Date: Wed, 29 Jan 2025 17:15:01 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Sankararaman Jayaraman <sankararaman.jayaraman@...adcom.com>
Cc: alexanderduyck@...com, alexandr.lobakin@...el.com,
 andrew+netdev@...n.ch, ast@...nel.org,
 bcm-kernel-feedback-list@...adcom.com, bpf@...r.kernel.org,
 daniel@...earbox.net, davem@...emloft.net, edumazet@...gle.com,
 hawk@...nel.org, john.fastabend@...il.com, netdev@...r.kernel.org,
 pabeni@...hat.com, ronak.doshi@...adcom.com, u9012063@...il.com
Subject: Re: [PATCH net v2] vmxnet3: Fix tx queue race condition with XDP

On Wed, 29 Jan 2025 23:47:03 +0530 Sankararaman Jayaraman wrote:
> If XDP traffic runs on a CPU which is greater than or equal to
> the number of the Tx queues of the NIC, then vmxnet3_xdp_get_tq()
> always picks up queue 0 for transmission as it uses reciprocal scale
> instead of simple modulo operation.
> 
> vmxnet3_xdp_xmit() and vmxnet3_xdp_xmit_frame() use the above
> returned queue without any locking which can lead to race conditions
> when multiple XDP xmits run in parallel on different CPU's.
> 
> This patch uses a simple module scheme when the current CPU equals or
> exceeds the number of Tx queues on the NIC. It also adds locking in
> vmxnet3_xdp_xmit() and vmxnet3_xdp_xmit_frame() functions.
> 
> Fixes: 54f00cce1178 ("vmxnet3: Add XDP support.")
> Signed-off-by: Sankararaman Jayaraman <sankararaman.jayaraman@...adcom.com>
> Signed-off-by: Ronak Doshi <ronak.doshi@...adcom.com>

Please add a --- separator between commit message and change log

> Changes v1-> v2:
> Retained the copyright dates as it is.
> Used spin_lock()/spin_unlock() instead of spin_lock_irqsave(). 

Wrong way around AFAICT. The lock is taken on the xmit path,
and driver supports netpoll. But this path won't be called
from IRQ. So the right type of call is very likely _irq().

Please do not post next version of the patch in reply to previous
posting. Instead add to the change log a lore link to previous
posting. See:
https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#changes-requested
Actually, also make sure you read at least the tl;dr section, too.

> @@ -226,6 +231,7 @@ vmxnet3_xdp_xmit(struct net_device *dev,
>  	struct vmxnet3_adapter *adapter = netdev_priv(dev);
>  	struct vmxnet3_tx_queue *tq;
>  	int i;
> +	struct netdev_queue *nq;

Reverse length order. So:

 	struct vmxnet3_adapter *adapter = netdev_priv(dev);
 	struct vmxnet3_tx_queue *tq;
+	struct netdev_queue *nq;
 	int i;
-- 
pw-bot: cr