netdev - Re: Co-existing XDP generic and native mode? (Re: [PATCH bpf-next v5 5/8] xdp: Provide extack messages when prog attachment failed)

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:   Fri, 1 Feb 2019 13:44:03 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Jesper Dangaard Brouer <brouer@...hat.com>, ast@...nel.org,
        David Miller <davem@...emloft.net>,
        Maciej Fijalkowski <maciejromanfijalkowski@...il.com>,
        netdev@...r.kernel.org, john.fastabend@...il.com,
        David Ahern <dsahern@...il.com>,
        Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: Co-existing XDP generic and native mode? (Re: [PATCH bpf-next
 v5 5/8] xdp: Provide extack messages when prog attachment failed)

On Fri, 1 Feb 2019 22:33:22 +0100, Daniel Borkmann wrote:
> On 02/01/2019 07:47 PM, Jakub Kicinski wrote:
> >> These are only refactor ideas, so if you can argue why your internal
> >> feature request for simultaneous generic and native make more sense,
> >> then I'm open for allowing this ?  
> > 
> > The request was actually to enable xdpoffload and xdpgeneric at the
> > same time.  I'm happy to have that as another HW offload exclusive
> > for now :)  
> 
> The latter is probably fine, though what's the concrete use case? :)

I think it was more of a "I expected this to work, since driver+offload
worked" than a feature request.  Looking back at it it was filed as bug
I converted it to feature.

> Reason we kept native vs generic separate is mainly so that native XDP
> drivers are discouraged to punt missing features to generic hook instead
> of properly implementing them in native mode.

Agreed, although one could make a counter argument that the
performance should be a strong enough incentive and we shouldn't stop
people for experimenting and prototyping :)  The code change looks
simple enough:

diff --git a/net/core/dev.c b/net/core/dev.c
index 8e276e0192a1..ce4880e5e95d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7976,11 +7976,13 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
        enum bpf_netdev_command query;
        struct bpf_prog *prog = NULL;
        bpf_op_t bpf_op, bpf_chk;
+       bool offload;
        int err;
 
        ASSERT_RTNL();
 
-       query = flags & XDP_FLAGS_HW_MODE ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
+       offload = flags & XDP_FLAGS_HW_MODE;
+       query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
 
        bpf_op = bpf_chk = ops->ndo_bpf;
        if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE)))
@@ -7991,8 +7993,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
                bpf_chk = generic_xdp_install;
 
        if (fd >= 0) {
-               if (__dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG) ||
-                   __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW))
+               if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG))
                        return -EEXIST;
                if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) &&
                    __dev_xdp_query(dev, bpf_op, query))
@@ -8003,8 +8004,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
                if (IS_ERR(prog))
                        return PTR_ERR(prog);
 
-               if (!(flags & XDP_FLAGS_HW_MODE) &&
-                   bpf_prog_is_dev_bound(prog->aux)) {
+               if (!offload && bpf_prog_is_dev_bound(prog->aux)) {
                        NL_SET_ERR_MSG(extack, "using device-bound program without HW_MODE flag is not supported");
                        bpf_prog_put(prog);
                        return -EINVAL;

Do you think we shouldn't do it?