On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon [off-list ref] wrote:

On 31 May 2019, at 2:42, Björn Töpel wrote:

quoted

From: Björn Töpel <redacted>

All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
command of ndo_bpf. The query code is fairly generic. This commit
refactors the query code up from the drivers to the netdev level.

The struct net_device has gained two new members: xdp_prog_hw and
xdp_flags. The former is the offloaded XDP program, if any, and the
latter tracks the flags that the supplied when attaching the XDP
program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.

The xdp_prog member, previously only used for SKB_MODE, is shared with
DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
mutually exclusive. To differentiate between the two modes, a new
internal flag is introduced as well.

I'm not entirely clear why this new flag is needed - GENERIC seems to
be an alias for SKB_MODE, so why just use SKB_MODE directly?

If the user does not explicitly specify a type (skb|drv|hw), then the
command should choose the correct type and then behave as if this type
was specified.

Yes, this is kind of hairy.

SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
SKB, DRV, HW, SKB+HW DRV+HW.

What complicates things further, is that SKB and DRV can be implicitly
(auto/no flags) or explicitly enabled (flags).

If a user doesn't pass any flags, the "best supported mode" should be
selected. If this "auto mode" is used, it should be seen as a third
mode. E.g.

ip link set dev eth0 xdp on -- OK
ip link set dev eth0 xdp off -- OK

ip link set dev eth0 xdp on -- OK # generic auto selected
ip link set dev eth0 xdpgeneric off -- NOK, bad flags

ip link set dev eth0 xdp on -- OK # drv auto selected
ip link set dev eth0 xdpdrv off -- NOK, bad flags

...and so on. The idea is that a user should use the same set of flags always.

The internal "GENERIC" flag is only to determine if the xdp_prog
represents a DRV version or SKB version. Maybe it would be clearer
just to add an additional xdp_prog_drv to the net_device, instead?

The logic in dev_change_xdp_fd() is too complicated.  It disallows
setting (drv|skb), but allows (hw|skb), which I'm not sure is
intentional.

It should be clearer as to which combinations are allowed.

Fair point. I'll try to clean it up further.


Björn

--
Jonathan

quoted

The program query operations is all done under the rtnl lock. However,
the xdp_prog member is __rcu annotated, and used in a lock-less manner
for the SKB_MODE. Now that xdp_prog member is shared from a query
program perspective, RCU read and assignments functions are still used
when altering xdp_prog, even when only for queries in DRV_MODE. This
is for sparse/lockdep correctness.

Signed-off-by: Björn Töpel <redacted>
---
 include/linux/netdevice.h |  15 +++--
 include/net/xdp.h         |   4 ++
 net/core/dev.c            | 138
++++++++++++++++++++++++--------------
 net/core/rtnetlink.c      |  53 +++++++--------
 net/core/xdp.c            |  17 +++--
 5 files changed, 139 insertions(+), 88 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 44b47e9df94a..f3a875a52c6c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -1940,6 +1940,9 @@ struct net_device {
 #endif
      struct hlist_node       index_hlist;

+     struct bpf_prog         *xdp_prog_hw;
+     u32                     xdp_flags;
+
 /*
  * Cache lines mostly used on transmit path
  */

@@ -2043,11 +2046,14 @@ struct net_device {
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)

+/* Reusing the XDP flags space for kernel internal flag */
+#define XDP_FLAGS_KERN_GENERIC (1U << 4)
+static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
+
 static inline bool netif_elide_gro(const struct net_device *dev)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
-             return true;
-     return false;
+     return !(dev->features & NETIF_F_GRO) ||
+             dev->xdp_flags & XDP_FLAGS_KERN_GENERIC;
 }

 #define      NETDEV_ALIGN            32

@@ -3684,8 +3690,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 mode);
 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/include/net/xdp.h b/include/net/xdp.h
index 0f25b3675c5c..3691280c8fc1 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h

@@ -51,6 +51,7 @@ struct xdp_mem_info {
 };

 struct page_pool;
