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

[Malladi, Meghana]

Hi Jakub,

On 5/1/2025 8:26 PM, Jakub Kicinski wrote:
> 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.

I can think of race conditions happening in the following cases:
1. Multiport use cases where traffic is being handled on more than one 
interface to a single Tx channel.
2. Having emac_xmit_xdp_frame() and icssg_ndo_start_xmit(), two 
different traffics being transmitted over a single interface to a single 
tx channel.

In both of the above scenarios Tx channel is a common resource which 
needs to be protected from any race conditions, which might happen 
during Tx descriptor push/pop. As suggested by you, I am currently 
excluding this patch and doing some stress testing. Regardless 
conceptually I still think spinlock is needed, please do correct me if I 
am wrong.

> > Fixes: 62aa3246f462 ("net: ti: icssg-prueth: Add XDP support")
> > Signed-off-by: Meghana Malladi <m-malladi@xxxxxx>
> > ---
> >   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.

Didn't know system can handle network interrupts in a hard IRQ context. 
Ok I will update to spin_lock_irq() if this patch is necessary.

--
Thanks,
Meghana Malladi