netdev - Re: [PATCH net-next 3/4] 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, 3 Feb 2022 16:10:38 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     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 3/4] net: dev: Makes sure netif_rx() can be
 invoked in any context.

On 2022-02-02 09:43:14 [-0800], Eric Dumazet wrote:
> Maybe worth mentioning this commit will show a negative impact, for
> network traffic
> over loopback interface.
> 
> My measure of the cost of local_bh_disable()/local_bh_enable() is ~6
> nsec on one of my lab x86 hosts.

So you are worried that 
    dev_loopback_xmit() -> netif_rx_ni()

becomes
    dev_loopback_xmit() -> netif_rx()

and by that 6nsec slower because of that bh off/on? Can these 6nsec get
a little lower if we substract the overhead of preempt-off/on? 
But maybe I picked the wrong loopback here.

> Perhaps we could have a generic netif_rx(), and a __netif_rx() for the
> virtual drivers (lo and maybe tunnels).
> 
> void __netif_rx(struct sk_buff *skb);
> 
> static inline int netif_rx(struct sk_buff *skb)
> {
>    int res;
>     local_bh_disable();
>     res = __netif_rx(skb);
>   local_bh_enable();
>   return res;
> }

But what is __netif_rx() doing? netif_rx_ni() has this part:

|       preempt_disable();
|       err = netif_rx_internal(skb);
|       if (local_softirq_pending())
|               do_softirq();
|       preempt_enable();

to ensure that smp_processor_id() and friends are quiet plus any raised
softirqs are processed. With the current netif_rx() we end up with:

|       local_bh_disable();
|       ret = netif_rx_internal(skb);
|       local_bh_enable();

which provides the same. Assuming __netif_rx() as:

| int __netif_rx(skb)
| {
|         trace_netif_rx_entry(skb);
| 
|         ret = netif_rx_internal(skb);
|         trace_netif_rx_exit(ret);
| 
|         return ret;
| }

and the loopback interface is not invoking this in_interrupt() context.

Sebastian