+struct netlink_ext_ack;

 struct zero_copy_allocator {
      void (*free)(struct zero_copy_allocator *zca, unsigned long handle);

@@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct

xdp_attachment_info *info,
 void xdp_attachment_setup(struct xdp_attachment_info *info,
                        struct netdev_bpf *bpf);

+bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
+                    struct netlink_ext_ack *extack);
+
 #endif /* __LINUX_NET_XDP_H__ */

diff --git a/net/core/dev.c b/net/core/dev.c
index b6b8505cfb3e..1a9da508149a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -8005,21 +8005,43 @@ 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)
+static u32 dev_xdp_query_generic(struct net_device *dev)
 {
-     struct netdev_bpf xdp;
+     struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);

-     if (!bpf_op)
-             return 0;
+     return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
+             prog->aux->id : 0;
+}

-     memset(&xdp, 0, sizeof(xdp));
-     xdp.command = cmd;
+static u32 dev_xdp_query_drv(struct net_device *dev)
+{
+     struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
+
+     return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
+             prog->aux->id : 0;
+}
+
+static u32 dev_xdp_current_mode(struct net_device *dev)
+{
+     struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);

-     /* Query must always succeed. */
-     WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
+     if (prog)
+             return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE :
+                     XDP_FLAGS_DRV_MODE;

        return xdp->flags & XDP_FLAGS_MODE;

quoted

+     return 0;
+}

-     return xdp.prog_id;
+u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
+{
+     switch (mode) {
+     case XDP_FLAGS_DRV_MODE:
+             return dev_xdp_query_drv(dev);
+     case XDP_FLAGS_SKB_MODE:
+             return dev_xdp_query_generic(dev);
+     case XDP_FLAGS_HW_MODE:
+             return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id : 0;
+     }
+     return 0;
 }

 static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,

@@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device

*dev, bpf_op_t bpf_op,
                         struct bpf_prog *prog)
 {
      struct netdev_bpf xdp;
+     int err;

      memset(&xdp, 0, sizeof(xdp));
-     if (flags & XDP_FLAGS_HW_MODE)
+     if (flags & XDP_FLAGS_HW_MODE) {
              xdp.command = XDP_SETUP_PROG_HW;
-     else
+             xdp.flags = XDP_FLAGS_HW_MODE;
+     } else {
              xdp.command = XDP_SETUP_PROG;
+             xdp.flags = flags;
+     }
      xdp.extack = extack;
-     xdp.flags = flags;
      xdp.prog = prog;

-     return bpf_op(dev, &xdp);
+     err = bpf_op(dev, &xdp);
+     if (err)
+             return err;
+
+     if (flags & XDP_FLAGS_HW_MODE) {
+             dev->xdp_prog_hw = prog;
+             return 0;
+     }
+
+     rcu_assign_pointer(dev->xdp_prog, prog);
+     dev->xdp_flags = prog ? flags : 0;
+     return 0;
 }

 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 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));
+     struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
+     bpf_op_t bpf_op;

-     /* 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,
+     if (prog) {
+             bpf_op = dev_xdp_query_generic(dev) ?
+                      generic_xdp_install : dev->netdev_ops->ndo_bpf;
+             WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev->xdp_flags,
                                      NULL));
+     }
+     if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
+             WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
+                                     NULL, XDP_FLAGS_HW_MODE, NULL));
 }

 /**

@@ -8080,41 +8104,49 @@ static void dev_xdp_uninstall(struct

net_device *dev)
 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;
      struct bpf_prog *prog = NULL;
-     bpf_op_t bpf_op, bpf_chk;
+     u32 mode, curr_mode;
+     bpf_op_t bpf_op;
      bool offload;
      int err;

      ASSERT_RTNL();

      offload = flags & XDP_FLAGS_HW_MODE;
-     query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
+     mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;

-     bpf_op = bpf_chk = ops->ndo_bpf;
+     bpf_op = dev->netdev_ops->ndo_bpf;
      if (!bpf_op && (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 (fd >= 0) {
-             if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
+     if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
+             mode = XDP_FLAGS_SKB_MODE;
+
+     curr_mode = dev_xdp_current_mode(dev);
+
+     if (!offload && curr_mode && (mode ^ curr_mode) &
+         (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
+             if (fd >= 0) {
                      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)) {
+             return 0;
+     }
+
+     if (!offload && dev_xdp_query(dev, mode) &&
+         !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
+             return -EBUSY;
+
+     if (fd >= 0) {
+             if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
+                 dev_xdp_query(dev, mode)) {
                      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, !!bpf_op);
              if (IS_ERR(prog))
                      return PTR_ERR(prog);

@@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,

struct netlink_ext_ack *extack,
              }
      }

+     if (mode == XDP_FLAGS_SKB_MODE) {
+             bpf_op = generic_xdp_install;
+             flags |= XDP_FLAGS_KERN_GENERIC;
+     }
      err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
      if (err < 0 && prog)
              bpf_prog_put(prog);

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index adcc045952c2..5e396fd01d8b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c

@@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff

*skb, struct net_device *dev)
      return 0;
 }

-static u32 rtnl_xdp_prog_skb(struct net_device *dev)
+static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
 {
-     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;
-}
-
-static u32 rtnl_xdp_prog_drv(struct net_device *dev)
-{
-     return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
XDP_QUERY_PROG);
+     switch (tgt_mode) {
+     case XDP_ATTACHED_DRV:
+             return XDP_FLAGS_DRV_MODE;
+     case XDP_ATTACHED_SKB:
+             return XDP_FLAGS_SKB_MODE;
+     case XDP_ATTACHED_HW:
+             return XDP_FLAGS_HW_MODE;
+     }
+     return 0;
 }

-static u32 rtnl_xdp_prog_hw(struct net_device *dev)
+static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
 {
-     return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
-                            XDP_QUERY_PROG_HW);
+     switch (tgt_mode) {
+     case XDP_ATTACHED_DRV:
+             return IFLA_XDP_DRV_PROG_ID;
+     case XDP_ATTACHED_SKB:
+             return IFLA_XDP_SKB_PROG_ID;
+     case XDP_ATTACHED_HW:
+             return IFLA_XDP_HW_PROG_ID;
+     }
+     return 0;
 }

 static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device
*dev,
-                            u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
-                            u32 (*get_prog_id)(struct net_device *dev))
+                            u32 *prog_id, u8 *mode, u8 tgt_mode)
 {
      u32 curr_id;
      int err;

-     curr_id = get_prog_id(dev);
+     curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
      if (!curr_id)
              return 0;

      *prog_id = curr_id;
-     err = nla_put_u32(skb, attr, curr_id);
+     err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
      if (err)
              return err;

@@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb,

struct net_device *dev)

      prog_id = 0;
      mode = XDP_ATTACHED_NONE;
-     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
XDP_ATTACHED_SKB,
-                               IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
+     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
XDP_ATTACHED_SKB);
      if (err)
              goto err_cancel;
-     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
XDP_ATTACHED_DRV,
-                               IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
+     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
XDP_ATTACHED_DRV);
      if (err)
              goto err_cancel;
-     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
XDP_ATTACHED_HW,
-                               IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
+     err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
XDP_ATTACHED_HW);
      if (err)
              goto err_cancel;

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 4b2b194f4f1f..af92c04a58d8 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c

@@ -388,16 +388,23 @@ int xdp_attachment_query(struct

xdp_attachment_info *info,
 }
 EXPORT_SYMBOL_GPL(xdp_attachment_query);

-bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
-                          struct netdev_bpf *bpf)
+bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
+                    struct netlink_ext_ack *extack)
 {
-     if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
-             NL_SET_ERR_MSG(bpf->extack,
-                            "program loaded with different flags");
+     if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
+             NL_SET_ERR_MSG(extack, "program loaded with different flags");
              return false;
      }
      return true;
 }
+
+bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
+                          struct netdev_bpf *bpf)
+{
+     if (info->prog)
+             return xdp_prog_flags_ok(bpf->flags, info->flags, bpf->extack);
+     return true;
+}
 EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);

 void xdp_attachment_setup(struct xdp_attachment_info *info,
--
2.20.1