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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87leysazrq.fsf@toke.dk>
Date:   Thu, 03 Feb 2022 13:41:13 +0100
From:   Toke Høiland-Jørgensen <toke@...e.dk>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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().

Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:

> On 2022-02-03 13:00:06 [+0100], Toke Høiland-Jørgensen wrote:
>> > Here is the code in larger context:
>> >
>> > #ifdef CONFIG_RPS
>> >     if (static_branch_unlikely(&rps_needed)) {
>> >         struct rps_dev_flow voidflow, *rflow = &voidflow;
>> >         int cpu;
>> >
>> >         preempt_disable();
>> >         rcu_read_lock();
>> >
>> >         cpu = get_rps_cpu(skb->dev, skb, &rflow);
>> >         if (cpu < 0)
>> >             cpu = smp_processor_id();
>> >
>> >         ret = enqueue_to_backlog(skb, cpu, &rflow->last_qtail);
>> >
>> >         rcu_read_unlock();
>> >         preempt_enable();
>> >     } else
>> > #endif
>> >
>> > This code needs the preempt_disable().
>> 
>> This is mostly so that the CPU ID stays the same throughout that section
>> of code, though, right? So wouldn't it work to replace the
>> preempt_disable() with a migrate_disable()? That should keep _RT happy,
>> no?
>
> 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?

-Toke