Re: [PATCH net-next v4 1/6] net: xdp: refactor XDP attach

["Björn Töpel" <bjorn.topel@gmail.com>]

On Mon, 8 Apr 2019 at 22:23, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 04/08/2019 07:05 PM, Toke Høiland-Jørgensen wrote:
> > From: Björn Töpel <bjorn.topel@intel.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@intel.com>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.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,
> >
>