netdev - Re: [PATCH 2/2] net: ks8851: Handle softirqs at the end of IRQ thread to fix hang

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] [thread-next>] [day] [month] [year] [list]

Date: Mon, 1 Apr 2024 12:41:23 +0200
From: Marek Vasut <marex@...x.de>
To: Ratheesh Kannoth <rkannoth@...vell.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Dmitry Torokhov <dmitry.torokhov@...il.com>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Mark Brown <broonie@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Ronald Wahl <ronald.wahl@...itan.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH 2/2] net: ks8851: Handle softirqs at the end of IRQ thread
 to fix hang

On 4/1/24 6:18 AM, Ratheesh Kannoth wrote:
> On 2024-03-31 at 19:51:46, Marek Vasut (marex@...x.de) wrote:
>> The ks8851_irq() thread may call ks8851_rx_pkts() in case there are
>> any packets in the MAC FIFO, which calls netif_rx(). This netif_rx()
>> implementation is guarded by local_bh_disable() and local_bh_enable().
>> The local_bh_enable() may call do_softirq() to run softirqs in case
>> any are pending. One of the softirqs is net_rx_action, which ultimately
>> reaches the driver .start_xmit callback. If that happens, the system
>> hangs. The entire call chain is below:
>>
>> ks8851_start_xmit_par from netdev_start_xmit
>> netdev_start_xmit from dev_hard_start_xmit
>> dev_hard_start_xmit from sch_direct_xmit
>> sch_direct_xmit from __dev_queue_xmit
>> __dev_queue_xmit from __neigh_update
>> __neigh_update from neigh_update
>> neigh_update from arp_process.constprop.0
>> arp_process.constprop.0 from __netif_receive_skb_one_core
>> __netif_receive_skb_one_core from process_backlog
>> process_backlog from __napi_poll.constprop.0
>> __napi_poll.constprop.0 from net_rx_action
>> net_rx_action from __do_softirq
>> __do_softirq from call_with_stack
>> call_with_stack from do_softirq
>> do_softirq from __local_bh_enable_ip
>> __local_bh_enable_ip from netif_rx
>> netif_rx from ks8851_irq
>> ks8851_irq from irq_thread_fn
>> irq_thread_fn from irq_thread
>> irq_thread from kthread
>> kthread from ret_from_fork
>>
>> The hang happens because ks8851_irq() first locks a spinlock in
>> ks8851_par.c ks8851_lock_par() spin_lock_irqsave(&ksp->lock, ...)
>> and with that spinlock locked, calls netif_rx(). Once the execution
>> reaches ks8851_start_xmit_par(), it calls ks8851_lock_par() again
>> which attempts to claim the already locked spinlock again, and the
>> hang happens.
>>
>> Move the do_softirq() call outside of the spinlock protected section
>> of ks8851_irq() by disabling BHs around the entire spinlock protected
>> section of ks8851_irq() handler. Place local_bh_enable() outside of
>> the spinlock protected section, so that it can trigger do_softirq()
>> without the ks8851_par.c ks8851_lock_par() spinlock being held, and
>> safely call ks8851_start_xmit_par() without attempting to lock the
>> already locked spinlock.
>>
>> Since ks8851_irq() is protected by local_bh_disable()/local_bh_enable()
>> now, replace netif_rx() with __netif_rx() which is not duplicating the
>> local_bh_disable()/local_bh_enable() calls.

[...]

>> diff --git a/drivers/net/ethernet/micrel/ks8851_common.c b/drivers/net/ethernet/micrel/ks8851_common.c
>> index 896d43bb8883d..b6b727e651f3d 100644
>> --- a/drivers/net/ethernet/micrel/ks8851_common.c
>> +++ b/drivers/net/ethernet/micrel/ks8851_common.c
>> @@ -299,7 +299,7 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>>   					ks8851_dbg_dumpkkt(ks, rxpkt);
>>
>>   				skb->protocol = eth_type_trans(skb, ks->netdev);
>> -				netif_rx(skb);
>> +				__netif_rx(skb);
>>
>>   				ks->netdev->stats.rx_packets++;
>>   				ks->netdev->stats.rx_bytes += rxlen;
>> @@ -325,11 +325,15 @@ static void ks8851_rx_pkts(struct ks8851_net *ks)
>>    */
>>   static irqreturn_t ks8851_irq(int irq, void *_ks)
>>   {
>> +	bool need_bh_off = !(hardirq_count() | softirq_count());
> IMO, in_task() macro would be better.

I don't think in_task() is exactly identical to !(hardirq_count() | 
softirq_count()) according to include/linux/preempt.h , it also takes 
NMI into consideration. I am not sure if that could pose a problem or not ?

This test here has been taken from net/core/dev.c netif_rx() , it is the 
same one used there around __netif_rx() invocation.

>>   	struct ks8851_net *ks = _ks;
>>   	unsigned handled = 0;
>>   	unsigned long flags;
>>   	unsigned int status;
>>
>> +	if (need_bh_off)
>> +		local_bh_disable();
> This threaded irq's thread function (ks8851_irq()) will always run in process context, right ?

I think so.

> Do you need "if(need_bh_off)" loop?

It is not a loop, it is invoked once. It is here to disable BHs so that 
the net_rx_action BH wouldn't run until after the spinlock protected 
section of the IRQ handler. Te net_rx_action may end up calling 
ks8851_start_xmit_par, which must be called with the spinlock released, 
otherwise the system would lock up.