netdev - Re: [PATCH net-next v2 2/3] net: dev: Makes sure netif

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, 10 Feb 2022 13:22:32 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Jakub Kicinski <kuba@...nel.org>
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>,
        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>
Subject: Re: [PATCH net-next v2 2/3] net: dev: Makes sure netif_rx() can be
 invoked in any context.

On 2022-02-07 08:47:17 [-0800], Jakub Kicinski wrote:
> On Sat, 5 Feb 2022 21:36:05 +0100 Sebastian Andrzej Siewior wrote:
> > On 2022-02-04 20:17:15 [-0800], Jakub Kicinski wrote:
> > > On Fri,  4 Feb 2022 21:12:58 +0100 Sebastian Andrzej Siewior wrote:  
> > > > +int __netif_rx(struct sk_buff *skb)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	trace_netif_rx_entry(skb);
> > > > +	ret = netif_rx_internal(skb);
> > > > +	trace_netif_rx_exit(ret);
> > > > +	return ret;
> > > > +}  
> > > 
> > > Any reason this is not exported? I don't think there's anything wrong
> > > with drivers calling this function, especially SW drivers which already
> > > know to be in BH. I'd vote for roughly all of $(ls drivers/net/*.c) to
> > > get the same treatment as loopback.  
> > 
> > Don't we end up in the same situation as netif_rx() vs netix_rx_ni()?
> 
> Sort of. TBH my understanding of the motivation is a bit vague.
> IIUC you want to reduce the API duplication so drivers know what to
> do[1]. I believe the quote from Eric you put in the commit message
> pertains to HW devices, where using netif_rx() is quite anachronistic. 
> But software devices like loopback, veth or tunnels may want to go via
> backlog for good reasons. Would it make it better if we called
> netif_rx() netif_rx_backlog() instead? Or am I missing the point?

So we do netif_rx_backlog() with the bh disable+enable and
__netif_rx_backlog() without it and export both tree wide? It would make
it more obvious indeed. Could we add
	WARN_ON_ONCE(!(hardirq_count() | softirq_count()))
to the shortcut to catch the "you did it wrong folks"? This costs me
about 2ns.

TL;DR

The netix_rx_ni() is problematic on RT and I tried to do something about
it. I remembered from the in_atomic() cleanup that a few drivers got it
wrong (one way or another). We added also netif_rx_any_context() which
is used by some of the drivers (which is yet another entry point) while
the few other got fixed.
Then I stumbled over the thread where the entry (netif_rx() vs
netif_rx_ni()) was wrong and Dave suggested to have one entry point for
them all. This sounded like a good idea since it would eliminate the
several API entry points where things can go wrong and my RT trouble
would vanish in one go.
The part with deprecated looked promising but I didn't take into account
that the overhead for legitimate users (like the backlog or the software
tunnels you mention) is not acceptable.

Sebastian