linux-kernel - Re: [PATCH] net: macb: Fix tx_ptr

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: <b4e5cc2c-ca62-467c-9b58-52831ebeb032@linux.dev>
Date: Thu, 28 Aug 2025 13:19:41 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Robert Hancock <robert.hancock@...ian.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "edumazet@...gle.com" <edumazet@...gle.com>,
 "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
 "davem@...emloft.net" <davem@...emloft.net>,
 "pabeni@...hat.com" <pabeni@...hat.com>, "kuba@...nel.org" <kuba@...nel.org>
Cc: "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>,
 "efault@....de" <efault@....de>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "claudiu.beznea@...on.dev" <claudiu.beznea@...on.dev>
Subject: Re: [PATCH] net: macb: Fix tx_ptr_lock locking

On 8/28/25 12:13, Robert Hancock wrote:
> On Thu, 2025-08-28 at 12:00 -0400, Sean Anderson wrote:
>> macb_start_xmit can be called with bottom-halves disabled (e.g.
>> transmitting from softirqs) as well as with interrupts disabled (with
>> netpoll). Because of this, all other functions taking tx_ptr_lock
>> must
>> disable IRQs, and macb_start_xmit must only re-enable IRQs if they
>> were already enabled.
>> 
>> Fixes: 138badbc21a0 ("net: macb: use NAPI for TX completion path")
>> Reported-by: Mike Galbraith <efault@....de>
>> Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
>> ---
>> 
>>  drivers/net/ethernet/cadence/macb_main.c | 25 ++++++++++++----------
>> --
>>  1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 16d28a8b3b56..b0a8dfa341ea 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -1228,7 +1228,7 @@ static int macb_tx_complete(struct macb_queue
>> *queue, int budget)
>>         int packets = 0;
>>         u32 bytes = 0;
>> 
>> -       spin_lock(&queue->tx_ptr_lock);
>> +       spin_lock_irq(&queue->tx_ptr_lock);
>> 
> 
> Hm, I think I used a non-IRQ lock here to avoid potentially disabling
> interrupts for so long during TX completion processing. I don't think I
> considered the netpoll case where start_xmit can be called with IRQs
> disabled however. Not sure if there is a better solution to satisfy
> that case without turning IRQs off entirely here?

Well, we have a single producer (macb_start_xmit) so we don't need to
take a lock to enqueue anything as long as we add barriers in the right
places.

--Sean