netdev - Re: [PATCH net-next 1/4] net: dev: Remove the preempt_disable() in netif_rx

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:   Thu, 3 Feb 2022 16:50:47 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Toke Høiland-Jørgensen <toke@...e.dk>
Cc:     Eric Dumazet <edumazet@...gle.com>, bpf <bpf@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH net-next 1/4] net: dev: Remove the preempt_disable() in
 netif_rx_internal().

On 2022-02-03 13:41:13 [+0100], Toke Høiland-Jørgensen wrote:
> Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:
> 
> > It would but as mentioned previously: BH is disabled and
> > smp_processor_id() is stable.
> 
> Ah, right, because of the change in loopback to use netif_rx_ni()? But
> that bit of the analysis only comes later in your series, so at the very
> least you should be explaining this in the commit message here. Or you
> could potentially squash patches 1 and 2 and do both changes at once,
> since it's changing two bits of the same function and both need the same
> analysis...
> 
> However, if we're going with Eric's suggestion of an internal
> __netif_rx() for loopback that *doesn't* do local_bh_disable() then this
> code would end up being called without BH disable, so we'd need the
> migrate_disable() anyway, no?

Eric suggested to the __netif_rx() for loopback which is already in
BH-disabled section. So if that is the only "allowed" caller, we
wouldn't have to worry.
If __netif_rx() becomes more users and one calls it from preemptible
context then we have a problem (like netif_rx() vs netif_rx_ni()).

migrate_disable() will shut up smp_processor_id(), yes, but we need
something to process pending softirqs. Otherwise they are delayed until
the next IRQ, spin_unlock_bh(), etc.

> -Toke

Sebastian