netdev - Re: [PATCH net-next] net: Correct wrong BH disable in hard-interrupt.

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, 18 Feb 2022 10:34:46 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Marek Szyprowski <m.szyprowski@...sung.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Felipe Balbi <balbi@...nel.org>, linux-usb@...r.kernel.org
Subject: Re: [PATCH net-next] net: Correct wrong BH disable in hard-interrupt.

On 2022-02-17 15:08:55 [+0100], Marek Szyprowski wrote:
> Hi All,
Hi,

> >> Marek, does this work for you?
> >
> > Yes, this fixed the issue. Thanks!
> >
> > Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
> 
> I've just noticed that there is one more issue left to fix (the $subject 
> patch is already applied) - this one comes from threaded irq (if I got 
> the stack trace right):

netif_rx() did only set the matching softirq bit and not more. Based on
that I don't see why NOHZ shouldn't complain about a pending softirq
once the CPU goes idle. Therefore I think the change I made is good
since it uncovered that.

> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 147 at kernel/softirq.c:363 
> __local_bh_enable_ip+0xa8/0x1c0
…
> CPU: 0 PID: 147 Comm: irq/150-dwc3 Not tainted 5.17.0-rc4-next-20220217+ 
> #4557
> Hardware name: Samsung TM2E board (DT)
> pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : __local_bh_enable_ip+0xa8/0x1c0
> lr : netif_rx+0xa4/0x2c0
> ...
> 
> Call trace:
>   __local_bh_enable_ip+0xa8/0x1c0
>   netif_rx+0xa4/0x2c0
>   rx_complete+0x214/0x250
>   usb_gadget_giveback_request+0x58/0x170
>   dwc3_gadget_giveback+0xe4/0x200
>   dwc3_gadget_endpoint_trbs_complete+0x100/0x388
>   dwc3_thread_interrupt+0x46c/0xe20

So dwc3_thread_interrupt() disables interrupts here. Felipe dropped it
and then added it back in
    e5f68b4a3e7b0 ("Revert "usb: dwc3: gadget: remove unnecessary _irqsave()"")

I would suggest to revert it (the above commit) and fixing the lockdep
splat in the gadget driver and other. I don't see the g_ether warning
Felipe mentioned. It might come from f_ncm (since it uses a timer) or
something else in the network stack (that uses a timeout timer).
But not now.
As much as I hate it, I suggest:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 183b90923f51b..a0c883f19a417 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4160,9 +4160,11 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void *_evt)
 	unsigned long flags;
 	irqreturn_t ret = IRQ_NONE;
 
+	local_bh_disable();
 	spin_lock_irqsave(&dwc->lock, flags);
 	ret = dwc3_process_event_buf(evt);
 	spin_unlock_irqrestore(&dwc->lock, flags);
+	local_bh_enable();
 
 	return ret;
 }


In the long run I would drop that irqsave (along with bh_disable() since
netif_rx() covers that) and make sure the there is no lockdep warning
popping up.

Marek, could you please give it a try?

Sebastian