netdev - Re: [PATCH RFC] r8152: Ensure that napi

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:   Fri, 14 May 2021 16:36:25 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-usb@...r.kernel.org, netdev@...r.kernel.org,
        Michal Svec <msvec@...e.com>,
        "David S. Miller" <davem@...emloft.net>,
        Hayes Wang <hayeswang@...ltek.com>,
        Thierry Reding <treding@...dia.com>,
        Lee Jones <lee.jones@...aro.org>,
        Borislav Petkov <bp@...en8.de>,
        LKML <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Boqun Feng <boqun.feng@...il.com>
Subject: Re: [PATCH RFC] r8152: Ensure that napi_schedule() is handled

On Sat, 15 May 2021 01:23:02 +0200 Thomas Gleixner wrote:
> On Fri, May 14 2021 at 14:41, Jakub Kicinski wrote:
> >> This is not related to force_irqthreads at all. This very driver invokes
> >> it from plain thread context.  
> >
> > I see, but a driver calling __napi_schedule_irqoff() from its IRQ
> > handler _would_ be an issue, right? Or do irq threads trigger softirq
> > processing on exit?  
> 
> Yes, they do. See irq_forced_thread_fn(). It has a local_bh_disable() /
> local_bh_ enable() pair around the invocation to ensure that.

Ah, excellent!

> >> You could have napi_schedule_intask() or something like that which would
> >> do the local_bh_disable()/enable() dance around the invocation of
> >> napi_schedule(). That would also document it clearly in the drivers. A
> >> quick grep shows a bunch of instances which could be replaced:
> >> 
> >> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c-5704-		local_bh_disable();
> >> drivers/net/ethernet/mellanox/mlx4/en_netdev.c-1830-		local_bh_disable();
> >> drivers/net/usb/r8152.c-1552-	local_bh_disable();
> >> drivers/net/virtio_net.c-1355-	local_bh_disable();
> >> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-1650-	local_bh_disable();
> >> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2015-		local_bh_disable();
> >> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2225-		local_bh_disable();
> >> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2235-		local_bh_disable();
> >> drivers/s390/net/qeth_core_main.c-3515-	local_bh_disable();  
> >
> > Very well aware, I've just sent a patch for mlx5 last week :)
> >
> > My initial reaction was the same as yours - we should add lockdep
> > check, and napi_schedule_intask(). But then I started wondering
> > if it's all for nothing on rt or with force_irqthreads, and therefore
> > we should just eat the extra check.  
> 
> We can make that work but sure I'm not going to argue when you decide to
> just go for raise_softirq_irqsoff().
> 
> I just hacked that check up which is actually useful beyond NAPI. It's
> straight forward except for that flush_smp_call_function_from_idle()
> oddball, which immeditately triggered that assert because block mq uses
> __raise_softirq_irqsoff() in a smp function call...
> 
> See below. Peter might have opinions though :)

Looks good to me, since my thinking that RT complicates things here was
wrong I'm perfectly happy with the lockdep + napi_schedule_intask().