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

["Malladi, Meghana" <m-malladi@ti.com>]

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@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.

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