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 07:25:01 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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 Thu, Feb 3, 2022 at 7:10 AM Sebastian Andrzej Siewior
<bigeasy@...utronix.de> wrote:
>
> 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()


No, the loopback device (ifconfig log) I am referring to is in
drivers/net/loopback.c

loopback_xmit() calls netif_rx() directly, while bh are already disabled.

>
> 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

Instead of adding a local_bh_disable()/local_bh_enable() in netif_rx()
I suggested
to rename current netif_rx() to __netif_rx() and add a wrapper, eg :

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e490b84732d1654bf067b30f2bb0b0825f88dea9..39232d99995cbd54c74e85905bb4af43b5b301ca
100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3668,7 +3668,17 @@ u32 bpf_prog_run_generic_xdp(struct sk_buff
*skb, struct xdp_buff *xdp,
                             struct bpf_prog *xdp_prog);
 void generic_xdp_tx(struct sk_buff *skb, struct bpf_prog *xdp_prog);
 int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb);
-int netif_rx(struct sk_buff *skb);
+int __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;
+}
+
 int netif_rx_ni(struct sk_buff *skb);
 int netif_rx_any_context(struct sk_buff *skb);
 int netif_receive_skb(struct sk_buff *skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 1baab07820f65f9bcf88a6d73e2c9ff741d33c18..f962e549e0bfea96cdba5bc7e1d8694e46652eac
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4819,7 +4819,7 @@ static int netif_rx_internal(struct sk_buff *skb)
 }

 /**
- *     netif_rx        -       post buffer to the network code
+ *     __netif_rx      -       post buffer to the network code
  *     @skb: buffer to post
  *
  *     This function receives a packet from a device driver and queues it for
@@ -4833,7 +4833,7 @@ static int netif_rx_internal(struct sk_buff *skb)
  *
  */

-int netif_rx(struct sk_buff *skb)
+int __netif_rx(struct sk_buff *skb)
 {
        int ret;

@@ -4844,7 +4844,7 @@ int netif_rx(struct sk_buff *skb)

        return ret;
 }
-EXPORT_SYMBOL(netif_rx);
+EXPORT_SYMBOL(__netif_rx);

 int netif_rx_ni(struct sk_buff *skb)
 {