| 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
| ||
|
Message-ID: <CAJ+HfNjfcGW=b_Ckox4jXf7od7yr+Sk2dXxFyO8Qpr-WJX0q7A@mail.gmail.com>
Date: Thu, 9 May 2019 12:40:10 +0200
From: Björn Töpel <bjorn.topel@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: Toke Høiland-Jørgensen <toke@...hat.com>,
David Miller <davem@...emloft.net>,
Netdev <netdev@...r.kernel.org>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Alexei Starovoitov <ast@...nel.org>,
Jakub Kicinski <jakub.kicinski@...ronome.com>
Subject: Re: [PATCH net-next v4 1/6] net: xdp: refactor XDP attach
On Mon, 8 Apr 2019 at 22:23, Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 04/08/2019 07:05 PM, Toke Høiland-Jørgensen wrote:
> > From: Björn Töpel <bjorn.topel@...el.com>
> >
> > Generic XDP and driver XDP cannot be enabled at the same time. However,
> > they don't share any state; let's fix that. Here, dev->xdp_prog, is
> > used for both driver and generic mode. This removes the need for the
> > XDP_QUERY_PROG command to ndo_bpf.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>
> > ---
> > include/linux/netdevice.h | 8 +-
> > net/core/dev.c | 148 ++++++++++++++++++++++++++-------------------
> > net/core/rtnetlink.c | 14 +---
> > 3 files changed, 93 insertions(+), 77 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index eb9f05e0863d..68d4bbc44c63 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1957,7 +1957,7 @@ struct net_device {
> > struct cpu_rmap *rx_cpu_rmap;
> > #endif
> > struct hlist_node index_hlist;
> > -
> > + unsigned int xdp_target;
> > /*
> > * Cache lines mostly used on transmit path
> > */
> > @@ -2063,7 +2063,8 @@ struct net_device {
> >
> > static inline bool netif_elide_gro(const struct net_device *dev)
> > {
> > - if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> > + if (!(dev->features & NETIF_F_GRO) ||
> > + dev->xdp_target == XDP_FLAGS_SKB_MODE)
> > return true;
> > return false;
> > }
> > @@ -3702,8 +3703,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> > typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
> > int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > int fd, u32 flags);
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> > - enum bpf_netdev_command cmd);
> > +u32 __dev_xdp_query(struct net_device *dev, unsigned int target);
> > int xdp_umem_query(struct net_device *dev, u16 queue_id);
> >
> > int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index d5b1315218d3..feafc3580350 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -5121,35 +5121,25 @@ static void __netif_receive_skb_list(struct list_head *head)
> >
> > static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
> > {
> > - struct bpf_prog *old = rtnl_dereference(dev->xdp_prog);
> > struct bpf_prog *new = xdp->prog;
> > - int ret = 0;
> > -
> > - switch (xdp->command) {
> > - case XDP_SETUP_PROG:
> > - rcu_assign_pointer(dev->xdp_prog, new);
> > - if (old)
> > - bpf_prog_put(old);
> >
> > - if (old && !new) {
> > + if (xdp->command == XDP_SETUP_PROG) {
> > + if (static_key_enabled(&generic_xdp_needed_key) && !new) {
> > static_branch_dec(&generic_xdp_needed_key);
> > - } else if (new && !old) {
> > + } else if (new &&
> > + !static_key_enabled(&generic_xdp_needed_key)) {
> > static_branch_inc(&generic_xdp_needed_key);
> > dev_disable_lro(dev);
> > dev_disable_gro_hw(dev);
> > }
> > - break;
> >
> > - case XDP_QUERY_PROG:
> > - xdp->prog_id = old ? old->aux->id : 0;
> > - break;
> > + if (new)
> > + bpf_prog_put(new);
>
> Hmm, why you drop ref on the new program that you're installing here?
>
> >
> > - default:
> > - ret = -EINVAL;
> > - break;
> > + return 0;
> > }
> >
> > - return ret;
> > + return -EINVAL;
> > }
> >
> > static int netif_receive_skb_internal(struct sk_buff *skb)
> > @@ -7981,30 +7971,50 @@ int dev_change_proto_down_generic(struct net_device *dev, bool proto_down)
> > }
> > EXPORT_SYMBOL(dev_change_proto_down_generic);
> >
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> > - enum bpf_netdev_command cmd)
> > +u32 __dev_xdp_query(struct net_device *dev, unsigned int target)
> > {
> > - struct netdev_bpf xdp;
> > + if (target == XDP_FLAGS_DRV_MODE || target == XDP_FLAGS_SKB_MODE) {
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> >
> > - if (!bpf_op)
> > - return 0;
> > + return prog && (dev->xdp_target == target) ? prog->aux->id : 0;
> > + }
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = cmd;
> > + if (target == XDP_FLAGS_HW_MODE) {
> > + struct netdev_bpf xdp = { .command = XDP_QUERY_PROG_HW };
> >
> > - /* Query must always succeed. */
> > - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> > + if (!dev->netdev_ops->ndo_bpf)
> > + return 0;
> > + dev->netdev_ops->ndo_bpf(dev, &xdp);
> > + return xdp.prog_id;
> > + }
> >
> > - return xdp.prog_id;
> > + WARN_ON(1);
> > + return 0;
> > }
> >
> > -static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> > +static int dev_xdp_install(struct net_device *dev, unsigned int target,
> > struct netlink_ext_ack *extack, u32 flags,
> > struct bpf_prog *prog)
> > {
> > - struct netdev_bpf xdp;
> > + const struct net_device_ops *ops = dev->netdev_ops;
> > + struct bpf_prog *old = NULL;
> > + struct netdev_bpf xdp = {};
> > + int ret;
> > +
> > + if (target != XDP_FLAGS_HW_MODE) {
> > + if (prog) {
> > + prog = bpf_prog_inc(prog);
> > + if (IS_ERR(prog))
> > + return PTR_ERR(prog);
> > + }
> > +
> > + old = rtnl_dereference(dev->xdp_prog);
> > + if (!old && !prog)
> > + return 0;
> > + rcu_assign_pointer(dev->xdp_prog, prog);
> > + dev->xdp_target = prog ? target : 0;
> > + }
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > if (flags & XDP_FLAGS_HW_MODE)
> > xdp.command = XDP_SETUP_PROG_HW;
> > else
> > @@ -8013,7 +8023,24 @@ static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> > xdp.flags = flags;
> > xdp.prog = prog;
> >
> > - return bpf_op(dev, &xdp);
> > + if (target == XDP_FLAGS_SKB_MODE)
> > + ret = generic_xdp_install(dev, &xdp);
> > + else
> > + ret = ops->ndo_bpf(dev, &xdp);
> > +
> > + if (target != XDP_FLAGS_HW_MODE) {
> > + if (ret) {
> > + if (prog)
> > + bpf_prog_put(prog);
> > + rcu_assign_pointer(dev->xdp_prog, old);
> > + dev->xdp_target = old ? target : 0;
>
> Hmm, please correct me if I'm wrong, but from reading this patch (with following
> generic XDP as a simple example), it means: above we inc ref on prog, fetch old
> prog, assign new prog to dev->xdp_prog, call generic_xdp_install(). If one would
> fail there, we drop the ref of the prog that is currently attached to dev->xdp_prog
> (effectively a use-after-free?), and assign the old prog back. So basically we
> would shortly flake wrt prog assignment. I don't think this is a good design, the
> update should only really happen once everything else succeeded and there is nothing
> that can fail anymore. The existing code is doing exactly the latter, why diverge
> from this practice?! (Also, why special casing XDP_FLAGS_HW_MODE? Feels a bit like
> a hack tbh.)
>
Really late reply. :-(
Yeah, I need to rework this one. My loose plan was to add the XDP
program to the netdevice (skb/native being mutual exclusive), and
later the per-queue program into the netdev_rx_queue structure. I left
HW programs out, keeping the old query mechanism. It probably makes
sense to add the offloaded program to the netdevice (and later to the
netdev_rx_queue) as well. IOW, two XDP-programs: skb/native and
offloaded in the netdev structure.
The update flow was: 1. update the netdev structures, and 2. call the
ndo (if applicable). 3. revert if fail.
I'll update this patch to follow the existing pattern: 1. try changing
the program via the ndos (if applicable) 2. update the netdevice on
success.
Björn
> > + } else {
> > + if (old)
> > + bpf_prog_put(old);
> > + }
> > + }
> > +
> > + return ret;
> > }
> >
> > static void dev_xdp_uninstall(struct net_device *dev)
> > @@ -8021,27 +8048,28 @@ static void dev_xdp_uninstall(struct net_device *dev)
> > struct netdev_bpf xdp;
> > bpf_op_t ndo_bpf;
> >
> > - /* Remove generic XDP */
> > - WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> > + /* Remove generic/native XDP */
> > + WARN_ON(dev_xdp_install(dev, dev->xdp_target, NULL, 0, NULL));
> >
> > /* Remove from the driver */
> > ndo_bpf = dev->netdev_ops->ndo_bpf;
> > if (!ndo_bpf)
> > return;
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = XDP_QUERY_PROG;
> > - WARN_ON(ndo_bpf(dev, &xdp));
> > - if (xdp.prog_id)
> > - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> > - NULL));
> > -
> > - /* Remove HW offload */
> > memset(&xdp, 0, sizeof(xdp));
> > xdp.command = XDP_QUERY_PROG_HW;
> > if (!ndo_bpf(dev, &xdp) && xdp.prog_id)
> > - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> > - NULL));
> > + WARN_ON(dev_xdp_install(dev, XDP_FLAGS_HW_MODE, NULL,
> > + xdp.prog_flags, NULL));
> > +}
> > +
> > +static bool dev_validate_active_xdp(struct net_device *dev, unsigned int target)
> > +{
> > + if (target == XDP_FLAGS_DRV_MODE)
> > + return dev->xdp_target != XDP_FLAGS_SKB_MODE;
> > + if (target == XDP_FLAGS_SKB_MODE)
> > + return dev->xdp_target != XDP_FLAGS_DRV_MODE;
> > + return true;
> > }
> >
> > /**
> > @@ -8057,40 +8085,36 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > int fd, u32 flags)
> > {
> > const struct net_device_ops *ops = dev->netdev_ops;
> > - enum bpf_netdev_command query;
> > + bool offload, drv = !!ops->ndo_bpf;
> > struct bpf_prog *prog = NULL;
> > - bpf_op_t bpf_op, bpf_chk;
> > - bool offload;
> > + unsigned int target;
> > int err;
> >
> > ASSERT_RTNL();
> >
> > offload = flags & XDP_FLAGS_HW_MODE;
> > - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> > + target = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
> >
> > - bpf_op = bpf_chk = ops->ndo_bpf;
> > - if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
> > + if (!drv && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) {
> > NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> > return -EOPNOTSUPP;
> > }
> > - if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> > - bpf_op = generic_xdp_install;
> > - if (bpf_op == bpf_chk)
> > - bpf_chk = generic_xdp_install;
> > + if (!drv || (flags & XDP_FLAGS_SKB_MODE))
> > + target = XDP_FLAGS_SKB_MODE;
> > +
> > + if (!dev_validate_active_xdp(dev, target)) {
> > + NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
> > + return -EEXIST;
> > + }
> >
> > if (fd >= 0) {
> > - if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> > - NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
> > - return -EEXIST;
> > - }
> > if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) &&
> > - __dev_xdp_query(dev, bpf_op, query)) {
> > + __dev_xdp_query(dev, target)) {
> > NL_SET_ERR_MSG(extack, "XDP program already attached");
> > return -EBUSY;
> > }
> >
> > - prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
> > - bpf_op == ops->ndo_bpf);
> > + prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, drv);
> > if (IS_ERR(prog))
> > return PTR_ERR(prog);
> >
> > @@ -8101,7 +8125,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > }
> > }
> >
> > - err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> > + err = dev_xdp_install(dev, target, extack, flags, prog);
> > if (err < 0 && prog)
> > bpf_prog_put(prog);
> >
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index f9b964fd4e4d..d9c7fe34c3a1 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1362,25 +1362,17 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
> >
> > static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> > {
> > - const struct bpf_prog *generic_xdp_prog;
> > -
> > - ASSERT_RTNL();
> > -
> > - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> > - if (!generic_xdp_prog)
> > - return 0;
> > - return generic_xdp_prog->aux->id;
> > + return __dev_xdp_query(dev, XDP_FLAGS_SKB_MODE);
> > }
> >
> > static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> > {
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
> > + return __dev_xdp_query(dev, XDP_FLAGS_DRV_MODE);
> > }
> >
> > static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> > {
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > - XDP_QUERY_PROG_HW);
> > + return __dev_xdp_query(dev, XDP_FLAGS_HW_MODE);
> > }
> >
> > static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
> >
>
Powered by blists - more mailing lists