Re: [PATCH net 3/4] net: ti: icssg-prueth: Fix race condition for traffic from different network sockets

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 28 Apr 2025 17:34:58 +0530 Meghana Malladi wrote:
> When dealing with transmitting traffic from different network
> sockets to a single Tx channel, freeing the DMA descriptors can lead
> to kernel panic with the following error:
> 
> [  394.602494] ------------[ cut here ]------------
> [  394.607134] kernel BUG at lib/genalloc.c:508!
> [  394.611485] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
> 
> logs: https://gist.github.com/MeghanaMalladiTI/ad1d1da3b6e966bc6962c105c0b1d0b6
> 
> The above error was reproduced when sending XDP traffic from XSK
> socket along with network traffic from BSD socket. This causes
> a race condition leading to corrupted DMA descriptors. Fix this
> by adding spinlock protection while accessing the DMA descriptors
> of a Tx ring.

IDK how XSK vs normal sockets matters after what is now patch 4.
The only possible race you may be protecting against is pushing 
work vs completion. Please double check this is even needed, 
and if so fix the commit msg.

> Fixes: 62aa3246f462 ("net: ti: icssg-prueth: Add XDP support")
> Signed-off-by: Meghana Malladi <m-malladi@ti.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_common.c | 7 +++++++
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 4f45f2b6b67f..a120ff6fec8f 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -157,7 +157,9 @@ int emac_tx_complete_packets(struct prueth_emac *emac, int chn,
>  	tx_chn = &emac->tx_chns[chn];
>  
>  	while (true) {
> +		spin_lock(&tx_chn->lock);
>  		res = k3_udma_glue_pop_tx_chn(tx_chn->tx_chn, &desc_dma);
> +		spin_unlock(&tx_chn->lock);
>  		if (res == -ENODATA)
>  			break;
>  
> @@ -325,6 +327,7 @@ int prueth_init_tx_chns(struct prueth_emac *emac)
>  		snprintf(tx_chn->name, sizeof(tx_chn->name),
>  			 "tx%d-%d", slice, i);
>  
> +		spin_lock_init(&tx_chn->lock);
>  		tx_chn->emac = emac;
>  		tx_chn->id = i;
>  		tx_chn->descs_num = PRUETH_MAX_TX_DESC;
> @@ -627,7 +630,9 @@ u32 emac_xmit_xdp_frame(struct prueth_emac *emac,
>  	cppi5_hdesc_set_pktlen(first_desc, xdpf->len);
>  	desc_dma = k3_cppi_desc_pool_virt2dma(tx_chn->desc_pool, first_desc);
>  
> +	spin_lock_bh(&tx_chn->lock);
>  	ret = k3_udma_glue_push_tx_chn(tx_chn->tx_chn, first_desc, desc_dma);
> +	spin_unlock_bh(&tx_chn->lock);

I'm afraid this needs to be some form of spin_lock_irq
The completions may run from hard irq context when netpoll/netconsole
is used.
-- 
pw-bot: cr