[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <7e91a1c1-237d-463d-8045-eb7ca9e8c8df@ti.com>
Date: Fri, 2 May 2025 15:01:53 +0530
From: "Malladi, Meghana" <m-malladi@...com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <dan.carpenter@...aro.org>, <john.fastabend@...il.com>, <hawk@...nel.org>,
<daniel@...earbox.net>, <ast@...nel.org>, <pabeni@...hat.com>,
<edumazet@...gle.com>, <davem@...emloft.net>, <andrew+netdev@...n.ch>,
<bpf@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<netdev@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
<srk@...com>, Vignesh Raghavendra <vigneshr@...com>,
Roger Quadros
<rogerq@...nel.org>, <danishanwar@...com>
Subject: Re: [PATCH net 3/4] net: ti: icssg-prueth: Fix race condition for
traffic from different network sockets
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@...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
Powered by blists - more mailing lists