netdev - Re: xdp generic default option

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, 20 Aug 2020 10:25:39 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>,
        Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>, brouer@...hat.com,
        David Ahern <dsahern@...il.com>,
        Toke Høiland-Jørgensen 
        <toke@...hat.com>
Subject: Re: xdp generic default option

On Wed, 19 Aug 2020 13:57:51 -0700
Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:

> On Wed, Aug 19, 2020 at 2:29 AM Lorenzo Bianconi <lorenzo@...nel.org> wrote:
> >
> > Hi Andrii,
> >
> > working on xdp multi-buff I figured out now xdp generic is the default choice
> > if not specified by userspace. In particular after commit 7f0a838254bd
> > ("bpf, xdp: Maintain info on attached XDP BPF programs in net_device"), running
> > the command below, XDP will run in generic mode even if the underlay driver
> > support XDP in native mode:
> >
> > $ip link set dev eth0 xdp obj prog.o
> > $ip link show dev eth0
> > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 xdpgeneric qdisc mq state UP mode DEFAULT
> >    group default qlen 1024
> >    link/ether f0:ad:4e:09:6b:57 brd ff:ff:ff:ff:ff:ff
> >    prog/xdp id 1 tag 3b185187f1855c4c jited
> >
> > Is it better to use xdpdrv as default choice if not specified by userspace?
> > doing something like:
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index a00aa737ce29..1f85880ee412 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8747,9 +8747,9 @@ static enum bpf_xdp_mode dev_xdp_mode(u32 flags)
> >  {
> >         if (flags & XDP_FLAGS_HW_MODE)
> >                 return XDP_MODE_HW;
> > -       if (flags & XDP_FLAGS_DRV_MODE)
> > -               return XDP_MODE_DRV;
> > -       return XDP_MODE_SKB;
> > +       if (flags & XDP_FLAGS_SKB_MODE)
> > +               return XDP_MODE_SKB;
> > +       return XDP_MODE_DRV;
> >  }
> >  
> 
> I think the better way would be to choose XDP_MODE_DRV if ndo_bpf !=
> NULL and XDP_MODE_SKB otherwise. That seems to be matching original
> behavior, no?

Yes, but this silent fallback to XDP_MODE_SKB (generic-XDP) have
cause a lot of support issues in the past.  I wish we could change it.
We already changed all the samples/bpf/ to ask for XDP_FLAGS_DRV_MODE,
so they behave this way.

d50ecc46d18f ("samples/bpf: Attach XDP programs in driver mode by default")
 https://git.kernel.org/torvalds/c/d50ecc46d18fa

> It was not my intent to change the behavior, sorry about that. I'll
> post patch a bit later today.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer