[PATCH v4 bpf-next 0/9] BPF XDP link

[PATCH v4 bpf-next 0/9] BPF XDP link

[Andrii Nakryiko]

Following cgroup and netns examples, implement bpf_link support for XDP.

The semantics is described in patch #2. Program and link attachments are
mutually exclusive, in the sense that neither link can replace attached
program nor program can replace attached link. Link can't replace attached
link as well, as is the case for any other bpf_link implementation.

Patch #1 refactors existing BPF program-based attachment API and centralizes
high-level query/attach decisions in generic kernel code, while drivers are
kept simple and are instructed with low-level decisions about attaching and
detaching specific bpf_prog. This also makes QUERY command unnecessary, and
patch #8 removes support for it from all kernel drivers. If that's a bad idea,
we can drop that patch altogether.

With refactoring in patch #1, adding bpf_xdp_link is completely transparent to
drivers, they are still functioning at the level of "effective" bpf_prog, that
should be called in XDP data path.

Corresponding libbpf support for BPF XDP link is added in patch #5.

v3->v4:
- fix a compilation warning in one of drivers (Jakub);

v2->v3:
- fix build when CONFIG_BPF_SYSCALL=n (kernel test robot);

v1->v2:
- fix prog refcounting bug (David);
- split dev_change_xdp_fd() changes into 2 patches (David);
- add extack messages to all user-induced errors (David).


Andrii Nakryiko (9):
  bpf: make bpf_link API available indepently of CONFIG_BPF_SYSCALL
  bpf, xdp: maintain info on attached XDP BPF programs in net_device
  bpf, xdp: extract common XDP program attachment logic
  bpf, xdp: add bpf_link-based XDP attachment API
  bpf, xdp: implement LINK_UPDATE for BPF XDP link
  bpf: implement BPF XDP link-specific introspection APIs
  libbpf: add support for BPF XDP link
  selftests/bpf: add BPF XDP link selftests
  bpf, xdp: remove XDP_QUERY_PROG and XDP_QUERY_PROG_HW XDP commands

 drivers/net/ethernet/amazon/ena/ena_netdev.c  |   6 -
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |   4 -
 .../net/ethernet/cavium/thunder/nicvf_main.c  |   3 -
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |   5 -
 drivers/net/ethernet/intel/i40e/i40e_main.c   |   3 -
 drivers/net/ethernet/intel/ice/ice_main.c     |   3 -
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   4 -
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |   6 -
 drivers/net/ethernet/marvell/mvneta.c         |   5 -
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |   3 -
 .../net/ethernet/mellanox/mlx4/en_netdev.c    |  24 -
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  18 -
 .../ethernet/netronome/nfp/nfp_net_common.c   |   4 -
 .../net/ethernet/qlogic/qede/qede_filter.c    |   3 -
 drivers/net/ethernet/sfc/efx.c                |   5 -
 drivers/net/ethernet/socionext/netsec.c       |   3 -
 drivers/net/ethernet/ti/cpsw_priv.c           |   3 -
 drivers/net/hyperv/netvsc_bpf.c               |  21 +-
 drivers/net/netdevsim/bpf.c                   |   4 -
 drivers/net/netdevsim/netdevsim.h             |   2 +-
 drivers/net/tun.c                             |  15 -
 drivers/net/veth.c                            |  15 -
 drivers/net/virtio_net.c                      |  17 -
 drivers/net/xen-netfront.c                    |  21 -
 include/linux/bpf.h                           |  81 ++-
 include/linux/netdevice.h                     |  29 +-
 include/net/xdp.h                             |   2 -
 include/uapi/linux/bpf.h                      |  10 +-
 kernel/bpf/syscall.c                          |   5 +
 net/core/dev.c                                | 530 +++++++++++++-----
 net/core/rtnetlink.c                          |   5 +-
 net/core/xdp.c                                |   9 -
 tools/include/uapi/linux/bpf.h                |  10 +-
 tools/lib/bpf/libbpf.c                        |   9 +-
 tools/lib/bpf/libbpf.h                        |   2 +
 tools/lib/bpf/libbpf.map                      |   1 +
 .../selftests/bpf/prog_tests/section_names.c  |   2 +-
 .../selftests/bpf/prog_tests/xdp_link.c       | 137 +++++
 .../selftests/bpf/progs/test_xdp_link.c       |  12 +
 39 files changed, 658 insertions(+), 383 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_link.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_link.c

-- 
2.24.1

[PATCH v4 bpf-next 1/9] bpf: make bpf_link API available indepently of CONFIG_BPF_SYSCALL

[Andrii Nakryiko]

Similarly to bpf_prog, make bpf_link and related generic API available
unconditionally to make it easier to have bpf_link support in various parts of
the kernel. Stub out init/prime/settle/cleanup and inc/put APIs.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 include/linux/bpf.h | 81 ++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 26 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bae557ff2da8..33bb21b1dfe5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -761,6 +761,32 @@ struct bpf_array_aux {
 	struct work_struct work;
 };
 
+struct bpf_link {
+	atomic64_t refcnt;
+	u32 id;
+	enum bpf_link_type type;
+	const struct bpf_link_ops *ops;
+	struct bpf_prog *prog;
+	struct work_struct work;
+};
+
+struct bpf_link_ops {
+	void (*release)(struct bpf_link *link);
+	void (*dealloc)(struct bpf_link *link);
+	int (*update_prog)(struct bpf_link *link, struct bpf_prog *new_prog,
+			   struct bpf_prog *old_prog);
+	void (*show_fdinfo)(const struct bpf_link *link, struct seq_file *seq);
+	int (*fill_link_info)(const struct bpf_link *link,
+			      struct bpf_link_info *info);
+};
+
+struct bpf_link_primer {
+	struct bpf_link *link;
+	struct file *file;
+	int fd;
+	u32 id;
+};
+
 struct bpf_struct_ops_value;
 struct btf_type;
 struct btf_member;
@@ -1143,32 +1169,6 @@ static inline bool bpf_bypass_spec_v4(void)
 int bpf_map_new_fd(struct bpf_map *map, int flags);
 int bpf_prog_new_fd(struct bpf_prog *prog);
 
-struct bpf_link {
-	atomic64_t refcnt;
-	u32 id;
-	enum bpf_link_type type;
-	const struct bpf_link_ops *ops;
-	struct bpf_prog *prog;
-	struct work_struct work;
-};
-
-struct bpf_link_primer {
-	struct bpf_link *link;
-	struct file *file;
-	int fd;
-	u32 id;
-};
-
-struct bpf_link_ops {
-	void (*release)(struct bpf_link *link);
-	void (*dealloc)(struct bpf_link *link);
-	int (*update_prog)(struct bpf_link *link, struct bpf_prog *new_prog,
-			   struct bpf_prog *old_prog);
-	void (*show_fdinfo)(const struct bpf_link *link, struct seq_file *seq);
-	int (*fill_link_info)(const struct bpf_link *link,
-			      struct bpf_link_info *info);
-};
-
 void bpf_link_init(struct bpf_link *link, enum bpf_link_type type,
 		   const struct bpf_link_ops *ops, struct bpf_prog *prog);
 int bpf_link_prime(struct bpf_link *link, struct bpf_link_primer *primer);
@@ -1370,6 +1370,35 @@ static inline void __bpf_prog_uncharge(struct user_struct *user, u32 pages)
 {
 }
 
+static inline void bpf_link_init(struct bpf_link *link, enum bpf_link_type type,
+				 const struct bpf_link_ops *ops,
+				 struct bpf_prog *prog)
+{
+}
+
+static inline int bpf_link_prime(struct bpf_link *link,
+				 struct bpf_link_primer *primer)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int bpf_link_settle(struct bpf_link_primer *primer)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void bpf_link_cleanup(struct bpf_link_primer *primer)
+{
+}
+
+static inline void bpf_link_inc(struct bpf_link *link)
+{
+}
+
+static inline void bpf_link_put(struct bpf_link *link)
+{
+}
+
 static inline int bpf_obj_get_user(const char __user *pathname, int flags)
 {
 	return -EOPNOTSUPP;
-- 
2.24.1

[PATCH v4 bpf-next 2/9] bpf, xdp: maintain info on attached XDP BPF programs in net_device

[Andrii Nakryiko]

Instead of delegating to drivers, maintain information about which BPF
programs are attached in which XDP modes (generic/skb, driver, or hardware)
locally in net_device. This effectively obsoletes XDP_QUERY_PROG command.

Such re-organization simplifies existing code already. But it also allows to
further add bpf_link-based XDP attachments without drivers having to know
about any of this at all, which seems like a good setup.
XDP_SETUP_PROG/XDP_SETUP_PROG_HW are just low-level commands to driver to
install/uninstall active BPF program. All the higher-level concerns about
prog/link interaction will be contained within generic driver-agnostic logic.

All the XDP_QUERY_PROG calls to driver in dev_xdp_uninstall() were removed.
It's not clear for me why dev_xdp_uninstall() were passing previous prog_flags
when resetting installed programs. That seems unnecessary, plus most drivers
don't populate prog_flags anyways. Having XDP_SETUP_PROG vs XDP_SETUP_PROG_HW
should be enough of an indicator of what is required of driver to correctly
reset active BPF program. dev_xdp_uninstall() is also generalized as an
iteration over all three supported mode.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 include/linux/netdevice.h |  17 +++-
 net/core/dev.c            | 158 +++++++++++++++++++++-----------------
 net/core/rtnetlink.c      |   5 +-
 3 files changed, 105 insertions(+), 75 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ac2cd3f49aba..cad44b40c776 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -889,6 +889,17 @@ struct netlink_ext_ack;
 struct xdp_umem;
 struct xdp_dev_bulk_queue;
 
+enum bpf_xdp_mode {
+	XDP_MODE_SKB = 0,
+	XDP_MODE_DRV = 1,
+	XDP_MODE_HW = 2,
+	__MAX_XDP_MODE
+};
+
+struct bpf_xdp_entity {
+	struct bpf_prog *prog;
+};
+
 struct netdev_bpf {
 	enum bpf_netdev_command command;
 	union {
@@ -2142,6 +2153,9 @@ struct net_device {
 #endif
 	const struct udp_tunnel_nic_info	*udp_tunnel_nic_info;
 	struct udp_tunnel_nic	*udp_tunnel_nic;
+
+	/* protected by rtnl_lock */
+	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
 };
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
@@ -3817,8 +3831,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, int expected_fd, u32 flags);
-u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
-		    enum bpf_netdev_command cmd);
+u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode 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/net/core/dev.c b/net/core/dev.c
index b820527f0a8d..7e753e248cef 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8716,84 +8716,103 @@ 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 enum bpf_xdp_mode dev_xdp_mode(u32 flags)
 {
-	struct netdev_bpf xdp;
+	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 (!bpf_op)
-		return 0;
+static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
+{
+	switch (mode) {
+	case XDP_MODE_SKB:
+		return generic_xdp_install;
+	case XDP_MODE_DRV:
+	case XDP_MODE_HW:
+		return dev->netdev_ops->ndo_bpf;
+	default:
+		return NULL;
+	};
+}
 
-	memset(&xdp, 0, sizeof(xdp));
-	xdp.command = cmd;
+static struct bpf_prog *dev_xdp_prog(struct net_device *dev,
+				     enum bpf_xdp_mode mode)
+{
+	return dev->xdp_state[mode].prog;
+}
+
+u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode)
+{
+	struct bpf_prog *prog = dev_xdp_prog(dev, mode);
 
-	/* Query must always succeed. */
-	WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
+	return prog ? prog->aux->id : 0;
+}
 
-	return xdp.prog_id;
+static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
+			     struct bpf_prog *prog)
+{
+	dev->xdp_state[mode].prog = prog;
 }
 
-static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
-			   struct netlink_ext_ack *extack, u32 flags,
-			   struct bpf_prog *prog)
+static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
+			   bpf_op_t bpf_op, struct netlink_ext_ack *extack,
+			   u32 flags, struct bpf_prog *prog)
 {
-	bool non_hw = !(flags & XDP_FLAGS_HW_MODE);
-	struct bpf_prog *prev_prog = NULL;
 	struct netdev_bpf xdp;
 	int err;
 
-	if (non_hw) {
-		prev_prog = bpf_prog_by_id(__dev_xdp_query(dev, bpf_op,
-							   XDP_QUERY_PROG));
-		if (IS_ERR(prev_prog))
-			prev_prog = NULL;
-	}
-
 	memset(&xdp, 0, sizeof(xdp));
-	if (flags & XDP_FLAGS_HW_MODE)
-		xdp.command = XDP_SETUP_PROG_HW;
-	else
-		xdp.command = XDP_SETUP_PROG;
+	xdp.command = mode == XDP_MODE_HW ? XDP_SETUP_PROG_HW : XDP_SETUP_PROG;
 	xdp.extack = extack;
 	xdp.flags = flags;
 	xdp.prog = prog;
 
+	/* Drivers assume refcnt is already incremented (i.e, prog pointer is
+	 * "moved" into driver), so they don't increment it on their own, but
+	 * they do decrement refcnt when program is detached or replaced.
+	 * Given net_device also owns link/prog, we need to bump refcnt here
+	 * to prevent drivers from underflowing it.
+	 */
+	if (prog)
+		bpf_prog_inc(prog);
 	err = bpf_op(dev, &xdp);
-	if (!err && non_hw)
-		bpf_prog_change_xdp(prev_prog, prog);
+	if (err) {
+		if (prog)
+			bpf_prog_put(prog);
+		return err;
+	}
 
-	if (prev_prog)
-		bpf_prog_put(prev_prog);
+	if (mode != XDP_MODE_HW)
+		bpf_prog_change_xdp(dev_xdp_prog(dev, mode), prog);
 
-	return err;
+	return 0;
 }
 
 static void dev_xdp_uninstall(struct net_device *dev)
 {
-	struct netdev_bpf xdp;
-	bpf_op_t ndo_bpf;
+	struct bpf_prog *prog;
+	enum bpf_xdp_mode mode;
+	bpf_op_t bpf_op;
 
-	/* Remove generic XDP */
-	WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
+	ASSERT_RTNL();
 
-	/* Remove from the driver */
-	ndo_bpf = dev->netdev_ops->ndo_bpf;
-	if (!ndo_bpf)
-		return;
+	for (mode = XDP_MODE_SKB; mode < __MAX_XDP_MODE; mode++) {
+		prog = dev_xdp_prog(dev, mode);
+		if (!prog)
+			continue;
 
-	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));
+		bpf_op = dev_xdp_bpf_op(dev, mode);
+		if (!bpf_op)
+			continue;
 
-	/* 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, mode, bpf_op, NULL, 0, NULL));
+
+		bpf_prog_put(prog);
+		dev_xdp_set_prog(dev, mode, NULL);
+	}
 }
 
 /**
@@ -8810,29 +8829,22 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		      int fd, int expected_fd, u32 flags)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
-	enum bpf_netdev_command query;
+	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
+	bool offload = mode == XDP_MODE_HW;
 	u32 prog_id, expected_id = 0;
-	bpf_op_t bpf_op, bpf_chk;
 	struct bpf_prog *prog;
-	bool offload;
+	bpf_op_t bpf_op;
 	int err;
 
 	ASSERT_RTNL();
 
-	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))) {
+	bpf_op = dev_xdp_bpf_op(dev, mode);
+	if (!bpf_op) {
 		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;
 
-	prog_id = __dev_xdp_query(dev, bpf_op, query);
+	prog_id = dev_xdp_prog_id(dev, mode);
 	if (flags & XDP_FLAGS_REPLACE) {
 		if (expected_fd >= 0) {
 			prog = bpf_prog_get_type_dev(expected_fd,
@@ -8850,8 +8862,11 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		}
 	}
 	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");
+		enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB
+					       ? XDP_MODE_DRV : XDP_MODE_SKB;
+
+		if (!offload && dev_xdp_prog_id(dev, other_mode)) {
+			NL_SET_ERR_MSG(extack, "Native and generic XDP can't be active at the same time");
 			return -EEXIST;
 		}
 
@@ -8866,7 +8881,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 			return PTR_ERR(prog);
 
 		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");
+			NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported");
 			bpf_prog_put(prog);
 			return -EINVAL;
 		}
@@ -8895,11 +8910,14 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		prog = NULL;
 	}
 
-	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
-	if (err < 0 && prog)
+	err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog);
+	if (err < 0 && prog) {
 		bpf_prog_put(prog);
+		return err;
+	}
+	dev_xdp_set_prog(dev, mode, prog);
 
-	return err;
+	return 0;
 }
 
 /**
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9aedc15736ad..754fdfafacb0 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1416,13 +1416,12 @@ static u32 rtnl_xdp_prog_skb(struct net_device *dev)
 
 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_prog_id(dev, XDP_MODE_DRV);
 }
 
 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_prog_id(dev, XDP_MODE_HW);
 }
 
 static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
-- 
2.24.1

Re: [PATCH v4 bpf-next 2/9] bpf, xdp: maintain info on attached XDP BPF programs in net_device

[Maciej Fijalkowski]

On Tue, Jul 21, 2020 at 11:45:55PM -0700, Andrii Nakryiko wrote:
> Instead of delegating to drivers, maintain information about which BPF
> programs are attached in which XDP modes (generic/skb, driver, or hardware)
> locally in net_device. This effectively obsoletes XDP_QUERY_PROG command.
> 
> Such re-organization simplifies existing code already. But it also allows to
> further add bpf_link-based XDP attachments without drivers having to know
> about any of this at all, which seems like a good setup.
> XDP_SETUP_PROG/XDP_SETUP_PROG_HW are just low-level commands to driver to
> install/uninstall active BPF program. All the higher-level concerns about
> prog/link interaction will be contained within generic driver-agnostic logic.
> 
> All the XDP_QUERY_PROG calls to driver in dev_xdp_uninstall() were removed.
> It's not clear for me why dev_xdp_uninstall() were passing previous prog_flags
> when resetting installed programs. That seems unnecessary, plus most drivers
> don't populate prog_flags anyways. Having XDP_SETUP_PROG vs XDP_SETUP_PROG_HW
> should be enough of an indicator of what is required of driver to correctly
> reset active BPF program. dev_xdp_uninstall() is also generalized as an
> iteration over all three supported mode.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  include/linux/netdevice.h |  17 +++-
>  net/core/dev.c            | 158 +++++++++++++++++++++-----------------
>  net/core/rtnetlink.c      |   5 +-
>  3 files changed, 105 insertions(+), 75 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ac2cd3f49aba..cad44b40c776 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -889,6 +889,17 @@ struct netlink_ext_ack;
>  struct xdp_umem;
>  struct xdp_dev_bulk_queue;
>  
> +enum bpf_xdp_mode {
> +	XDP_MODE_SKB = 0,
> +	XDP_MODE_DRV = 1,
> +	XDP_MODE_HW = 2,
> +	__MAX_XDP_MODE
> +};
> +
> +struct bpf_xdp_entity {
> +	struct bpf_prog *prog;
> +};
> +
>  struct netdev_bpf {
>  	enum bpf_netdev_command command;
>  	union {
> @@ -2142,6 +2153,9 @@ struct net_device {
>  #endif
>  	const struct udp_tunnel_nic_info	*udp_tunnel_nic_info;
>  	struct udp_tunnel_nic	*udp_tunnel_nic;
> +
> +	/* protected by rtnl_lock */
> +	struct bpf_xdp_entity	xdp_state[__MAX_XDP_MODE];
>  };
>  #define to_net_dev(d) container_of(d, struct net_device, dev)
>  
> @@ -3817,8 +3831,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, int expected_fd, u32 flags);
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> -		    enum bpf_netdev_command cmd);
> +u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode 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/net/core/dev.c b/net/core/dev.c
> index b820527f0a8d..7e753e248cef 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8716,84 +8716,103 @@ 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 enum bpf_xdp_mode dev_xdp_mode(u32 flags)
>  {
> -	struct netdev_bpf xdp;
> +	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 (!bpf_op)
> -		return 0;
> +static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
> +{
> +	switch (mode) {
> +	case XDP_MODE_SKB:
> +		return generic_xdp_install;
> +	case XDP_MODE_DRV:
> +	case XDP_MODE_HW:
> +		return dev->netdev_ops->ndo_bpf;
> +	default:
> +		return NULL;
> +	};
> +}
>  
> -	memset(&xdp, 0, sizeof(xdp));
> -	xdp.command = cmd;
> +static struct bpf_prog *dev_xdp_prog(struct net_device *dev,
> +				     enum bpf_xdp_mode mode)
> +{
> +	return dev->xdp_state[mode].prog;
> +}
> +
> +u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode)
> +{
> +	struct bpf_prog *prog = dev_xdp_prog(dev, mode);
>  
> -	/* Query must always succeed. */
> -	WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> +	return prog ? prog->aux->id : 0;
> +}
>  
> -	return xdp.prog_id;
> +static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
> +			     struct bpf_prog *prog)
> +{
> +	dev->xdp_state[mode].prog = prog;
>  }
>  
> -static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> -			   struct netlink_ext_ack *extack, u32 flags,
> -			   struct bpf_prog *prog)
> +static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
> +			   bpf_op_t bpf_op, struct netlink_ext_ack *extack,
> +			   u32 flags, struct bpf_prog *prog)
>  {
> -	bool non_hw = !(flags & XDP_FLAGS_HW_MODE);
> -	struct bpf_prog *prev_prog = NULL;
>  	struct netdev_bpf xdp;
>  	int err;
>  
> -	if (non_hw) {
> -		prev_prog = bpf_prog_by_id(__dev_xdp_query(dev, bpf_op,
> -							   XDP_QUERY_PROG));
> -		if (IS_ERR(prev_prog))
> -			prev_prog = NULL;
> -	}
> -
>  	memset(&xdp, 0, sizeof(xdp));
> -	if (flags & XDP_FLAGS_HW_MODE)
> -		xdp.command = XDP_SETUP_PROG_HW;
> -	else
> -		xdp.command = XDP_SETUP_PROG;
> +	xdp.command = mode == XDP_MODE_HW ? XDP_SETUP_PROG_HW : XDP_SETUP_PROG;
>  	xdp.extack = extack;
>  	xdp.flags = flags;
>  	xdp.prog = prog;
>  
> +	/* Drivers assume refcnt is already incremented (i.e, prog pointer is
> +	 * "moved" into driver), so they don't increment it on their own, but
> +	 * they do decrement refcnt when program is detached or replaced.
> +	 * Given net_device also owns link/prog, we need to bump refcnt here
> +	 * to prevent drivers from underflowing it.
> +	 */
> +	if (prog)
> +		bpf_prog_inc(prog);
>  	err = bpf_op(dev, &xdp);
> -	if (!err && non_hw)
> -		bpf_prog_change_xdp(prev_prog, prog);
> +	if (err) {
> +		if (prog)
> +			bpf_prog_put(prog);
> +		return err;
> +	}
>  
> -	if (prev_prog)
> -		bpf_prog_put(prev_prog);
> +	if (mode != XDP_MODE_HW)
> +		bpf_prog_change_xdp(dev_xdp_prog(dev, mode), prog);
>  
> -	return err;
> +	return 0;
>  }
>  
>  static void dev_xdp_uninstall(struct net_device *dev)
>  {
> -	struct netdev_bpf xdp;
> -	bpf_op_t ndo_bpf;
> +	struct bpf_prog *prog;
> +	enum bpf_xdp_mode mode;
> +	bpf_op_t bpf_op;
>  
> -	/* Remove generic XDP */
> -	WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> +	ASSERT_RTNL();
>  
> -	/* Remove from the driver */
> -	ndo_bpf = dev->netdev_ops->ndo_bpf;
> -	if (!ndo_bpf)
> -		return;
> +	for (mode = XDP_MODE_SKB; mode < __MAX_XDP_MODE; mode++) {
> +		prog = dev_xdp_prog(dev, mode);
> +		if (!prog)
> +			continue;
>  
> -	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));
> +		bpf_op = dev_xdp_bpf_op(dev, mode);
> +		if (!bpf_op)
> +			continue;

could we assume that we are iterating over the defined XDP modes so bpf_op
will always be a valid function pointer so that we could use directly
dev_xdp_bpf_op() in dev_xdp_install() ?

Just a nit, however current state is probably less error-prone when in
future we might be introducing new XDP mode.

>  
> -	/* 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, mode, bpf_op, NULL, 0, NULL));
> +
> +		bpf_prog_put(prog);
> +		dev_xdp_set_prog(dev, mode, NULL);
> +	}
>  }
>  
>  /**
> @@ -8810,29 +8829,22 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		      int fd, int expected_fd, u32 flags)
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;
> -	enum bpf_netdev_command query;
> +	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> +	bool offload = mode == XDP_MODE_HW;
>  	u32 prog_id, expected_id = 0;
> -	bpf_op_t bpf_op, bpf_chk;
>  	struct bpf_prog *prog;
> -	bool offload;
> +	bpf_op_t bpf_op;
>  	int err;
>  
>  	ASSERT_RTNL();
>  
> -	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))) {
> +	bpf_op = dev_xdp_bpf_op(dev, mode);
> +	if (!bpf_op) {
>  		NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
>  		return -EOPNOTSUPP;
>  	}

Seems that we won't ever hit this case with this patch?
If flags are not drv/hw, then dev_xdp_mode() will always spit out the
XDP_MODE_SKB which then passed to dev_xdp_bpf_op() will in turn give the
generic_xdp_install().

I think this check was against the situation where user wanted to go with
native mode but underlying HW was not supporting it, right?

So right now we will always be silently going with generic XDP?

I haven't followed previous revisions so I might be missing something.

> -	if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> -		bpf_op = generic_xdp_install;
> -	if (bpf_op == bpf_chk)
> -		bpf_chk = generic_xdp_install;
>  
> -	prog_id = __dev_xdp_query(dev, bpf_op, query);
> +	prog_id = dev_xdp_prog_id(dev, mode);
>  	if (flags & XDP_FLAGS_REPLACE) {
>  		if (expected_fd >= 0) {
>  			prog = bpf_prog_get_type_dev(expected_fd,
> @@ -8850,8 +8862,11 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		}
>  	}
>  	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");
> +		enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB
> +					       ? XDP_MODE_DRV : XDP_MODE_SKB;
> +
> +		if (!offload && dev_xdp_prog_id(dev, other_mode)) {
> +			NL_SET_ERR_MSG(extack, "Native and generic XDP can't be active at the same time");
>  			return -EEXIST;
>  		}
>  
> @@ -8866,7 +8881,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  			return PTR_ERR(prog);
>  
>  		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");
> +			NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported");
>  			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> @@ -8895,11 +8910,14 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
>  		prog = NULL;
>  	}
>  
> -	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> -	if (err < 0 && prog)
> +	err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog);
> +	if (err < 0 && prog) {
>  		bpf_prog_put(prog);
> +		return err;
> +	}
> +	dev_xdp_set_prog(dev, mode, prog);
>  
> -	return err;
> +	return 0;
>  }
>  
>  /**
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 9aedc15736ad..754fdfafacb0 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1416,13 +1416,12 @@ static u32 rtnl_xdp_prog_skb(struct net_device *dev)
>  
>  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_prog_id(dev, XDP_MODE_DRV);
>  }
>  
>  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_prog_id(dev, XDP_MODE_HW);
>  }
>  
>  static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
> -- 
> 2.24.1
> 

Re: [PATCH v4 bpf-next 2/9] bpf, xdp: maintain info on attached XDP BPF programs in net_device

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 8:31 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Tue, Jul 21, 2020 at 11:45:55PM -0700, Andrii Nakryiko wrote:
> > Instead of delegating to drivers, maintain information about which BPF
> > programs are attached in which XDP modes (generic/skb, driver, or hardware)
> > locally in net_device. This effectively obsoletes XDP_QUERY_PROG command.
> >
> > Such re-organization simplifies existing code already. But it also allows to
> > further add bpf_link-based XDP attachments without drivers having to know
> > about any of this at all, which seems like a good setup.
> > XDP_SETUP_PROG/XDP_SETUP_PROG_HW are just low-level commands to driver to
> > install/uninstall active BPF program. All the higher-level concerns about
> > prog/link interaction will be contained within generic driver-agnostic logic.
> >
> > All the XDP_QUERY_PROG calls to driver in dev_xdp_uninstall() were removed.
> > It's not clear for me why dev_xdp_uninstall() were passing previous prog_flags
> > when resetting installed programs. That seems unnecessary, plus most drivers
> > don't populate prog_flags anyways. Having XDP_SETUP_PROG vs XDP_SETUP_PROG_HW
> > should be enough of an indicator of what is required of driver to correctly
> > reset active BPF program. dev_xdp_uninstall() is also generalized as an
> > iteration over all three supported mode.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  include/linux/netdevice.h |  17 +++-
> >  net/core/dev.c            | 158 +++++++++++++++++++++-----------------
> >  net/core/rtnetlink.c      |   5 +-
> >  3 files changed, 105 insertions(+), 75 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index ac2cd3f49aba..cad44b40c776 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -889,6 +889,17 @@ struct netlink_ext_ack;
> >  struct xdp_umem;
> >  struct xdp_dev_bulk_queue;
> >
> > +enum bpf_xdp_mode {
> > +     XDP_MODE_SKB = 0,
> > +     XDP_MODE_DRV = 1,
> > +     XDP_MODE_HW = 2,
> > +     __MAX_XDP_MODE
> > +};
> > +
> > +struct bpf_xdp_entity {
> > +     struct bpf_prog *prog;
> > +};
> > +

[...]

> >
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> > -                 enum bpf_netdev_command cmd)
> > +static enum bpf_xdp_mode dev_xdp_mode(u32 flags)
> >  {
> > -     struct netdev_bpf xdp;
> > +     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 (!bpf_op)
> > -             return 0;
> > +static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
> > +{
> > +     switch (mode) {
> > +     case XDP_MODE_SKB:
> > +             return generic_xdp_install;
> > +     case XDP_MODE_DRV:
> > +     case XDP_MODE_HW:
> > +             return dev->netdev_ops->ndo_bpf;
> > +     default:
> > +             return NULL;
> > +     };
> > +}
> >

[...]

> >
> >  static void dev_xdp_uninstall(struct net_device *dev)
> >  {
> > -     struct netdev_bpf xdp;
> > -     bpf_op_t ndo_bpf;
> > +     struct bpf_prog *prog;
> > +     enum bpf_xdp_mode mode;
> > +     bpf_op_t bpf_op;
> >
> > -     /* Remove generic XDP */
> > -     WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> > +     ASSERT_RTNL();
> >
> > -     /* Remove from the driver */
> > -     ndo_bpf = dev->netdev_ops->ndo_bpf;
> > -     if (!ndo_bpf)
> > -             return;
> > +     for (mode = XDP_MODE_SKB; mode < __MAX_XDP_MODE; mode++) {
> > +             prog = dev_xdp_prog(dev, mode);
> > +             if (!prog)
> > +                     continue;
> >
> > -     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));
> > +             bpf_op = dev_xdp_bpf_op(dev, mode);
> > +             if (!bpf_op)
> > +                     continue;
>
> could we assume that we are iterating over the defined XDP modes so bpf_op
> will always be a valid function pointer so that we could use directly
> dev_xdp_bpf_op() in dev_xdp_install() ?
>
> Just a nit, however current state is probably less error-prone when in
> future we might be introducing new XDP mode.

No we can't because for DRV and HW modes, dev->netdev_ops->ndo_bpf can
be NULL. So even though there are 3 possible XDP modes, only one (SKB,
using generic_xdp_install handler) might be supported for a given
net_device.

>
> >
> > -     /* 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, mode, bpf_op, NULL, 0, NULL));
> > +
> > +             bpf_prog_put(prog);
> > +             dev_xdp_set_prog(dev, mode, NULL);
> > +     }
> >  }
> >
> >  /**
> > @@ -8810,29 +8829,22 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> >                     int fd, int expected_fd, u32 flags)
> >  {
> >       const struct net_device_ops *ops = dev->netdev_ops;
> > -     enum bpf_netdev_command query;
> > +     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > +     bool offload = mode == XDP_MODE_HW;
> >       u32 prog_id, expected_id = 0;
> > -     bpf_op_t bpf_op, bpf_chk;
> >       struct bpf_prog *prog;
> > -     bool offload;
> > +     bpf_op_t bpf_op;
> >       int err;
> >
> >       ASSERT_RTNL();
> >
> > -     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))) {
> > +     bpf_op = dev_xdp_bpf_op(dev, mode);
> > +     if (!bpf_op) {
> >               NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> >               return -EOPNOTSUPP;
> >       }
>
> Seems that we won't ever hit this case with this patch?
> If flags are not drv/hw, then dev_xdp_mode() will always spit out the
> XDP_MODE_SKB which then passed to dev_xdp_bpf_op() will in turn give the
> generic_xdp_install().
>

But flags can be drv/hw, in which case dev_xdp_bpf_op() will return
dev->netdev_ops->ndo_bpf, which is NULL, so at that time we should
emit error that driver doesn't support native mode.

> I think this check was against the situation where user wanted to go with
> native mode but underlying HW was not supporting it, right?

right, and it's still the case.

>
> So right now we will always be silently going with generic XDP?

No, please check dev_xdp_bpf_op() implementation again, it returns
generic_xdp_install for SKB mode only.

>
> I haven't followed previous revisions so I might be missing something.
>
> > -     if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> > -             bpf_op = generic_xdp_install;
> > -     if (bpf_op == bpf_chk)
> > -             bpf_chk = generic_xdp_install;
> >
> > -     prog_id = __dev_xdp_query(dev, bpf_op, query);
> > +     prog_id = dev_xdp_prog_id(dev, mode);
> >       if (flags & XDP_FLAGS_REPLACE) {
> >               if (expected_fd >= 0) {
> >                       prog = bpf_prog_get_type_dev(expected_fd,

[...]

Re: [PATCH v4 bpf-next 2/9] bpf, xdp: maintain info on attached XDP BPF programs in net_device

[Maciej Fijalkowski]

On Wed, Jul 22, 2020 at 12:35:36PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 22, 2020 at 8:31 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > On Tue, Jul 21, 2020 at 11:45:55PM -0700, Andrii Nakryiko wrote:
> > > Instead of delegating to drivers, maintain information about which BPF
> > > programs are attached in which XDP modes (generic/skb, driver, or hardware)
> > > locally in net_device. This effectively obsoletes XDP_QUERY_PROG command.
> > >
> > > Such re-organization simplifies existing code already. But it also allows to
> > > further add bpf_link-based XDP attachments without drivers having to know
> > > about any of this at all, which seems like a good setup.
> > > XDP_SETUP_PROG/XDP_SETUP_PROG_HW are just low-level commands to driver to
> > > install/uninstall active BPF program. All the higher-level concerns about
> > > prog/link interaction will be contained within generic driver-agnostic logic.
> > >
> > > All the XDP_QUERY_PROG calls to driver in dev_xdp_uninstall() were removed.
> > > It's not clear for me why dev_xdp_uninstall() were passing previous prog_flags
> > > when resetting installed programs. That seems unnecessary, plus most drivers
> > > don't populate prog_flags anyways. Having XDP_SETUP_PROG vs XDP_SETUP_PROG_HW
> > > should be enough of an indicator of what is required of driver to correctly
> > > reset active BPF program. dev_xdp_uninstall() is also generalized as an
> > > iteration over all three supported mode.
> > >
> > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > ---
> > >  include/linux/netdevice.h |  17 +++-
> > >  net/core/dev.c            | 158 +++++++++++++++++++++-----------------
> > >  net/core/rtnetlink.c      |   5 +-
> > >  3 files changed, 105 insertions(+), 75 deletions(-)
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index ac2cd3f49aba..cad44b40c776 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -889,6 +889,17 @@ struct netlink_ext_ack;
> > >  struct xdp_umem;
> > >  struct xdp_dev_bulk_queue;
> > >
> > > +enum bpf_xdp_mode {
> > > +     XDP_MODE_SKB = 0,
> > > +     XDP_MODE_DRV = 1,
> > > +     XDP_MODE_HW = 2,
> > > +     __MAX_XDP_MODE
> > > +};
> > > +
> > > +struct bpf_xdp_entity {
> > > +     struct bpf_prog *prog;
> > > +};
> > > +
> 
> [...]
> 
> > >
> > > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> > > -                 enum bpf_netdev_command cmd)
> > > +static enum bpf_xdp_mode dev_xdp_mode(u32 flags)
> > >  {
> > > -     struct netdev_bpf xdp;
> > > +     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 (!bpf_op)
> > > -             return 0;
> > > +static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
> > > +{
> > > +     switch (mode) {
> > > +     case XDP_MODE_SKB:
> > > +             return generic_xdp_install;
> > > +     case XDP_MODE_DRV:
> > > +     case XDP_MODE_HW:
> > > +             return dev->netdev_ops->ndo_bpf;
> > > +     default:
> > > +             return NULL;
> > > +     };
> > > +}
> > >
> 
> [...]
> 
> > >
> > >  static void dev_xdp_uninstall(struct net_device *dev)
> > >  {
> > > -     struct netdev_bpf xdp;
> > > -     bpf_op_t ndo_bpf;
> > > +     struct bpf_prog *prog;
> > > +     enum bpf_xdp_mode mode;
> > > +     bpf_op_t bpf_op;
> > >
> > > -     /* Remove generic XDP */
> > > -     WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> > > +     ASSERT_RTNL();
> > >
> > > -     /* Remove from the driver */
> > > -     ndo_bpf = dev->netdev_ops->ndo_bpf;
> > > -     if (!ndo_bpf)
> > > -             return;
> > > +     for (mode = XDP_MODE_SKB; mode < __MAX_XDP_MODE; mode++) {
> > > +             prog = dev_xdp_prog(dev, mode);
> > > +             if (!prog)
> > > +                     continue;
> > >
> > > -     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));
> > > +             bpf_op = dev_xdp_bpf_op(dev, mode);
> > > +             if (!bpf_op)
> > > +                     continue;
> >
> > could we assume that we are iterating over the defined XDP modes so bpf_op
> > will always be a valid function pointer so that we could use directly
> > dev_xdp_bpf_op() in dev_xdp_install() ?
> >
> > Just a nit, however current state is probably less error-prone when in
> > future we might be introducing new XDP mode.
> 
> No we can't because for DRV and HW modes, dev->netdev_ops->ndo_bpf can
> be NULL. So even though there are 3 possible XDP modes, only one (SKB,
> using generic_xdp_install handler) might be supported for a given
> net_device.

Thanks, I missed the fact about ndo_bpf being NULL. Comments below can be
ignored.

> 
> >
> > >
> > > -     /* 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, mode, bpf_op, NULL, 0, NULL));
> > > +
> > > +             bpf_prog_put(prog);
> > > +             dev_xdp_set_prog(dev, mode, NULL);
> > > +     }
> > >  }
> > >
> > >  /**
> > > @@ -8810,29 +8829,22 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > >                     int fd, int expected_fd, u32 flags)
> > >  {
> > >       const struct net_device_ops *ops = dev->netdev_ops;
> > > -     enum bpf_netdev_command query;
> > > +     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > > +     bool offload = mode == XDP_MODE_HW;
> > >       u32 prog_id, expected_id = 0;
> > > -     bpf_op_t bpf_op, bpf_chk;
> > >       struct bpf_prog *prog;
> > > -     bool offload;
> > > +     bpf_op_t bpf_op;
> > >       int err;
> > >
> > >       ASSERT_RTNL();
> > >
> > > -     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))) {
> > > +     bpf_op = dev_xdp_bpf_op(dev, mode);
> > > +     if (!bpf_op) {
> > >               NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> > >               return -EOPNOTSUPP;
> > >       }
> >
> > Seems that we won't ever hit this case with this patch?
> > If flags are not drv/hw, then dev_xdp_mode() will always spit out the
> > XDP_MODE_SKB which then passed to dev_xdp_bpf_op() will in turn give the
> > generic_xdp_install().
> >
> 
> But flags can be drv/hw, in which case dev_xdp_bpf_op() will return
> dev->netdev_ops->ndo_bpf, which is NULL, so at that time we should
> emit error that driver doesn't support native mode.
> 
> > I think this check was against the situation where user wanted to go with
> > native mode but underlying HW was not supporting it, right?
> 
> right, and it's still the case.
> 
> >
> > So right now we will always be silently going with generic XDP?
> 
> No, please check dev_xdp_bpf_op() implementation again, it returns
> generic_xdp_install for SKB mode only.
> 
> >
> > I haven't followed previous revisions so I might be missing something.
> >
> > > -     if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE))
> > > -             bpf_op = generic_xdp_install;
> > > -     if (bpf_op == bpf_chk)
> > > -             bpf_chk = generic_xdp_install;
> > >
> > > -     prog_id = __dev_xdp_query(dev, bpf_op, query);
> > > +     prog_id = dev_xdp_prog_id(dev, mode);
> > >       if (flags & XDP_FLAGS_REPLACE) {
> > >               if (expected_fd >= 0) {
> > >                       prog = bpf_prog_get_type_dev(expected_fd,
> 
> [...]

[PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[Andrii Nakryiko]

Further refactor XDP attachment code. dev_change_xdp_fd() is split into two
parts: getting bpf_progs from FDs and attachment logic, working with
bpf_progs. This makes attachment  logic a bit more straightforward and
prepares code for bpf_xdp_link inclusion, which will share the common logic.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 net/core/dev.c | 165 +++++++++++++++++++++++++++----------------------
 1 file changed, 91 insertions(+), 74 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 7e753e248cef..abf573b2dcf4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct net_device *dev)
 	}
 }
 
-/**
- *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
- *	@dev: device
- *	@extack: netlink extended ack
- *	@fd: new program fd or negative value to clear
- *	@expected_fd: old program fd that userspace expects to replace or clear
- *	@flags: xdp-related flags
- *
- *	Set or clear a bpf program for a device
- */
-int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
-		      int fd, int expected_fd, u32 flags)
+static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
+			  struct bpf_prog *new_prog, struct bpf_prog *old_prog,
+			  u32 flags)
 {
-	const struct net_device_ops *ops = dev->netdev_ops;
-	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
-	bool offload = mode == XDP_MODE_HW;
-	u32 prog_id, expected_id = 0;
-	struct bpf_prog *prog;
+	struct bpf_prog *cur_prog;
+	enum bpf_xdp_mode mode;
 	bpf_op_t bpf_op;
 	int err;
 
 	ASSERT_RTNL();
 
-	bpf_op = dev_xdp_bpf_op(dev, mode);
-	if (!bpf_op) {
-		NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
-		return -EOPNOTSUPP;
+	/* just one XDP mode bit should be set, zero defaults to SKB mode */
+	if (hweight32(flags & XDP_FLAGS_MODES) > 1) {
+		NL_SET_ERR_MSG(extack, "Only one XDP mode flag can be set");
+		return -EINVAL;
+	}
+	/* old_prog != NULL implies XDP_FLAGS_REPLACE is set */
+	if (old_prog && !(flags & XDP_FLAGS_REPLACE)) {
+		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified");
+		return -EINVAL;
 	}
 
-	prog_id = dev_xdp_prog_id(dev, mode);
-	if (flags & XDP_FLAGS_REPLACE) {
-		if (expected_fd >= 0) {
-			prog = bpf_prog_get_type_dev(expected_fd,
-						     BPF_PROG_TYPE_XDP,
-						     bpf_op == ops->ndo_bpf);
-			if (IS_ERR(prog))
-				return PTR_ERR(prog);
-			expected_id = prog->aux->id;
-			bpf_prog_put(prog);
-		}
-
-		if (prog_id != expected_id) {
-			NL_SET_ERR_MSG(extack, "Active program does not match expected");
-			return -EEXIST;
-		}
+	mode = dev_xdp_mode(flags);
+	cur_prog = dev_xdp_prog(dev, mode);
+	if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) {
+		NL_SET_ERR_MSG(extack, "Active program does not match expected");
+		return -EEXIST;
 	}
-	if (fd >= 0) {
+	if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) {
+		NL_SET_ERR_MSG(extack, "XDP program already attached");
+		return -EBUSY;
+	}
+
+	if (new_prog) {
+		bool offload = mode == XDP_MODE_HW;
 		enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB
 					       ? XDP_MODE_DRV : XDP_MODE_SKB;
 
-		if (!offload && dev_xdp_prog_id(dev, other_mode)) {
+		if (!offload && dev_xdp_prog(dev, other_mode)) {
 			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) && prog_id) {
-			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);
-		if (IS_ERR(prog))
-			return PTR_ERR(prog);
-
-		if (!offload && bpf_prog_is_dev_bound(prog->aux)) {
+		if (!offload && bpf_prog_is_dev_bound(new_prog->aux)) {
 			NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported");
-			bpf_prog_put(prog);
 			return -EINVAL;
 		}
-
-		if (prog->expected_attach_type == BPF_XDP_DEVMAP) {
+		if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) {
 			NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device");
-			bpf_prog_put(prog);
 			return -EINVAL;
 		}
-
-		if (prog->expected_attach_type == BPF_XDP_CPUMAP) {
-			NL_SET_ERR_MSG(extack,
-				       "BPF_XDP_CPUMAP programs can not be attached to a device");
-			bpf_prog_put(prog);
+		if (new_prog->expected_attach_type == BPF_XDP_CPUMAP) {
+			NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");
 			return -EINVAL;
 		}
+	}
 
-		/* prog->aux->id may be 0 for orphaned device-bound progs */
-		if (prog->aux->id && prog->aux->id == prog_id) {
-			bpf_prog_put(prog);
-			return 0;
+	/* don't call drivers if the effective program didn't change */
+	if (new_prog != cur_prog) {
+		bpf_op = dev_xdp_bpf_op(dev, mode);
+		if (!bpf_op) {
+			NL_SET_ERR_MSG(extack, "Underlying driver does not support XDP in native mode");
+			return -EOPNOTSUPP;
 		}
-	} else {
-		if (!prog_id)
-			return 0;
-		prog = NULL;
-	}
 
-	err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog);
-	if (err < 0 && prog) {
-		bpf_prog_put(prog);
-		return err;
+		err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog);
+		if (err)
+			return err;
 	}
-	dev_xdp_set_prog(dev, mode, prog);
+
+	dev_xdp_set_prog(dev, mode, new_prog);
+	if (cur_prog)
+		bpf_prog_put(cur_prog);
 
 	return 0;
 }
 
+/**
+ *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
+ *	@dev: device
+ *	@extack: netlink extended ack
+ *	@fd: new program fd or negative value to clear
+ *	@expected_fd: old program fd that userspace expects to replace or clear
+ *	@flags: xdp-related flags
+ *
+ *	Set or clear a bpf program for a device
+ */
+int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
+		      int fd, int expected_fd, u32 flags)
+{
+	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
+	struct bpf_prog *new_prog = NULL, *old_prog = NULL;
+	int err;
+
+	ASSERT_RTNL();
+
+	if (fd >= 0) {
+		new_prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
+						 mode != XDP_MODE_SKB);
+		if (IS_ERR(new_prog))
+			return PTR_ERR(new_prog);
+	}
+
+	if (expected_fd >= 0) {
+		old_prog = bpf_prog_get_type_dev(expected_fd, BPF_PROG_TYPE_XDP,
+						 mode != XDP_MODE_SKB);
+		if (IS_ERR(old_prog)) {
+			err = PTR_ERR(old_prog);
+			old_prog = NULL;
+			goto err_out;
+		}
+	}
+
+	err = dev_xdp_attach(dev, extack, new_prog, old_prog, flags);
+
+err_out:
+	if (err && new_prog)
+		bpf_prog_put(new_prog);
+	if (old_prog)
+		bpf_prog_put(old_prog);
+	return err;
+}
+
 /**
  *	dev_new_index	-	allocate an ifindex
  *	@net: the applicable net namespace
-- 
2.24.1

Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[Maciej Fijalkowski]

On Tue, Jul 21, 2020 at 11:45:56PM -0700, Andrii Nakryiko wrote:
> Further refactor XDP attachment code. dev_change_xdp_fd() is split into two
> parts: getting bpf_progs from FDs and attachment logic, working with
> bpf_progs. This makes attachment  logic a bit more straightforward and
> prepares code for bpf_xdp_link inclusion, which will share the common logic.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  net/core/dev.c | 165 +++++++++++++++++++++++++++----------------------
>  1 file changed, 91 insertions(+), 74 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7e753e248cef..abf573b2dcf4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct net_device *dev)
>  	}
>  }
>  
> -/**
> - *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
> - *	@dev: device
> - *	@extack: netlink extended ack
> - *	@fd: new program fd or negative value to clear
> - *	@expected_fd: old program fd that userspace expects to replace or clear
> - *	@flags: xdp-related flags
> - *
> - *	Set or clear a bpf program for a device
> - */
> -int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> -		      int fd, int expected_fd, u32 flags)
> +static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
> +			  struct bpf_prog *new_prog, struct bpf_prog *old_prog,
> +			  u32 flags)
>  {
> -	const struct net_device_ops *ops = dev->netdev_ops;
> -	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> -	bool offload = mode == XDP_MODE_HW;
> -	u32 prog_id, expected_id = 0;
> -	struct bpf_prog *prog;
> +	struct bpf_prog *cur_prog;
> +	enum bpf_xdp_mode mode;
>  	bpf_op_t bpf_op;
>  	int err;
>  
>  	ASSERT_RTNL();

couldn't we rely on caller's rtnl assertion? dev_change_xdp_fd() already
has one.

>  
> -	bpf_op = dev_xdp_bpf_op(dev, mode);
> -	if (!bpf_op) {
> -		NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode");
> -		return -EOPNOTSUPP;
> +	/* just one XDP mode bit should be set, zero defaults to SKB mode */
> +	if (hweight32(flags & XDP_FLAGS_MODES) > 1) {
> +		NL_SET_ERR_MSG(extack, "Only one XDP mode flag can be set");
> +		return -EINVAL;
> +	}
> +	/* old_prog != NULL implies XDP_FLAGS_REPLACE is set */
> +	if (old_prog && !(flags & XDP_FLAGS_REPLACE)) {
> +		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not specified");
> +		return -EINVAL;
>  	}
>  
> -	prog_id = dev_xdp_prog_id(dev, mode);
> -	if (flags & XDP_FLAGS_REPLACE) {
> -		if (expected_fd >= 0) {
> -			prog = bpf_prog_get_type_dev(expected_fd,
> -						     BPF_PROG_TYPE_XDP,
> -						     bpf_op == ops->ndo_bpf);
> -			if (IS_ERR(prog))
> -				return PTR_ERR(prog);
> -			expected_id = prog->aux->id;
> -			bpf_prog_put(prog);
> -		}
> -
> -		if (prog_id != expected_id) {
> -			NL_SET_ERR_MSG(extack, "Active program does not match expected");
> -			return -EEXIST;
> -		}
> +	mode = dev_xdp_mode(flags);
> +	cur_prog = dev_xdp_prog(dev, mode);
> +	if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) {
> +		NL_SET_ERR_MSG(extack, "Active program does not match expected");
> +		return -EEXIST;
>  	}
> -	if (fd >= 0) {
> +	if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) {
> +		NL_SET_ERR_MSG(extack, "XDP program already attached");
> +		return -EBUSY;
> +	}
> +
> +	if (new_prog) {
> +		bool offload = mode == XDP_MODE_HW;
>  		enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB
>  					       ? XDP_MODE_DRV : XDP_MODE_SKB;
>  
> -		if (!offload && dev_xdp_prog_id(dev, other_mode)) {
> +		if (!offload && dev_xdp_prog(dev, other_mode)) {
>  			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) && prog_id) {
> -			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);
> -		if (IS_ERR(prog))
> -			return PTR_ERR(prog);
> -
> -		if (!offload && bpf_prog_is_dev_bound(prog->aux)) {
> +		if (!offload && bpf_prog_is_dev_bound(new_prog->aux)) {
>  			NL_SET_ERR_MSG(extack, "Using device-bound program without HW_MODE flag is not supported");
> -			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> -
> -		if (prog->expected_attach_type == BPF_XDP_DEVMAP) {
> +		if (new_prog->expected_attach_type == BPF_XDP_DEVMAP) {
>  			NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device");
> -			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> -
> -		if (prog->expected_attach_type == BPF_XDP_CPUMAP) {
> -			NL_SET_ERR_MSG(extack,
> -				       "BPF_XDP_CPUMAP programs can not be attached to a device");
> -			bpf_prog_put(prog);
> +		if (new_prog->expected_attach_type == BPF_XDP_CPUMAP) {
> +			NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");

bpf_prog_put() missing?

>  			return -EINVAL;
>  		}
> +	}
>  
> -		/* prog->aux->id may be 0 for orphaned device-bound progs */
> -		if (prog->aux->id && prog->aux->id == prog_id) {
> -			bpf_prog_put(prog);
> -			return 0;
> +	/* don't call drivers if the effective program didn't change */
> +	if (new_prog != cur_prog) {
> +		bpf_op = dev_xdp_bpf_op(dev, mode);
> +		if (!bpf_op) {
> +			NL_SET_ERR_MSG(extack, "Underlying driver does not support XDP in native mode");
> +			return -EOPNOTSUPP;
>  		}
> -	} else {
> -		if (!prog_id)
> -			return 0;
> -		prog = NULL;
> -	}
>  
> -	err = dev_xdp_install(dev, mode, bpf_op, extack, flags, prog);
> -	if (err < 0 && prog) {
> -		bpf_prog_put(prog);
> -		return err;
> +		err = dev_xdp_install(dev, mode, bpf_op, extack, flags, new_prog);
> +		if (err)
> +			return err;
>  	}
> -	dev_xdp_set_prog(dev, mode, prog);
> +
> +	dev_xdp_set_prog(dev, mode, new_prog);
> +	if (cur_prog)
> +		bpf_prog_put(cur_prog);
>  
>  	return 0;
>  }
>  
> +/**
> + *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
> + *	@dev: device
> + *	@extack: netlink extended ack
> + *	@fd: new program fd or negative value to clear
> + *	@expected_fd: old program fd that userspace expects to replace or clear
> + *	@flags: xdp-related flags
> + *
> + *	Set or clear a bpf program for a device
> + */
> +int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> +		      int fd, int expected_fd, u32 flags)
> +{
> +	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> +	struct bpf_prog *new_prog = NULL, *old_prog = NULL;
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	if (fd >= 0) {
> +		new_prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
> +						 mode != XDP_MODE_SKB);
> +		if (IS_ERR(new_prog))
> +			return PTR_ERR(new_prog);
> +	}
> +
> +	if (expected_fd >= 0) {
> +		old_prog = bpf_prog_get_type_dev(expected_fd, BPF_PROG_TYPE_XDP,
> +						 mode != XDP_MODE_SKB);
> +		if (IS_ERR(old_prog)) {
> +			err = PTR_ERR(old_prog);
> +			old_prog = NULL;
> +			goto err_out;
> +		}
> +	}
> +
> +	err = dev_xdp_attach(dev, extack, new_prog, old_prog, flags);
> +
> +err_out:
> +	if (err && new_prog)
> +		bpf_prog_put(new_prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +	return err;
> +}
> +
>  /**
>   *	dev_new_index	-	allocate an ifindex
>   *	@net: the applicable net namespace
> -- 
> 2.24.1
> 

Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[Andrii Nakryiko]

On Wed, Jul 22, 2020 at 12:18 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Tue, Jul 21, 2020 at 11:45:56PM -0700, Andrii Nakryiko wrote:
> > Further refactor XDP attachment code. dev_change_xdp_fd() is split into two
> > parts: getting bpf_progs from FDs and attachment logic, working with
> > bpf_progs. This makes attachment  logic a bit more straightforward and
> > prepares code for bpf_xdp_link inclusion, which will share the common logic.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  net/core/dev.c | 165 +++++++++++++++++++++++++++----------------------
> >  1 file changed, 91 insertions(+), 74 deletions(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 7e753e248cef..abf573b2dcf4 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct net_device *dev)
> >       }
> >  }
> >
> > -/**
> > - *   dev_change_xdp_fd - set or clear a bpf program for a device rx path
> > - *   @dev: device
> > - *   @extack: netlink extended ack
> > - *   @fd: new program fd or negative value to clear
> > - *   @expected_fd: old program fd that userspace expects to replace or clear
> > - *   @flags: xdp-related flags
> > - *
> > - *   Set or clear a bpf program for a device
> > - */
> > -int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> > -                   int fd, int expected_fd, u32 flags)
> > +static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
> > +                       struct bpf_prog *new_prog, struct bpf_prog *old_prog,
> > +                       u32 flags)
> >  {
> > -     const struct net_device_ops *ops = dev->netdev_ops;
> > -     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > -     bool offload = mode == XDP_MODE_HW;
> > -     u32 prog_id, expected_id = 0;
> > -     struct bpf_prog *prog;
> > +     struct bpf_prog *cur_prog;
> > +     enum bpf_xdp_mode mode;
> >       bpf_op_t bpf_op;
> >       int err;
> >
> >       ASSERT_RTNL();
>
> couldn't we rely on caller's rtnl assertion? dev_change_xdp_fd() already
> has one.

dev_xdp_attach() is also used from the bpf_link attaching function
(dev_xdp_attach_link() in the later patch). I can remove ASSERT_RTNL()
from dev_change_xdp_fd(), though, it doesn't have to do that check, if
dev_xdp_attach() does it already.

[...]

> > -
> > -             if (prog->expected_attach_type == BPF_XDP_CPUMAP) {
> > -                     NL_SET_ERR_MSG(extack,
> > -                                    "BPF_XDP_CPUMAP programs can not be attached to a device");
> > -                     bpf_prog_put(prog);
> > +             if (new_prog->expected_attach_type == BPF_XDP_CPUMAP) {
> > +                     NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP programs can not be attached to a device");
>
> bpf_prog_put() missing?
>

Nope, program putting on error is handled outside the
dev_xdp_attach(), either by bpf() LINK_CREATE handling logic or by
dev_change_xdp_fd().

> >                       return -EINVAL;
> >               }
> > +     }
> >

[...]

Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[Shay Agroskin]

Andrii Nakryiko <andriin@fb.com> writes:

> Further refactor XDP attachment code. dev_change_xdp_fd() is 
> split into two
> parts: getting bpf_progs from FDs and attachment logic, working 
> with
> bpf_progs. This makes attachment  logic a bit more 
> straightforward and
> prepares code for bpf_xdp_link inclusion, which will share the 
> common logic.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  net/core/dev.c | 165 
>  +++++++++++++++++++++++++++----------------------
>  1 file changed, 91 insertions(+), 74 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 7e753e248cef..abf573b2dcf4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct 
> net_device *dev)
>  	}
>  }
>  
> -/**
> - *	dev_change_xdp_fd - set or clear a bpf program for a 
> device rx path
> - *	@dev: device
> - *	@extack: netlink extended ack
> - *	@fd: new program fd or negative value to clear
> - *	@expected_fd: old program fd that userspace expects to 
> replace or clear
> - *	@flags: xdp-related flags
> - *
> - *	Set or clear a bpf program for a device
> - */
> -int dev_change_xdp_fd(struct net_device *dev, struct 
> netlink_ext_ack *extack,
> -		      int fd, int expected_fd, u32 flags)
> +static int dev_xdp_attach(struct net_device *dev, struct 
> netlink_ext_ack *extack,
> +			  struct bpf_prog *new_prog, struct 
> bpf_prog *old_prog,
> +			  u32 flags)
>  {
> -	const struct net_device_ops *ops = dev->netdev_ops;
> -	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> -	bool offload = mode == XDP_MODE_HW;
> -	u32 prog_id, expected_id = 0;
> -	struct bpf_prog *prog;
> +	struct bpf_prog *cur_prog;
> +	enum bpf_xdp_mode mode;
>  	bpf_op_t bpf_op;
>  	int err;
>  
>  	ASSERT_RTNL();
>  
> -	bpf_op = dev_xdp_bpf_op(dev, mode);
> -	if (!bpf_op) {
> -		NL_SET_ERR_MSG(extack, "underlying driver does not 
> support XDP in native mode");
> -		return -EOPNOTSUPP;
> +	/* just one XDP mode bit should be set, zero defaults to 
> SKB mode */
> +	if (hweight32(flags & XDP_FLAGS_MODES) > 1) {

Not sure if it's more efficient but running
    if ((flags & XDP) & ((flags & XDP) - 1) != 0)

returns whether a number is a multiple of 2.
Should be equivalent to what you checked with hweight32. It is 
less readable though. Just thought I'd throw that in.
Taken from 
https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2

> +		NL_SET_ERR_MSG(extack, "Only one XDP mode flag can 
> be set");
> +		return -EINVAL;
> +	}
> +	/* old_prog != NULL implies XDP_FLAGS_REPLACE is set */
> +	if (old_prog && !(flags & XDP_FLAGS_REPLACE)) {
> +		NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not 
> specified");
> +		return -EINVAL;
>  	}
>  
> -	prog_id = dev_xdp_prog_id(dev, mode);
> -	if (flags & XDP_FLAGS_REPLACE) {
> -		if (expected_fd >= 0) {
> -			prog = bpf_prog_get_type_dev(expected_fd,
> - 
> BPF_PROG_TYPE_XDP,
> -						     bpf_op == 
> ops->ndo_bpf);
> -			if (IS_ERR(prog))
> -				return PTR_ERR(prog);
> -			expected_id = prog->aux->id;
> -			bpf_prog_put(prog);
> -		}
> -
> -		if (prog_id != expected_id) {
> -			NL_SET_ERR_MSG(extack, "Active program 
> does not match expected");
> -			return -EEXIST;
> -		}
> +	mode = dev_xdp_mode(flags);
> +	cur_prog = dev_xdp_prog(dev, mode);
> +	if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) {
> +		NL_SET_ERR_MSG(extack, "Active program does not 
> match expected");
> +		return -EEXIST;
>  	}
> -	if (fd >= 0) {
> +	if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && cur_prog) {
> +		NL_SET_ERR_MSG(extack, "XDP program already 
> attached");
> +		return -EBUSY;
> +	}
> +
> +	if (new_prog) {
> +		bool offload = mode == XDP_MODE_HW;
>  		enum bpf_xdp_mode other_mode = mode == 
>  XDP_MODE_SKB
>  					       ? XDP_MODE_DRV : 
>  XDP_MODE_SKB;
>  
> -		if (!offload && dev_xdp_prog_id(dev, other_mode)) 
> {
> +		if (!offload && dev_xdp_prog(dev, other_mode)) {
>  			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) && 
> prog_id) {
> -			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);
> -		if (IS_ERR(prog))
> -			return PTR_ERR(prog);
> -
> -		if (!offload && bpf_prog_is_dev_bound(prog->aux)) 
> {
> +		if (!offload && 
> bpf_prog_is_dev_bound(new_prog->aux)) {
>  			NL_SET_ERR_MSG(extack, "Using device-bound 
>  program without HW_MODE flag is not supported");
> -			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> -
> -		if (prog->expected_attach_type == BPF_XDP_DEVMAP) 
> {
> +		if (new_prog->expected_attach_type == 
> BPF_XDP_DEVMAP) {
>  			NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP 
>  programs can not be attached to a device");
> -			bpf_prog_put(prog);
>  			return -EINVAL;
>  		}
> -
> -		if (prog->expected_attach_type == BPF_XDP_CPUMAP) 
> {
> -			NL_SET_ERR_MSG(extack,
> -				       "BPF_XDP_CPUMAP programs 
> can not be attached to a device");
> -			bpf_prog_put(prog);
> +		if (new_prog->expected_attach_type == 
> BPF_XDP_CPUMAP) {
> +			NL_SET_ERR_MSG(extack, "BPF_XDP_CPUMAP 
> programs can not be attached to a device");
>  			return -EINVAL;
>  		}
> +	}
>  
> -		/* prog->aux->id may be 0 for orphaned 
> device-bound progs */
> -		if (prog->aux->id && prog->aux->id == prog_id) {
> -			bpf_prog_put(prog);
> -			return 0;
> +	/* don't call drivers if the effective program didn't 
> change */
> +	if (new_prog != cur_prog) {
> +		bpf_op = dev_xdp_bpf_op(dev, mode);
> +		if (!bpf_op) {
> +			NL_SET_ERR_MSG(extack, "Underlying driver 
> does not support XDP in native mode");
> +			return -EOPNOTSUPP;
>  		}
> -	} else {
> -		if (!prog_id)
> -			return 0;
> -		prog = NULL;
> -	}
>  
> -	err = dev_xdp_install(dev, mode, bpf_op, extack, flags, 
> prog);
> -	if (err < 0 && prog) {
> -		bpf_prog_put(prog);
> -		return err;
> +		err = dev_xdp_install(dev, mode, bpf_op, extack, 
> flags, new_prog);
> +		if (err)
> +			return err;
>  	}
> -	dev_xdp_set_prog(dev, mode, prog);
> +
> +	dev_xdp_set_prog(dev, mode, new_prog);
> +	if (cur_prog)
> +		bpf_prog_put(cur_prog);
>  
>  	return 0;
>  }
>  
> +/**
> + *	dev_change_xdp_fd - set or clear a bpf program for a 
> device rx path
> + *	@dev: device
> + *	@extack: netlink extended ack
> + *	@fd: new program fd or negative value to clear
> + *	@expected_fd: old program fd that userspace expects to 
> replace or clear
> + *	@flags: xdp-related flags
> + *
> + *	Set or clear a bpf program for a device
> + */
> +int dev_change_xdp_fd(struct net_device *dev, struct 
> netlink_ext_ack *extack,
> +		      int fd, int expected_fd, u32 flags)
> +{
> +	enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> +	struct bpf_prog *new_prog = NULL, *old_prog = NULL;
> +	int err;
> +
> +	ASSERT_RTNL();
> +
> +	if (fd >= 0) {
> +		new_prog = bpf_prog_get_type_dev(fd, 
> BPF_PROG_TYPE_XDP,
> +						 mode != 
> XDP_MODE_SKB);
> +		if (IS_ERR(new_prog))
> +			return PTR_ERR(new_prog);
> +	}
> +
> +	if (expected_fd >= 0) {
> +		old_prog = bpf_prog_get_type_dev(expected_fd, 
> BPF_PROG_TYPE_XDP,
> +						 mode != 
> XDP_MODE_SKB);
> +		if (IS_ERR(old_prog)) {
> +			err = PTR_ERR(old_prog);
> +			old_prog = NULL;
> +			goto err_out;
> +		}
> +	}
> +
> +	err = dev_xdp_attach(dev, extack, new_prog, old_prog, 
> flags);
> +
> +err_out:
> +	if (err && new_prog)
> +		bpf_prog_put(new_prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +	return err;
> +}
> +
>  /**
>   *	dev_new_index	-	allocate an ifindex
>   *	@net: the applicable net namespace

Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[Andrii Nakryiko]

On Mon, Jul 27, 2020 at 5:08 AM Shay Agroskin <shayagr@amazon.com> wrote:
>
>
> Andrii Nakryiko <andriin@fb.com> writes:
>
> > Further refactor XDP attachment code. dev_change_xdp_fd() is
> > split into two
> > parts: getting bpf_progs from FDs and attachment logic, working
> > with
> > bpf_progs. This makes attachment  logic a bit more
> > straightforward and
> > prepares code for bpf_xdp_link inclusion, which will share the
> > common logic.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> >  net/core/dev.c | 165
> >  +++++++++++++++++++++++++++----------------------
> >  1 file changed, 91 insertions(+), 74 deletions(-)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 7e753e248cef..abf573b2dcf4 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8815,111 +8815,128 @@ static void dev_xdp_uninstall(struct
> > net_device *dev)
> >       }
> >  }
> >
> > -/**
> > - *   dev_change_xdp_fd - set or clear a bpf program for a
> > device rx path
> > - *   @dev: device
> > - *   @extack: netlink extended ack
> > - *   @fd: new program fd or negative value to clear
> > - *   @expected_fd: old program fd that userspace expects to
> > replace or clear
> > - *   @flags: xdp-related flags
> > - *
> > - *   Set or clear a bpf program for a device
> > - */
> > -int dev_change_xdp_fd(struct net_device *dev, struct
> > netlink_ext_ack *extack,
> > -                   int fd, int expected_fd, u32 flags)
> > +static int dev_xdp_attach(struct net_device *dev, struct
> > netlink_ext_ack *extack,
> > +                       struct bpf_prog *new_prog, struct
> > bpf_prog *old_prog,
> > +                       u32 flags)
> >  {
> > -     const struct net_device_ops *ops = dev->netdev_ops;
> > -     enum bpf_xdp_mode mode = dev_xdp_mode(flags);
> > -     bool offload = mode == XDP_MODE_HW;
> > -     u32 prog_id, expected_id = 0;
> > -     struct bpf_prog *prog;
> > +     struct bpf_prog *cur_prog;
> > +     enum bpf_xdp_mode mode;
> >       bpf_op_t bpf_op;
> >       int err;
> >
> >       ASSERT_RTNL();
> >
> > -     bpf_op = dev_xdp_bpf_op(dev, mode);
> > -     if (!bpf_op) {
> > -             NL_SET_ERR_MSG(extack, "underlying driver does not
> > support XDP in native mode");
> > -             return -EOPNOTSUPP;
> > +     /* just one XDP mode bit should be set, zero defaults to
> > SKB mode */
> > +     if (hweight32(flags & XDP_FLAGS_MODES) > 1) {
>
> Not sure if it's more efficient but running
>     if ((flags & XDP) & ((flags & XDP) - 1) != 0)
>
> returns whether a number is a multiple of 2.
> Should be equivalent to what you checked with hweight32. It is
> less readable though. Just thought I'd throw that in.

so I just preserved what is there in netlink-handling code. It also is
not a performance-critical part. What you propose might work, but
using hweight32 is more explicit about allowing zero or one bits set.


> Taken from
> https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2
>
> > +             NL_SET_ERR_MSG(extack, "Only one XDP mode flag can
> > be set");
> > +             return -EINVAL;
> > +     }
> > +     /* old_prog != NULL implies XDP_FLAGS_REPLACE is set */
> > +     if (old_prog && !(flags & XDP_FLAGS_REPLACE)) {
> > +             NL_SET_ERR_MSG(extack, "XDP_FLAGS_REPLACE is not
> > specified");
> > +             return -EINVAL;
> >       }
> >

[...]

Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[sdf]

On 07/21, Andrii Nakryiko wrote:
> Further refactor XDP attachment code. dev_change_xdp_fd() is split into  
> two
> parts: getting bpf_progs from FDs and attachment logic, working with
> bpf_progs. This makes attachment  logic a bit more straightforward and
> prepares code for bpf_xdp_link inclusion, which will share the common  
> logic.
It looks like this patch breaks xdp tests for me:
* test_xdping.sh
* test_xdp_vlan.sh

Can you please verify on your side?

Looking at tools/testing/selftests/bpf/xdping.c I see it has:
static __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST;

And it attaches program two times in the same net namespace,
so I don't see how it could've worked before the change :-/
(unless, of coarse, the previous code was buggy).

Re: [PATCH v4 bpf-next 3/9] bpf, xdp: extract common XDP program attachment logic

[Andrii Nakryiko]

On Tue, Aug 11, 2020 at 11:14 AM <sdf@google.com> wrote:
>
> On 07/21, Andrii Nakryiko wrote:
> > Further refactor XDP attachment code. dev_change_xdp_fd() is split into
> > two
> > parts: getting bpf_progs from FDs and attachment logic, working with
> > bpf_progs. This makes attachment  logic a bit more straightforward and
> > prepares code for bpf_xdp_link inclusion, which will share the common
> > logic.
> It looks like this patch breaks xdp tests for me:
> * test_xdping.sh
> * test_xdp_vlan.sh
>
> Can you please verify on your side?
>
> Looking at tools/testing/selftests/bpf/xdping.c I see it has:
> static __u32 xdp_flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
>
> And it attaches program two times in the same net namespace,
> so I don't see how it could've worked before the change :-/
> (unless, of coarse, the previous code was buggy).

Ok, so according to the old logic, XDP_FLAGS_UPDATE_IF_NOEXIST flag is
only checked if new program fd is not -1. So if we are installing a
new program and specify XDP_FLAGS_UPDATE_IF_NOEXIST, we'll be allowed
to do this only if there is no BPF program already attached. But we
are uninstalling program, then XDP_FLAGS_UPDATE_IF_NOEXIST is ignored
and we are allowed to uninstall any BPF program.

I can easily fix this by moving the XDP_FLAGS_UPDATE_IF_NOEXIST check
inside `if (new_prog) {}` section. I'm not sure which semantics was
actually originally intended. Maybe XDP folks can chime in here?

[PATCH v4 bpf-next 4/9] bpf, xdp: add bpf_link-based XDP attachment API

[Andrii Nakryiko]

Add bpf_link-based API (bpf_xdp_link) to attach BPF XDP program through
BPF_LINK_CREATE command.

bpf_xdp_link is mutually exclusive with direct BPF program attachment,
previous BPF program should be detached prior to attempting to create a new
bpf_xdp_link attachment (for a given XDP mode). Once BPF link is attached, it
can't be replaced by other BPF program attachment or link attachment. It will
be detached only when the last BPF link FD is closed.

bpf_xdp_link will be auto-detached when net_device is shutdown, similarly to
how other BPF links behave (cgroup, flow_dissector). At that point bpf_link
will become defunct, but won't be destroyed until last FD is closed.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 include/linux/netdevice.h |   4 +
 include/uapi/linux/bpf.h  |   7 +-
 kernel/bpf/syscall.c      |   5 ++
 net/core/dev.c            | 169 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 178 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cad44b40c776..7d3c412fcfe5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -888,6 +888,7 @@ struct bpf_prog_offload_ops;
 struct netlink_ext_ack;
 struct xdp_umem;
 struct xdp_dev_bulk_queue;
+struct bpf_xdp_link;
 
 enum bpf_xdp_mode {
 	XDP_MODE_SKB = 0,
@@ -898,6 +899,7 @@ enum bpf_xdp_mode {
 
 struct bpf_xdp_entity {
 	struct bpf_prog *prog;
+	struct bpf_xdp_link *link;
 };
 
 struct netdev_bpf {
@@ -3831,7 +3833,9 @@ 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, int expected_fd, u32 flags);
+int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
 u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode 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/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 54d0c886e3ba..1da0c2984e7f 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -230,6 +230,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_INET_SOCK_RELEASE,
 	BPF_XDP_CPUMAP,
 	BPF_SK_LOOKUP,
+	BPF_XDP,
 	__MAX_BPF_ATTACH_TYPE
 };
 
@@ -242,6 +243,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_CGROUP = 3,
 	BPF_LINK_TYPE_ITER = 4,
 	BPF_LINK_TYPE_NETNS = 5,
+	BPF_LINK_TYPE_XDP = 6,
 
 	MAX_BPF_LINK_TYPE,
 };
@@ -607,7 +609,10 @@ union bpf_attr {
 
 	struct { /* struct used by BPF_LINK_CREATE command */
 		__u32		prog_fd;	/* eBPF program to attach */
-		__u32		target_fd;	/* object to attach to */
+		union {
+			__u32		target_fd;	/* object to attach to */
+			__u32		target_ifindex; /* target ifindex */
+		};
 		__u32		attach_type;	/* attach type */
 		__u32		flags;		/* extra flags */
 	} link_create;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d07417d17712..0244030f214f 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2824,6 +2824,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
 		return BPF_PROG_TYPE_TRACING;
 	case BPF_SK_LOOKUP:
 		return BPF_PROG_TYPE_SK_LOOKUP;
+	case BPF_XDP:
+		return BPF_PROG_TYPE_XDP;
 	default:
 		return BPF_PROG_TYPE_UNSPEC;
 	}
@@ -3902,6 +3904,9 @@ static int link_create(union bpf_attr *attr)
 	case BPF_PROG_TYPE_SK_LOOKUP:
 		ret = netns_bpf_link_create(attr, prog);
 		break;
+	case BPF_PROG_TYPE_XDP:
+		ret = bpf_xdp_link_attach(attr, prog);
+		break;
 	default:
 		ret = -EINVAL;
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index abf573b2dcf4..f916072f31de 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8716,6 +8716,12 @@ int dev_change_proto_down_generic(struct net_device *dev, bool proto_down)
 }
 EXPORT_SYMBOL(dev_change_proto_down_generic);
 
+struct bpf_xdp_link {
+	struct bpf_link link;
+	struct net_device *dev; /* protected by rtnl_lock, no refcnt held */
+	int flags;
+};
+
 static enum bpf_xdp_mode dev_xdp_mode(u32 flags)
 {
 	if (flags & XDP_FLAGS_HW_MODE)
@@ -8738,9 +8744,19 @@ static bpf_op_t dev_xdp_bpf_op(struct net_device *dev, enum bpf_xdp_mode mode)
 	};
 }
 
+static struct bpf_xdp_link *dev_xdp_link(struct net_device *dev,
+					 enum bpf_xdp_mode mode)
+{
+	return dev->xdp_state[mode].link;
+}
+
 static struct bpf_prog *dev_xdp_prog(struct net_device *dev,
 				     enum bpf_xdp_mode mode)
 {
+	struct bpf_xdp_link *link = dev_xdp_link(dev, mode);
+
+	if (link)
+		return link->link.prog;
 	return dev->xdp_state[mode].prog;
 }
 
@@ -8751,9 +8767,17 @@ u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode)
 	return prog ? prog->aux->id : 0;
 }
 
+static void dev_xdp_set_link(struct net_device *dev, enum bpf_xdp_mode mode,
+			     struct bpf_xdp_link *link)
+{
+	dev->xdp_state[mode].link = link;
+	dev->xdp_state[mode].prog = NULL;
+}
+
 static void dev_xdp_set_prog(struct net_device *dev, enum bpf_xdp_mode mode,
 			     struct bpf_prog *prog)
 {
+	dev->xdp_state[mode].link = NULL;
 	dev->xdp_state[mode].prog = prog;
 }
 
@@ -8793,6 +8817,7 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 
 static void dev_xdp_uninstall(struct net_device *dev)
 {
+	struct bpf_xdp_link *link;
 	struct bpf_prog *prog;
 	enum bpf_xdp_mode mode;
 	bpf_op_t bpf_op;
@@ -8810,14 +8835,20 @@ static void dev_xdp_uninstall(struct net_device *dev)
 
 		WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL));
 
-		bpf_prog_put(prog);
-		dev_xdp_set_prog(dev, mode, NULL);
+		/* auto-detach link from net device */
+		link = dev_xdp_link(dev, mode);
+		if (link)
+			link->dev = NULL;
+		else
+			bpf_prog_put(prog);
+
+		dev_xdp_set_link(dev, mode, NULL);
 	}
 }
 
 static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack,
-			  struct bpf_prog *new_prog, struct bpf_prog *old_prog,
-			  u32 flags)
+			  struct bpf_xdp_link *link, struct bpf_prog *new_prog,
+			  struct bpf_prog *old_prog, u32 flags)
 {
 	struct bpf_prog *cur_prog;
 	enum bpf_xdp_mode mode;
@@ -8826,6 +8857,14 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 
 	ASSERT_RTNL();
 
+	/* either link or prog attachment, never both */
+	if (link && (new_prog || old_prog))
+		return -EINVAL;
+	/* link supports only XDP mode flags */
+	if (link && (flags & ~XDP_FLAGS_MODES)) {
+		NL_SET_ERR_MSG(extack, "Invalid XDP flags for BPF link attachment");
+		return -EINVAL;
+	}
 	/* just one XDP mode bit should be set, zero defaults to SKB mode */
 	if (hweight32(flags & XDP_FLAGS_MODES) > 1) {
 		NL_SET_ERR_MSG(extack, "Only one XDP mode flag can be set");
@@ -8838,7 +8877,18 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 	}
 
 	mode = dev_xdp_mode(flags);
+	/* can't replace attached link */
+	if (dev_xdp_link(dev, mode)) {
+		NL_SET_ERR_MSG(extack, "Can't replace active BPF XDP link");
+		return -EBUSY;
+	}
+
 	cur_prog = dev_xdp_prog(dev, mode);
+	/* can't replace attached prog with link */
+	if (link && cur_prog) {
+		NL_SET_ERR_MSG(extack, "Can't replace active XDP program with BPF link");
+		return -EBUSY;
+	}
 	if ((flags & XDP_FLAGS_REPLACE) && cur_prog != old_prog) {
 		NL_SET_ERR_MSG(extack, "Active program does not match expected");
 		return -EEXIST;
@@ -8848,6 +8898,10 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 		return -EBUSY;
 	}
 
+	/* put effective new program into new_prog */
+	if (link)
+		new_prog = link->link.prog;
+
 	if (new_prog) {
 		bool offload = mode == XDP_MODE_HW;
 		enum bpf_xdp_mode other_mode = mode == XDP_MODE_SKB
@@ -8884,13 +8938,116 @@ static int dev_xdp_attach(struct net_device *dev, struct netlink_ext_ack *extack
 			return err;
 	}
 
-	dev_xdp_set_prog(dev, mode, new_prog);
+	if (link)
+		dev_xdp_set_link(dev, mode, link);
+	else
+		dev_xdp_set_prog(dev, mode, new_prog);
 	if (cur_prog)
 		bpf_prog_put(cur_prog);
 
 	return 0;
 }
 
+static int dev_xdp_attach_link(struct net_device *dev,
+			       struct netlink_ext_ack *extack,
+			       struct bpf_xdp_link *link)
+{
+	return dev_xdp_attach(dev, extack, link, NULL, NULL, link->flags);
+}
+
+static int dev_xdp_detach_link(struct net_device *dev,
+			       struct netlink_ext_ack *extack,
+			       struct bpf_xdp_link *link)
+{
+	enum bpf_xdp_mode mode;
+	bpf_op_t bpf_op;
+
+	ASSERT_RTNL();
+
+	mode = dev_xdp_mode(link->flags);
+	if (dev_xdp_link(dev, mode) != link)
+		return -EINVAL;
+
+	bpf_op = dev_xdp_bpf_op(dev, mode);
+	WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL));
+	dev_xdp_set_link(dev, mode, NULL);
+	return 0;
+}
+
+static void bpf_xdp_link_release(struct bpf_link *link)
+{
+	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
+
+	rtnl_lock();
+
+	/* if racing with net_device's tear down, xdp_link->dev might be
+	 * already NULL, in which case link was already auto-detached
+	 */
+	if (xdp_link->dev)
+		WARN_ON(dev_xdp_detach_link(xdp_link->dev, NULL, xdp_link));
+
+	rtnl_unlock();
+}
+
+static void bpf_xdp_link_dealloc(struct bpf_link *link)
+{
+	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
+
+	kfree(xdp_link);
+}
+
+static const struct bpf_link_ops bpf_xdp_link_lops = {
+	.release = bpf_xdp_link_release,
+	.dealloc = bpf_xdp_link_dealloc,
+};
+
+int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct net *net = current->nsproxy->net_ns;
+	struct bpf_link_primer link_primer;
+	struct bpf_xdp_link *link;
+	struct net_device *dev;
+	int err, fd;
+
+	dev = dev_get_by_index(net, attr->link_create.target_ifindex);
+	if (!dev)
+		return -EINVAL;
+
+	link = kzalloc(sizeof(*link), GFP_USER);
+	if (!link) {
+		err = -ENOMEM;
+		goto out_put_dev;
+	}
+
+	bpf_link_init(&link->link, BPF_LINK_TYPE_XDP, &bpf_xdp_link_lops, prog);
+	link->dev = dev;
+	link->flags = attr->link_create.flags;
+
+	err = bpf_link_prime(&link->link, &link_primer);
+	if (err) {
+		kfree(link);
+		goto out_put_dev;
+	}
+
+	rtnl_lock();
+	err = dev_xdp_attach_link(dev, NULL, link);
+	rtnl_unlock();
+
+	if (err) {
+		bpf_link_cleanup(&link_primer);
+		goto out_put_dev;
+	}
+
+	fd = bpf_link_settle(&link_primer);
+	/* link itself doesn't hold dev's refcnt to not complicate shutdown */
+	dev_put(dev);
+	return fd;
+
+out_put_dev:
+	dev_put(dev);
+	return err;
+}
+
 /**
  *	dev_change_xdp_fd - set or clear a bpf program for a device rx path
  *	@dev: device
@@ -8927,7 +9084,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		}
 	}
 
-	err = dev_xdp_attach(dev, extack, new_prog, old_prog, flags);
+	err = dev_xdp_attach(dev, extack, NULL, new_prog, old_prog, flags);
 
 err_out:
 	if (err && new_prog)
-- 
2.24.1

[PATCH v4 bpf-next 5/9] bpf, xdp: implement LINK_UPDATE for BPF XDP link

[Andrii Nakryiko]

Add support for LINK_UPDATE command for BPF XDP link to enable reliable
replacement of underlying BPF program.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 net/core/dev.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index f916072f31de..44264eff7f1c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8996,9 +8996,52 @@ static void bpf_xdp_link_dealloc(struct bpf_link *link)
 	kfree(xdp_link);
 }
 
+static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
+			       struct bpf_prog *old_prog)
+{
+	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
+	enum bpf_xdp_mode mode;
+	bpf_op_t bpf_op;
+	int err = 0;
+
+	rtnl_lock();
+
+	/* link might have been auto-released already, so fail */
+	if (!xdp_link->dev) {
+		err = -ENOLINK;
+		goto out_unlock;
+	}
+
+	if (old_prog && link->prog != old_prog) {
+		err = -EPERM;
+		goto out_unlock;
+	}
+	old_prog = link->prog;
+	if (old_prog == new_prog) {
+		/* no-op, don't disturb drivers */
+		bpf_prog_put(new_prog);
+		goto out_unlock;
+	}
+
+	mode = dev_xdp_mode(xdp_link->flags);
+	bpf_op = dev_xdp_bpf_op(xdp_link->dev, mode);
+	err = dev_xdp_install(xdp_link->dev, mode, bpf_op, NULL,
+			      xdp_link->flags, new_prog);
+	if (err)
+		goto out_unlock;
+
+	old_prog = xchg(&link->prog, new_prog);
+	bpf_prog_put(old_prog);
+
+out_unlock:
+	rtnl_unlock();
+	return err;
+}
+
 static const struct bpf_link_ops bpf_xdp_link_lops = {
 	.release = bpf_xdp_link_release,
 	.dealloc = bpf_xdp_link_dealloc,
+	.update_prog = bpf_xdp_link_update,
 };
 
 int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
-- 
2.24.1

[PATCH v4 bpf-next 6/9] bpf: implement BPF XDP link-specific introspection APIs

[Andrii Nakryiko]

Implement XDP link-specific show_fdinfo and link_info to emit ifindex.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 include/uapi/linux/bpf.h |  3 +++
 net/core/dev.c           | 31 +++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 1da0c2984e7f..27bd1c8b0041 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4062,6 +4062,9 @@ struct bpf_link_info {
 			__u32 netns_ino;
 			__u32 attach_type;
 		} netns;
+		struct {
+			__u32 ifindex;
+		} xdp;
 	};
 } __attribute__((aligned(8)));
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 44264eff7f1c..9859dae7d4dc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8996,6 +8996,35 @@ static void bpf_xdp_link_dealloc(struct bpf_link *link)
 	kfree(xdp_link);
 }
 
+static void bpf_xdp_link_show_fdinfo(const struct bpf_link *link,
+				     struct seq_file *seq)
+{
+	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
+	u32 ifindex = 0;
+
+	rtnl_lock();
+	if (xdp_link->dev)
+		ifindex = xdp_link->dev->ifindex;
+	rtnl_unlock();
+
+	seq_printf(seq, "ifindex:\t%u\n", ifindex);
+}
+
+static int bpf_xdp_link_fill_link_info(const struct bpf_link *link,
+				       struct bpf_link_info *info)
+{
+	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
+	u32 ifindex = 0;
+
+	rtnl_lock();
+	if (xdp_link->dev)
+		ifindex = xdp_link->dev->ifindex;
+	rtnl_unlock();
+
+	info->xdp.ifindex = ifindex;
+	return 0;
+}
+
 static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 			       struct bpf_prog *old_prog)
 {
@@ -9041,6 +9070,8 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 static const struct bpf_link_ops bpf_xdp_link_lops = {
 	.release = bpf_xdp_link_release,
 	.dealloc = bpf_xdp_link_dealloc,
+	.show_fdinfo = bpf_xdp_link_show_fdinfo,
+	.fill_link_info = bpf_xdp_link_fill_link_info,
 	.update_prog = bpf_xdp_link_update,
 };
 
-- 
2.24.1

[PATCH v4 bpf-next 7/9] libbpf: add support for BPF XDP link

[Andrii Nakryiko]

Sync UAPI header and add support for using bpf_link-based XDP attachment.
Make xdp/ prog type set expected attach type. Kernel didn't enforce
attach_type for XDP programs before, so there is no backwards compatiblity
issues there.

Also fix section_names selftest to recognize that xdp prog types now have
expected attach type.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/include/uapi/linux/bpf.h                         | 10 +++++++++-
 tools/lib/bpf/libbpf.c                                 |  9 ++++++++-
 tools/lib/bpf/libbpf.h                                 |  2 ++
 tools/lib/bpf/libbpf.map                               |  1 +
 tools/testing/selftests/bpf/prog_tests/section_names.c |  2 +-
 5 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 54d0c886e3ba..27bd1c8b0041 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -230,6 +230,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_INET_SOCK_RELEASE,
 	BPF_XDP_CPUMAP,
 	BPF_SK_LOOKUP,
+	BPF_XDP,
 	__MAX_BPF_ATTACH_TYPE
 };
 
@@ -242,6 +243,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_CGROUP = 3,
 	BPF_LINK_TYPE_ITER = 4,
 	BPF_LINK_TYPE_NETNS = 5,
+	BPF_LINK_TYPE_XDP = 6,
 
 	MAX_BPF_LINK_TYPE,
 };
@@ -607,7 +609,10 @@ union bpf_attr {
 
 	struct { /* struct used by BPF_LINK_CREATE command */
 		__u32		prog_fd;	/* eBPF program to attach */
-		__u32		target_fd;	/* object to attach to */
+		union {
+			__u32		target_fd;	/* object to attach to */
+			__u32		target_ifindex; /* target ifindex */
+		};
 		__u32		attach_type;	/* attach type */
 		__u32		flags;		/* extra flags */
 	} link_create;
@@ -4057,6 +4062,9 @@ struct bpf_link_info {
 			__u32 netns_ino;
 			__u32 attach_type;
 		} netns;
+		struct {
+			__u32 ifindex;
+		} xdp;
 	};
 } __attribute__((aligned(8)));
 
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 846164c79df1..ce41ddec6229 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6915,7 +6915,8 @@ static const struct bpf_sec_def section_defs[] = {
 						BPF_XDP_DEVMAP),
 	BPF_EAPROG_SEC("xdp_cpumap/",		BPF_PROG_TYPE_XDP,
 						BPF_XDP_CPUMAP),
-	BPF_PROG_SEC("xdp",			BPF_PROG_TYPE_XDP),
+	BPF_EAPROG_SEC("xdp",			BPF_PROG_TYPE_XDP,
+						BPF_XDP),
 	BPF_PROG_SEC("perf_event",		BPF_PROG_TYPE_PERF_EVENT),
 	BPF_PROG_SEC("lwt_in",			BPF_PROG_TYPE_LWT_IN),
 	BPF_PROG_SEC("lwt_out",			BPF_PROG_TYPE_LWT_OUT),
@@ -8278,6 +8279,12 @@ bpf_program__attach_netns(struct bpf_program *prog, int netns_fd)
 	return bpf_program__attach_fd(prog, netns_fd, "netns");
 }
 
+struct bpf_link *bpf_program__attach_xdp(struct bpf_program *prog, int ifindex)
+{
+	/* target_fd/target_ifindex use the same field in LINK_CREATE */
+	return bpf_program__attach_fd(prog, ifindex, "xdp");
+}
+
 struct bpf_link *
 bpf_program__attach_iter(struct bpf_program *prog,
 			 const struct bpf_iter_attach_opts *opts)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index c2272132e929..daf33b79f760 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -257,6 +257,8 @@ LIBBPF_API struct bpf_link *
 bpf_program__attach_cgroup(struct bpf_program *prog, int cgroup_fd);
 LIBBPF_API struct bpf_link *
 bpf_program__attach_netns(struct bpf_program *prog, int netns_fd);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_xdp(struct bpf_program *prog, int ifindex);
 
 struct bpf_map;
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 6f0856abe299..ca49a6a7e5b2 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -286,6 +286,7 @@ LIBBPF_0.1.0 {
 		bpf_map__set_value_size;
 		bpf_map__type;
 		bpf_map__value_size;
+		bpf_program__attach_xdp;
 		bpf_program__autoload;
 		bpf_program__is_sk_lookup;
 		bpf_program__set_autoload;
diff --git a/tools/testing/selftests/bpf/prog_tests/section_names.c b/tools/testing/selftests/bpf/prog_tests/section_names.c
index 713167449c98..8b571890c57e 100644
--- a/tools/testing/selftests/bpf/prog_tests/section_names.c
+++ b/tools/testing/selftests/bpf/prog_tests/section_names.c
@@ -35,7 +35,7 @@ static struct sec_name_test tests[] = {
 		{-EINVAL, 0},
 	},
 	{"raw_tp/", {0, BPF_PROG_TYPE_RAW_TRACEPOINT, 0}, {-EINVAL, 0} },
-	{"xdp", {0, BPF_PROG_TYPE_XDP, 0}, {-EINVAL, 0} },
+	{"xdp", {0, BPF_PROG_TYPE_XDP, BPF_XDP}, {0, BPF_XDP} },
 	{"perf_event", {0, BPF_PROG_TYPE_PERF_EVENT, 0}, {-EINVAL, 0} },
 	{"lwt_in", {0, BPF_PROG_TYPE_LWT_IN, 0}, {-EINVAL, 0} },
 	{"lwt_out", {0, BPF_PROG_TYPE_LWT_OUT, 0}, {-EINVAL, 0} },
-- 
2.24.1

[PATCH v4 bpf-next 8/9] selftests/bpf: add BPF XDP link selftests

[Andrii Nakryiko]

Add selftest validating all the attachment logic around BPF XDP link. Test
also link updates and get_obj_info() APIs.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 .../selftests/bpf/prog_tests/xdp_link.c       | 137 ++++++++++++++++++
 .../selftests/bpf/progs/test_xdp_link.c       |  12 ++
 2 files changed, 149 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_link.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_link.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_link.c b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
new file mode 100644
index 000000000000..52cba6795d40
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020 Facebook */
+#include <uapi/linux/if_link.h>
+#include <test_progs.h>
+#include "test_xdp_link.skel.h"
+
+#define IFINDEX_LO 1
+
+void test_xdp_link(void)
+{
+	__u32 duration = 0, id1, id2, id0 = 0, prog_fd1, prog_fd2, err;
+	DECLARE_LIBBPF_OPTS(bpf_xdp_set_link_opts, opts, .old_fd = -1);
+	struct test_xdp_link *skel1 = NULL, *skel2 = NULL;
+	struct bpf_link_info link_info;
+	struct bpf_prog_info prog_info;
+	struct bpf_link *link;
+	__u32 link_info_len = sizeof(link_info);
+	__u32 prog_info_len = sizeof(prog_info);
+
+	skel1 = test_xdp_link__open_and_load();
+	if (CHECK(!skel1, "skel_load", "skeleton open and load failed\n"))
+		goto cleanup;
+	prog_fd1 = bpf_program__fd(skel1->progs.xdp_handler);
+
+	skel2 = test_xdp_link__open_and_load();
+	if (CHECK(!skel2, "skel_load", "skeleton open and load failed\n"))
+		goto cleanup;
+	prog_fd2 = bpf_program__fd(skel2->progs.xdp_handler);
+
+	memset(&prog_info, 0, sizeof(prog_info));
+	err = bpf_obj_get_info_by_fd(prog_fd1, &prog_info, &prog_info_len);
+	if (CHECK(err, "fd_info1", "failed %d\n", -errno))
+		goto cleanup;
+	id1 = prog_info.id;
+
+	memset(&prog_info, 0, sizeof(prog_info));
+	err = bpf_obj_get_info_by_fd(prog_fd2, &prog_info, &prog_info_len);
+	if (CHECK(err, "fd_info2", "failed %d\n", -errno))
+		goto cleanup;
+	id2 = prog_info.id;
+
+	/* set initial prog attachment */
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, prog_fd1, XDP_FLAGS_REPLACE, &opts);
+	if (CHECK(err, "fd_attach", "initial prog attach failed: %d\n", err))
+		goto cleanup;
+
+	/* validate prog ID */
+	err = bpf_get_link_xdp_id(IFINDEX_LO, &id0, 0);
+	CHECK(err || id0 != id1, "id1_check",
+	      "loaded prog id %u != id1 %u, err %d", id0, id1, err);
+
+	/* BPF link is not allowed to replace prog attachment */
+	link = bpf_program__attach_xdp(skel1->progs.xdp_handler, IFINDEX_LO);
+	if (CHECK(!IS_ERR(link), "link_attach_fail", "unexpected success\n")) {
+		bpf_link__destroy(link);
+		/* best-effort detach prog */
+		opts.old_fd = prog_fd1;
+		bpf_set_link_xdp_fd_opts(IFINDEX_LO, -1, XDP_FLAGS_REPLACE, &opts);
+		goto cleanup;
+	}
+
+	/* detach BPF program */
+	opts.old_fd = prog_fd1;
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, -1, XDP_FLAGS_REPLACE, &opts);
+	if (CHECK(err, "prog_detach", "failed %d\n", err))
+		goto cleanup;
+
+	/* now BPF link should attach successfully */
+	link = bpf_program__attach_xdp(skel1->progs.xdp_handler, IFINDEX_LO);
+	if (CHECK(IS_ERR(link), "link_attach", "failed: %ld\n", PTR_ERR(link)))
+		goto cleanup;
+	skel1->links.xdp_handler = link;
+
+	/* validate prog ID */
+	err = bpf_get_link_xdp_id(IFINDEX_LO, &id0, 0);
+	if (CHECK(err || id0 != id1, "id1_check",
+		  "loaded prog id %u != id1 %u, err %d", id0, id1, err))
+		goto cleanup;
+
+	/* BPF prog attach is not allowed to replace BPF link */
+	opts.old_fd = prog_fd1;
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, prog_fd2, XDP_FLAGS_REPLACE, &opts);
+	if (CHECK(!err, "prog_attach_fail", "unexpected success\n"))
+		goto cleanup;
+
+	/* Can't force-update when BPF link is active */
+	err = bpf_set_link_xdp_fd(IFINDEX_LO, prog_fd2, 0);
+	if (CHECK(!err, "prog_update_fail", "unexpected success\n"))
+		goto cleanup;
+
+	/* Can't force-detach when BPF link is active */
+	err = bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0);
+	if (CHECK(!err, "prog_detach_fail", "unexpected success\n"))
+		goto cleanup;
+
+	/* BPF link is not allowed to replace another BPF link */
+	link = bpf_program__attach_xdp(skel2->progs.xdp_handler, IFINDEX_LO);
+	if (CHECK(!IS_ERR(link), "link_attach_fail", "unexpected success\n")) {
+		bpf_link__destroy(link);
+		goto cleanup;
+	}
+
+	bpf_link__destroy(skel1->links.xdp_handler);
+	skel1->links.xdp_handler = NULL;
+
+	/* new link attach should succeed */
+	link = bpf_program__attach_xdp(skel2->progs.xdp_handler, IFINDEX_LO);
+	if (CHECK(IS_ERR(link), "link_attach", "failed: %ld\n", PTR_ERR(link)))
+		goto cleanup;
+	skel2->links.xdp_handler = link;
+
+	err = bpf_get_link_xdp_id(IFINDEX_LO, &id0, 0);
+	if (CHECK(err || id0 != id2, "id2_check",
+		  "loaded prog id %u != id2 %u, err %d", id0, id1, err))
+		goto cleanup;
+
+	/* updating program under active BPF link works as expected */
+	err = bpf_link__update_program(link, skel1->progs.xdp_handler);
+	if (CHECK(err, "link_upd", "failed: %d\n", err))
+		goto cleanup;
+
+	memset(&link_info, 0, sizeof(link_info));
+	err = bpf_obj_get_info_by_fd(bpf_link__fd(link), &link_info, &link_info_len);
+	if (CHECK(err, "link_info", "failed: %d\n", err))
+		goto cleanup;
+
+	CHECK(link_info.type != BPF_LINK_TYPE_XDP, "link_type",
+	      "got %u != exp %u\n", link_info.type, BPF_LINK_TYPE_XDP);
+	CHECK(link_info.prog_id != id1, "link_prog_id",
+	      "got %u != exp %u\n", link_info.prog_id, id1);
+	CHECK(link_info.xdp.ifindex != IFINDEX_LO, "link_ifindex",
+	      "got %u != exp %u\n", link_info.xdp.ifindex, IFINDEX_LO);
+
+cleanup:
+	test_xdp_link__destroy(skel1);
+	test_xdp_link__destroy(skel2);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_link.c b/tools/testing/selftests/bpf/progs/test_xdp_link.c
new file mode 100644
index 000000000000..eb93ea95d1d8
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_xdp_link.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020 Facebook */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+char LICENSE[] SEC("license") = "GPL";
+
+SEC("xdp/handler")
+int xdp_handler(struct xdp_md *xdp)
+{
+	return 0;
+}
-- 
2.24.1

[PATCH v4 bpf-next 9/9] bpf, xdp: remove XDP_QUERY_PROG and XDP_QUERY_PROG_HW XDP commands

[Andrii Nakryiko]

Now that BPF program/link management is centralized in generic net_device
code, kernel code never queries program id from drivers, so
XDP_QUERY_PROG/XDP_QUERY_PROG_HW commands are unnecessary.

This patch removes all the implementations of those commands in kernel, along
the xdp_attachment_query().

This patch was compile-tested on allyesconfig.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  6 -----
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  4 ----
 .../net/ethernet/cavium/thunder/nicvf_main.c  |  3 ---
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  5 ----
 drivers/net/ethernet/intel/i40e/i40e_main.c   |  3 ---
 drivers/net/ethernet/intel/ice/ice_main.c     |  3 ---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |  4 ----
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  6 -----
 drivers/net/ethernet/marvell/mvneta.c         |  5 ----
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  3 ---
 .../net/ethernet/mellanox/mlx4/en_netdev.c    | 24 -------------------
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 18 --------------
 .../ethernet/netronome/nfp/nfp_net_common.c   |  4 ----
 .../net/ethernet/qlogic/qede/qede_filter.c    |  3 ---
 drivers/net/ethernet/sfc/efx.c                |  5 ----
 drivers/net/ethernet/socionext/netsec.c       |  3 ---
 drivers/net/ethernet/ti/cpsw_priv.c           |  3 ---
 drivers/net/hyperv/netvsc_bpf.c               | 21 +---------------
 drivers/net/netdevsim/bpf.c                   |  4 ----
 drivers/net/netdevsim/netdevsim.h             |  2 +-
 drivers/net/tun.c                             | 15 ------------
 drivers/net/veth.c                            | 15 ------------
 drivers/net/virtio_net.c                      | 17 -------------
 drivers/net/xen-netfront.c                    | 21 ----------------
 include/linux/netdevice.h                     |  8 -------
 include/net/xdp.h                             |  2 --
 net/core/dev.c                                |  4 ----
 net/core/xdp.c                                |  9 -------
 28 files changed, 2 insertions(+), 218 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 91be3ffa1c5c..c1a517166016 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -576,15 +576,9 @@ static int ena_xdp_set(struct net_device *netdev, struct netdev_bpf *bpf)
  */
 static int ena_xdp(struct net_device *netdev, struct netdev_bpf *bpf)
 {
-	struct ena_adapter *adapter = netdev_priv(netdev);
-
 	switch (bpf->command) {
 	case XDP_SETUP_PROG:
 		return ena_xdp_set(netdev, bpf);
-	case XDP_QUERY_PROG:
-		bpf->prog_id = adapter->xdp_bpf_prog ?
-			adapter->xdp_bpf_prog->aux->id : 0;
-		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 5e3b4a3b69ea..2704a4709bc7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -330,10 +330,6 @@ int bnxt_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	case XDP_SETUP_PROG:
 		rc = bnxt_xdp_set(bp, xdp->prog);
 		break;
-	case XDP_QUERY_PROG:
-		xdp->prog_id = bp->xdp_prog ? bp->xdp_prog->aux->id : 0;
-		rc = 0;
-		break;
 	default:
 		rc = -EINVAL;
 		break;
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 2ba0ce115e63..1c6163934e20 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1906,9 +1906,6 @@ static int nicvf_xdp(struct net_device *netdev, struct netdev_bpf *xdp)
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return nicvf_xdp_setup(nic, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = nic->xdp_prog ? nic->xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index d0cc1dc49aaa..7d0198644c35 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -2076,14 +2076,9 @@ static int setup_xdp(struct net_device *dev, struct bpf_prog *prog)
 
 static int dpaa2_eth_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
-	struct dpaa2_eth_priv *priv = netdev_priv(dev);
-
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return setup_xdp(dev, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = priv->xdp_prog ? priv->xdp_prog->aux->id : 0;
-		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index dadbfb3d2a2b..d8315811cbdf 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12923,9 +12923,6 @@ static int i40e_xdp(struct net_device *dev,
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return i40e_xdp_setup(vsi, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = vsi->xdp_prog ? vsi->xdp_prog->aux->id : 0;
-		return 0;
 	case XDP_SETUP_XSK_UMEM:
 		return i40e_xsk_umem_setup(vsi, xdp->xsk.umem,
 					   xdp->xsk.queue_id);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index a1cef089201a..0eb1eb8229d4 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -1974,9 +1974,6 @@ static int ice_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return ice_xdp_setup_prog(vsi, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = vsi->xdp_prog ? vsi->xdp_prog->aux->id : 0;
-		return 0;
 	case XDP_SETUP_XSK_UMEM:
 		return ice_xsk_umem_setup(vsi, xdp->xsk.umem,
 					  xdp->xsk.queue_id);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 4d898ff21a46..6f32b1706ab9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -10190,10 +10190,6 @@ static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return ixgbe_xdp_setup(dev, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = adapter->xdp_prog ?
-			adapter->xdp_prog->aux->id : 0;
-		return 0;
 	case XDP_SETUP_XSK_UMEM:
 		return ixgbe_xsk_umem_setup(adapter, xdp->xsk.umem,
 					    xdp->xsk.queue_id);
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 6e9a397db583..a6267569bfa9 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -4502,15 +4502,9 @@ static int ixgbevf_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 
 static int ixgbevf_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
-	struct ixgbevf_adapter *adapter = netdev_priv(dev);
-
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return ixgbevf_xdp_setup(dev, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = adapter->xdp_prog ?
-			       adapter->xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 7191902f6cfe..044f9e80442d 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -4417,14 +4417,9 @@ static int mvneta_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
 
 static int mvneta_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
-	struct mvneta_port *pp = netdev_priv(dev);
-
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return mvneta_xdp_setup(dev, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = pp->xdp_prog ? pp->xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index 6a3f356640a0..cd5e9d60307e 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -4656,9 +4656,6 @@ static int mvpp2_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return mvpp2_xdp_setup(port, xdp);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = port->xdp_prog ? port->xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 2b8608f8f0a9..106513f772c3 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2802,35 +2802,11 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 	return err;
 }
 
-static u32 mlx4_xdp_query(struct net_device *dev)
-{
-	struct mlx4_en_priv *priv = netdev_priv(dev);
-	struct mlx4_en_dev *mdev = priv->mdev;
-	const struct bpf_prog *xdp_prog;
-	u32 prog_id = 0;
-
-	if (!priv->tx_ring_num[TX_XDP])
-		return prog_id;
-
-	mutex_lock(&mdev->state_lock);
-	xdp_prog = rcu_dereference_protected(
-		priv->rx_ring[0]->xdp_prog,
-		lockdep_is_held(&mdev->state_lock));
-	if (xdp_prog)
-		prog_id = xdp_prog->aux->id;
-	mutex_unlock(&mdev->state_lock);
-
-	return prog_id;
-}
-
 static int mlx4_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return mlx4_xdp_set(dev, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = mlx4_xdp_query(dev);
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 4e5d83f6334a..0a22459807aa 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -4517,29 +4517,11 @@ static int mlx5e_xdp_set(struct net_device *netdev, struct bpf_prog *prog)
 	return err;
 }
 
-static u32 mlx5e_xdp_query(struct net_device *dev)
-{
-	struct mlx5e_priv *priv = netdev_priv(dev);
-	const struct bpf_prog *xdp_prog;
-	u32 prog_id = 0;
-
-	mutex_lock(&priv->state_lock);
-	xdp_prog = priv->channels.params.xdp_prog;
-	if (xdp_prog)
-		prog_id = xdp_prog->aux->id;
-	mutex_unlock(&priv->state_lock);
-
-	return prog_id;
-}
-
 static int mlx5e_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return mlx5e_xdp_set(dev, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = mlx5e_xdp_query(dev);
-		return 0;
 	case XDP_SETUP_XSK_UMEM:
 		return mlx5e_xsk_setup_umem(dev, xdp->xsk.umem,
 					    xdp->xsk.queue_id);
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 44608873d3d9..39ee23e8c0bf 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -3614,10 +3614,6 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_bpf *xdp)
 		return nfp_net_xdp_setup_drv(nn, xdp);
 	case XDP_SETUP_PROG_HW:
 		return nfp_net_xdp_setup_hw(nn, xdp);
-	case XDP_QUERY_PROG:
-		return xdp_attachment_query(&nn->xdp, xdp);
-	case XDP_QUERY_PROG_HW:
-		return xdp_attachment_query(&nn->xdp_hw, xdp);
 	default:
 		return nfp_app_bpf(nn->app, nn, xdp);
 	}
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index b7d0b6ccebd3..f961f65d9372 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -1045,9 +1045,6 @@ int qede_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return qede_xdp_set(edev, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = edev->xdp_prog ? edev->xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index befd253af918..ce1a37cc7440 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -721,15 +721,10 @@ static int efx_xdp_setup_prog(struct efx_nic *efx, struct bpf_prog *prog)
 static int efx_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	struct efx_nic *efx = netdev_priv(dev);
-	struct bpf_prog *xdp_prog;
 
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return efx_xdp_setup_prog(efx, xdp->prog);
-	case XDP_QUERY_PROG:
-		xdp_prog = rtnl_dereference(efx->xdp_prog);
-		xdp->prog_id = xdp_prog ? xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 0f366cc50b74..25db667fa879 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1811,9 +1811,6 @@ static int netsec_xdp(struct net_device *ndev, struct netdev_bpf *xdp)
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return netsec_xdp_setup(priv, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = priv->xdp_prog ? priv->xdp_prog->aux->id : 0;
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/ethernet/ti/cpsw_priv.c b/drivers/net/ethernet/ti/cpsw_priv.c
index a399f3659346..d6d7a7d9c7ad 100644
--- a/drivers/net/ethernet/ti/cpsw_priv.c
+++ b/drivers/net/ethernet/ti/cpsw_priv.c
@@ -1286,9 +1286,6 @@ int cpsw_ndo_bpf(struct net_device *ndev, struct netdev_bpf *bpf)
 	case XDP_SETUP_PROG:
 		return cpsw_xdp_prog_setup(priv, bpf);
 
-	case XDP_QUERY_PROG:
-		return xdp_attachment_query(&priv->xdpi, bpf);
-
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
index 8e4141552423..440486d9c999 100644
--- a/drivers/net/hyperv/netvsc_bpf.c
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -163,16 +163,6 @@ int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog)
 	return ret;
 }
 
-static u32 netvsc_xdp_query(struct netvsc_device *nvdev)
-{
-	struct bpf_prog *prog = netvsc_xdp_get(nvdev);
-
-	if (prog)
-		return prog->aux->id;
-
-	return 0;
-}
-
 int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 {
 	struct net_device_context *ndevctx = netdev_priv(dev);
@@ -182,12 +172,7 @@ int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 	int ret;
 
 	if (!nvdev || nvdev->destroy) {
-		if (bpf->command == XDP_QUERY_PROG) {
-			bpf->prog_id = 0;
-			return 0; /* Query must always succeed */
-		} else {
-			return -ENODEV;
-		}
+		return -ENODEV;
 	}
 
 	switch (bpf->command) {
@@ -208,10 +193,6 @@ int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 
 		return ret;
 
-	case XDP_QUERY_PROG:
-		bpf->prog_id = netvsc_xdp_query(nvdev);
-		return 0;
-
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 0b362b8dac17..2e90512f3bbe 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -551,10 +551,6 @@ int nsim_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 	ASSERT_RTNL();
 
 	switch (bpf->command) {
-	case XDP_QUERY_PROG:
-		return xdp_attachment_query(&ns->xdp, bpf);
-	case XDP_QUERY_PROG_HW:
-		return xdp_attachment_query(&ns->xdp_hw, bpf);
 	case XDP_SETUP_PROG:
 		err = nsim_setup_prog_checks(ns, bpf);
 		if (err)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index d164052e0393..284f7092241d 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -121,7 +121,7 @@ static inline void nsim_bpf_uninit(struct netdevsim *ns)
 
 static inline int nsim_bpf(struct net_device *dev, struct netdev_bpf *bpf)
 {
-	return bpf->command == XDP_QUERY_PROG ? 0 : -EOPNOTSUPP;
+	return -EOPNOTSUPP;
 }
 
 static inline int nsim_bpf_disable_tc(struct netdevsim *ns)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 7adeb91bd368..061bebe25cb1 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1184,26 +1184,11 @@ static int tun_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	return 0;
 }
 
-static u32 tun_xdp_query(struct net_device *dev)
-{
-	struct tun_struct *tun = netdev_priv(dev);
-	const struct bpf_prog *xdp_prog;
-
-	xdp_prog = rtnl_dereference(tun->xdp_prog);
-	if (xdp_prog)
-		return xdp_prog->aux->id;
-
-	return 0;
-}
-
 static int tun_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return tun_xdp_set(dev, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = tun_xdp_query(dev);
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index b594f03eeddb..e56cd562a664 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1198,26 +1198,11 @@ static int veth_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	return err;
 }
 
-static u32 veth_xdp_query(struct net_device *dev)
-{
-	struct veth_priv *priv = netdev_priv(dev);
-	const struct bpf_prog *xdp_prog;
-
-	xdp_prog = priv->_xdp_prog;
-	if (xdp_prog)
-		return xdp_prog->aux->id;
-
-	return 0;
-}
-
 static int veth_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return veth_xdp_set(dev, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = veth_xdp_query(dev);
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ba38765dc490..6fa8fe5ef160 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2490,28 +2490,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	return err;
 }
 
-static u32 virtnet_xdp_query(struct net_device *dev)
-{
-	struct virtnet_info *vi = netdev_priv(dev);
-	const struct bpf_prog *xdp_prog;
-	int i;
-
-	for (i = 0; i < vi->max_queue_pairs; i++) {
-		xdp_prog = rtnl_dereference(vi->rq[i].xdp_prog);
-		if (xdp_prog)
-			return xdp_prog->aux->id;
-	}
-	return 0;
-}
-
 static int virtnet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return virtnet_xdp_set(dev, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = virtnet_xdp_query(dev);
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index ed995df19247..895a2a36e441 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1478,32 +1478,11 @@ static int xennet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	return 0;
 }
 
-static u32 xennet_xdp_query(struct net_device *dev)
-{
-	unsigned int num_queues = dev->real_num_tx_queues;
-	struct netfront_info *np = netdev_priv(dev);
-	const struct bpf_prog *xdp_prog;
-	struct netfront_queue *queue;
-	unsigned int i;
-
-	for (i = 0; i < num_queues; ++i) {
-		queue = &np->queues[i];
-		xdp_prog = rtnl_dereference(queue->xdp_prog);
-		if (xdp_prog)
-			return xdp_prog->aux->id;
-	}
-
-	return 0;
-}
-
 static int xennet_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		return xennet_xdp_set(dev, xdp->prog, xdp->extack);
-	case XDP_QUERY_PROG:
-		xdp->prog_id = xennet_xdp_query(dev);
-		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7d3c412fcfe5..1046763cd0dc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -876,8 +876,6 @@ enum bpf_netdev_command {
 	 */
 	XDP_SETUP_PROG,
 	XDP_SETUP_PROG_HW,
-	XDP_QUERY_PROG,
-	XDP_QUERY_PROG_HW,
 	/* BPF program for offload callbacks, invoked at program load time. */
 	BPF_OFFLOAD_MAP_ALLOC,
 	BPF_OFFLOAD_MAP_FREE,
@@ -911,12 +909,6 @@ struct netdev_bpf {
 			struct bpf_prog *prog;
 			struct netlink_ext_ack *extack;
 		};
-		/* XDP_QUERY_PROG, XDP_QUERY_PROG_HW */
-		struct {
-			u32 prog_id;
-			/* flags with which program was installed */
-			u32 prog_flags;
-		};
 		/* BPF_OFFLOAD_MAP_ALLOC, BPF_OFFLOAD_MAP_FREE */
 		struct {
 			struct bpf_offloaded_map *offmap;
diff --git a/include/net/xdp.h b/include/net/xdp.h
index 5be0d4d65b94..627680b35e31 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -224,8 +224,6 @@ struct xdp_attachment_info {
 };
 
 struct netdev_bpf;
-int xdp_attachment_query(struct xdp_attachment_info *info,
-			 struct netdev_bpf *bpf);
 bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
 			     struct netdev_bpf *bpf);
 void xdp_attachment_setup(struct xdp_attachment_info *info,
diff --git a/net/core/dev.c b/net/core/dev.c
index 9859dae7d4dc..183965cac45b 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5468,10 +5468,6 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
 		}
 		break;
 
-	case XDP_QUERY_PROG:
-		xdp->prog_id = old ? old->aux->id : 0;
-		break;
-
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 3c45f99e26d5..48aba933a5a8 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -400,15 +400,6 @@ void __xdp_release_frame(void *data, struct xdp_mem_info *mem)
 }
 EXPORT_SYMBOL_GPL(__xdp_release_frame);
 
-int xdp_attachment_query(struct xdp_attachment_info *info,
-			 struct netdev_bpf *bpf)
-{
-	bpf->prog_id = info->prog ? info->prog->aux->id : 0;
-	bpf->prog_flags = info->prog ? info->flags : 0;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(xdp_attachment_query);
-
 bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
 			     struct netdev_bpf *bpf)
 {
-- 
2.24.1

Re: [PATCH v4 bpf-next 0/9] BPF XDP link

[Alexei Starovoitov]

On Tue, Jul 21, 2020 at 11:46 PM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Following cgroup and netns examples, implement bpf_link support for XDP.
>
> The semantics is described in patch #2. Program and link attachments are
> mutually exclusive, in the sense that neither link can replace attached
> program nor program can replace attached link. Link can't replace attached
> link as well, as is the case for any other bpf_link implementation.
>
> Patch #1 refactors existing BPF program-based attachment API and centralizes
> high-level query/attach decisions in generic kernel code, while drivers are
> kept simple and are instructed with low-level decisions about attaching and
> detaching specific bpf_prog. This also makes QUERY command unnecessary, and
> patch #8 removes support for it from all kernel drivers. If that's a bad idea,
> we can drop that patch altogether.
>
> With refactoring in patch #1, adding bpf_xdp_link is completely transparent to
> drivers, they are still functioning at the level of "effective" bpf_prog, that
> should be called in XDP data path.
>
> Corresponding libbpf support for BPF XDP link is added in patch #5.
>
> v3->v4:
> - fix a compilation warning in one of drivers (Jakub);

As far as I could review everything looks fine,
so I've applied the set.
The code is delicate. I wish people were more active doing code reviews.
So I encourage folks to still look at it. If there is anything missing I'm sure
Andrii will fix it up in the follow up.