net: Add support for XDP in egress path

From: David Ahern <[email protected]>

Add new bpf_attach_type, BPF_XDP_EGRESS, for BPF programs attached at the XDP layer, but the egress path.

Since egress path will not have ingress_ifindex and rx_queue_index set, update xdp_is_valid_access to block access to these entries in the xdp context when a program is attached to egress path.

Update dev_change_xdp_fd to verify expected_attach_type for a program is BPF_XDP_EGRESS if egress argument is set.

The next patch adds support for the egress ifindex.

Signed-off-by Prashant Bhole <[email protected]>

Signed-off-by David Ahern <[email protected]>

---
 include/uapi/linux/bpf.h       |  1 +
 net/core/dev.c                 | 11 +++++++++++
 net/core/filter.c              |  8 ++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 4 files changed, 21 insertions(+)

include/uapi/linux/bpf.h +1 -0

index 2e29a671d67e..a9d384998e8b 100644
@@ -215,6 +215,7 @@ enum bpf_attach_type {
 	BPF_TRACE_FEXIT,
 	BPF_MODIFY_RETURN,
 	BPF_LSM_MAC,
+	BPF_XDP_EGRESS,
 	__MAX_BPF_ATTACH_TYPE
 };
 

net/core/dev.c +11 -0

index 97180458e7cb..e8a62bdb395b 100644
@@ -8732,6 +8732,17 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 		if (IS_ERR(prog))
 			return PTR_ERR(prog);
 
+		if (egress && prog->expected_attach_type != BPF_XDP_EGRESS) {
+			NL_SET_ERR_MSG(extack, "XDP program in Tx path must use BPF_XDP_EGRESS attach type");
+			bpf_prog_put(prog);
+			return -EINVAL;
+		}
+		if (!egress && prog->expected_attach_type == BPF_XDP_EGRESS) {
+			NL_SET_ERR_MSG(extack, "XDP program in Rx path can not use BPF_XDP_EGRESS attach type");
+			bpf_prog_put(prog);
+			return -EINVAL;
+		}
+
 		if (!offload && bpf_prog_is_dev_bound(prog->aux)) {
 			NL_SET_ERR_MSG(extack, "using device-bound program without HW_MODE flag is not supported");
 			bpf_prog_put(prog);

net/core/filter.c +0 -0

index 7d6ceaa54d21..bcb56448f336 100644

From: David Ahern <[email protected]>

Running programs in the egress path, on skbs or xdp_frames, does not require driver specific resources like Rx path. Accordingly, the programs can be run in core code, so add xdp_egress_prog to net_device to hold a reference to an attached program.

For UAPI, add IFLA_XDP_EGRESS to if_link.h to specify egress programs, add a new attach flag, XDP_ATTACHED_EGRESS_CORE, to denote the attach point is at the core level (as opposed to driver or hardware) and add IFLA_XDP_EGRESS_CORE_PROG_ID for reporting the program id.

Add egress argument to do_setlink_xdp to denote processing of IFLA_XDP_EGRESS versus IFLA_XDP, and add a check that none of the existing modes (SKB, DRV or HW) are set since those modes are not valid. The expectation is that XDP_FLAGS_HW_MODE will be used later (e.g., offloading guest programs).

Add rtnl_xdp_egress_fill and helpers as the egress counterpart to the existing rtnl_xdp_fill.

Signed-off-by David Ahern <[email protected]>

---
 include/linux/netdevice.h          |  1 +
 include/uapi/linux/if_link.h       |  3 +
 net/core/rtnetlink.c               | 96 ++++++++++++++++++++++++++++--
 tools/include/uapi/linux/if_link.h |  3 +
 4 files changed, 99 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -0

index d0bb9e09660a..3133247681fd 100644
@@ -1995,6 +1995,7 @@ struct net_device {
 	unsigned int		real_num_rx_queues;
 
 	struct bpf_prog __rcu	*xdp_prog;
+	struct bpf_prog __rcu	*xdp_egress_prog;
 	unsigned long		gro_flush_timeout;
 	rx_handler_func_t __rcu	*rx_handler;
 	void __rcu		*rx_handler_data;

include/uapi/linux/if_link.h +3 -0

index 127c704eeba9..b3c6cb2f0f0a 100644
@@ -170,6 +170,7 @@ enum {
 	IFLA_PROP_LIST,
 	IFLA_ALT_IFNAME, /* Alternative ifname */
 	IFLA_PERM_ADDRESS,
+	IFLA_XDP_EGRESS, /* nested attribute with 1 or more IFLA_XDP_ attrs */
 	__IFLA_MAX
 };
 

@@ -988,6 +989,7 @@ enum {
 	XDP_ATTACHED_SKB,
 	XDP_ATTACHED_HW,
 	XDP_ATTACHED_MULTI,
+	XDP_ATTACHED_EGRESS_CORE,
 };
 
 enum {

@@ -1000,6 +1002,7 @@ enum {
 	IFLA_XDP_SKB_PROG_ID,
 	IFLA_XDP_HW_PROG_ID,
 	IFLA_XDP_EXPECTED_FD,
+	IFLA_XDP_EGRESS_CORE_PROG_ID,
 	__IFLA_XDP_MAX,
 };
 

net/core/rtnetlink.c +85 -4

index dc44af16226a..e9bc5cee06c8 100644
@@ -1030,7 +1030,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_PORT_ID */
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
-	       + rtnl_xdp_size() /* IFLA_XDP */
+	       + rtnl_xdp_size() * 2 /* IFLA_XDP and IFLA_XDP_EGRESS */
 	       + nla_total_size(4)  /* IFLA_EVENT */
 	       + nla_total_size(4)  /* IFLA_NEW_NETNSID */
 	       + nla_total_size(4)  /* IFLA_NEW_IFINDEX */

@@ -1395,6 +1395,42 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static u32 rtnl_xdp_egress_prog(struct net_device *dev)
+{
+	const struct bpf_prog *prog;
+
+	ASSERT_RTNL();
+
+	prog = rtnl_dereference(dev->xdp_egress_prog);
+	if (!prog)
+		return 0;
+	return prog->aux->id;
+}
+
+static int rtnl_xdp_egress_report(struct sk_buff *skb, struct net_device *dev,
+				  u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
+				  u32 (*get_prog_id)(struct net_device *dev))
+{
+	u32 curr_id;
+	int err;
+
+	curr_id = get_prog_id(dev);
+	if (!curr_id)
+		return 0;
+
+	*prog_id = curr_id;
+	err = nla_put_u32(skb, attr, curr_id);
+	if (err)
+		return err;
+
+	if (*mode != XDP_ATTACHED_NONE)
+		*mode = XDP_ATTACHED_MULTI;
+	else
+		*mode = tgt_mode;
+
+	return 0;
+}
+
 static u32 rtnl_xdp_prog_skb(struct net_device *dev)
 {
 	const struct bpf_prog *generic_xdp_prog;

@@ -1486,6 +1522,42 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
 	return err;
 }
 
+static int rtnl_xdp_egress_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	u8 mode = XDP_ATTACHED_NONE;
+	struct nlattr *xdp;
+	u32 prog_id = 0;
+	int err;
+
+	xdp = nla_nest_start_noflag(skb, IFLA_XDP_EGRESS);
+	if (!xdp)
+		return -EMSGSIZE;
+
+	err = rtnl_xdp_egress_report(skb, dev, &prog_id, &mode,
+				     XDP_ATTACHED_EGRESS_CORE,
+				     IFLA_XDP_EGRESS_CORE_PROG_ID,
+				     rtnl_xdp_egress_prog);
+	if (err)
+		goto err_cancel;
+
+	err = nla_put_u8(skb, IFLA_XDP_ATTACHED, mode);
+	if (err)
+		goto err_cancel;
+
+	if (prog_id && mode != XDP_ATTACHED_MULTI) {
+		err = nla_put_u32(skb, IFLA_XDP_PROG_ID, prog_id);
+		if (err)
+			goto err_cancel;
+	}
+
+	nla_nest_end(skb, xdp);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, xdp);
+	return err;
+}
+
 static u32 rtnl_get_event(unsigned long event)
 {
 	u32 rtnl_event_type = IFLA_EVENT_NONE;

@@ -1743,6 +1815,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 	if (rtnl_xdp_fill(skb, dev))
 		goto nla_put_failure;
 
+	if (rtnl_xdp_egress_fill(skb, dev))
+		goto nla_put_failure;
+
 	if (dev->rtnl_link_ops || rtnl_have_link_slave_info(dev)) {
 		if (rtnl_link_fill(skb, dev) < 0)
 			goto nla_put_failure;

@@ -1827,6 +1902,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_ALT_IFNAME]	= { .type = NLA_STRING,
 				    .len = ALTIFNAMSIZ - 1 },
 	[IFLA_PERM_ADDRESS]	= { .type = NLA_REJECT },
+	[IFLA_XDP_EGRESS]	= { .type = NLA_NESTED },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {

@@ -2482,7 +2558,8 @@ static int do_set_master(struct net_device *dev, int ifindex,
 #define DO_SETLINK_NOTIFY	0x03
 
 static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
-			  int *status, struct netlink_ext_ack *extack)
+			  int *status, bool egress,
+			  struct netlink_ext_ack *extack)
 {
 	struct nlattr *xdp[IFLA_XDP_MAX + 1];
 	u32 xdp_flags = 0;

@@ -2498,6 +2575,10 @@ static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
 
 	if (xdp[IFLA_XDP_FLAGS]) {
 		xdp_flags = nla_get_u32(xdp[IFLA_XDP_FLAGS]);
+		if (egress && xdp_flags & XDP_FLAGS_MODES) {
+			NL_SET_ERR_MSG(extack, "XDP_FLAGS_MODES not valid for egress");
+			goto out_einval;
+		}
 		if (xdp_flags & ~XDP_FLAGS_MASK)
 			goto out_einval;
 		if (hweight32(xdp_flags & XDP_FLAGS_MODES) > 1)

@@ -2515,7 +2596,7 @@ static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
 
 		err = dev_change_xdp_fd(dev, extack,
 					nla_get_s32(xdp[IFLA_XDP_FD]),
-					expected_fd, xdp_flags, false);
+					expected_fd, xdp_flags, egress);
 		if (err)
 			return err;
 

@@ -2821,7 +2902,14 @@ static int do_setlink(const struct sk_buff *skb,
 	}
 
 	if (tb[IFLA_XDP]) {
-		err = do_setlink_xdp(dev, tb[IFLA_XDP], &status, extack);
+		err = do_setlink_xdp(dev, tb[IFLA_XDP], &status, false, extack);

From: David Ahern <[email protected]>

Patch adds egress XDP support in libbpf.

New section name hint, xdp_egress, is added to set expected attach type at program load. Programs can use xdp_egress as the prefix in the SEC statement to load the program with the BPF_XDP_EGRESS attach type set.

egress is added to bpf_xdp_set_link_opts to specify egress type for use with bpf_set_link_xdp_fd_opts. Update library side to check for flag and set nla_type to IFLA_XDP_EGRESS.

Add egress version of bpf_get_link_xdp* info and id apis with core code refactored to handle both rx and tx paths.

Signed-off-by Prashant Bhole <[email protected]>

Co-developed-by David Ahern <[email protected]>

Signed-off-by David Ahern <[email protected]>

---
 tools/lib/bpf/libbpf.c   |  2 ++
 tools/lib/bpf/libbpf.h   |  9 +++++-
 tools/lib/bpf/libbpf.map |  2 ++
 tools/lib/bpf/netlink.c  | 63 +++++++++++++++++++++++++++++++++++-----
 4 files changed, 67 insertions(+), 9 deletions(-)

tools/lib/bpf/libbpf.c +2 -0

index 8f480e29a6b0..32fc970495d9 100644
@@ -6366,6 +6366,8 @@ static const struct bpf_sec_def section_defs[] = {
 		.is_attach_btf = true,
 		.expected_attach_type = BPF_LSM_MAC,
 		.attach_fn = attach_lsm),
+	BPF_EAPROG_SEC("xdp_egress",		BPF_PROG_TYPE_XDP,
+						BPF_XDP_EGRESS),
 	BPF_PROG_SEC("xdp",			BPF_PROG_TYPE_XDP),
 	BPF_PROG_SEC("perf_event",		BPF_PROG_TYPE_PERF_EVENT),
 	BPF_PROG_SEC("lwt_in",			BPF_PROG_TYPE_LWT_IN),

tools/lib/bpf/libbpf.h +3 -1

index f1dacecb1619..3feb1242f78e 100644
@@ -453,14 +453,16 @@ struct xdp_link_info {
 	__u32 drv_prog_id;
 	__u32 hw_prog_id;
 	__u32 skb_prog_id;
+	__u32 egress_core_prog_id;
 	__u8 attach_mode;
 };
 
 struct bpf_xdp_set_link_opts {
 	size_t sz;
 	int old_fd;
+	__u8  egress;
 };
-#define bpf_xdp_set_link_opts__last_field old_fd
+#define bpf_xdp_set_link_opts__last_field egress
 
 LIBBPF_API int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
 LIBBPF_API int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,

As I pointed out on the RFC patch, I'm concerned whether this will work right with freplace programs attaching to XDP programs. It may just be that I'm missing something, but in that case please explain why it works? :)

Quoted 229 lines · David Ahern

From: David Ahern <[email protected]>

Running programs in the egress path, on skbs or xdp_frames, does not require driver specific resources like Rx path. Accordingly, the programs can be run in core code, so add xdp_egress_prog to net_device to hold a reference to an attached program.

For UAPI, add IFLA_XDP_EGRESS to if_link.h to specify egress programs, add a new attach flag, XDP_ATTACHED_EGRESS_CORE, to denote the attach point is at the core level (as opposed to driver or hardware) and add IFLA_XDP_EGRESS_CORE_PROG_ID for reporting the program id.

Add egress argument to do_setlink_xdp to denote processing of IFLA_XDP_EGRESS versus IFLA_XDP, and add a check that none of the existing modes (SKB, DRV or HW) are set since those modes are not valid. The expectation is that XDP_FLAGS_HW_MODE will be used later (e.g., offloading guest programs).

Add rtnl_xdp_egress_fill and helpers as the egress counterpart to the existing rtnl_xdp_fill.

Signed-off-by David Ahern <[email protected]>

---
 include/linux/netdevice.h          |  1 +
 include/uapi/linux/if_link.h       |  3 +
 net/core/rtnetlink.c               | 96 ++++++++++++++++++++++++++++--
 tools/include/uapi/linux/if_link.h |  3 +
 4 files changed, 99 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -0

index d0bb9e09660a..3133247681fd 100644
@@ -1995,6 +1995,7 @@ struct net_device {
 	unsigned int		real_num_rx_queues;
 
 	struct bpf_prog __rcu	*xdp_prog;
+	struct bpf_prog __rcu	*xdp_egress_prog;
 	unsigned long		gro_flush_timeout;
 	rx_handler_func_t __rcu	*rx_handler;
 	void __rcu		*rx_handler_data;

include/uapi/linux/if_link.h +3 -0

index 127c704eeba9..b3c6cb2f0f0a 100644
@@ -170,6 +170,7 @@ enum {
 	IFLA_PROP_LIST,
 	IFLA_ALT_IFNAME, /* Alternative ifname */
 	IFLA_PERM_ADDRESS,
+	IFLA_XDP_EGRESS, /* nested attribute with 1 or more IFLA_XDP_ attrs */
 	__IFLA_MAX
 };
 

@@ -988,6 +989,7 @@ enum {
 	XDP_ATTACHED_SKB,
 	XDP_ATTACHED_HW,
 	XDP_ATTACHED_MULTI,
+	XDP_ATTACHED_EGRESS_CORE,
 };
 
 enum {

@@ -1000,6 +1002,7 @@ enum {
 	IFLA_XDP_SKB_PROG_ID,
 	IFLA_XDP_HW_PROG_ID,
 	IFLA_XDP_EXPECTED_FD,
+	IFLA_XDP_EGRESS_CORE_PROG_ID,
 	__IFLA_XDP_MAX,
 };
 

net/core/rtnetlink.c +92 -4

index dc44af16226a..e9bc5cee06c8 100644
@@ -1030,7 +1030,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_PORT_ID */
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
-	       + rtnl_xdp_size() /* IFLA_XDP */
+	       + rtnl_xdp_size() * 2 /* IFLA_XDP and IFLA_XDP_EGRESS */
 	       + nla_total_size(4)  /* IFLA_EVENT */
 	       + nla_total_size(4)  /* IFLA_NEW_NETNSID */
 	       + nla_total_size(4)  /* IFLA_NEW_IFINDEX */

@@ -1395,6 +1395,42 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static u32 rtnl_xdp_egress_prog(struct net_device *dev)
+{
+	const struct bpf_prog *prog;
+
+	ASSERT_RTNL();
+
+	prog = rtnl_dereference(dev->xdp_egress_prog);
+	if (!prog)
+		return 0;
+	return prog->aux->id;
+}
+
+static int rtnl_xdp_egress_report(struct sk_buff *skb, struct net_device *dev,
+				  u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
+				  u32 (*get_prog_id)(struct net_device *dev))
+{
+	u32 curr_id;
+	int err;
+
+	curr_id = get_prog_id(dev);
+	if (!curr_id)
+		return 0;
+
+	*prog_id = curr_id;
+	err = nla_put_u32(skb, attr, curr_id);
+	if (err)
+		return err;
+
+	if (*mode != XDP_ATTACHED_NONE)
+		*mode = XDP_ATTACHED_MULTI;
+	else
+		*mode = tgt_mode;
+
+	return 0;
+}
+
 static u32 rtnl_xdp_prog_skb(struct net_device *dev)
 {
 	const struct bpf_prog *generic_xdp_prog;

@@ -1486,6 +1522,42 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
 	return err;
 }
 
+static int rtnl_xdp_egress_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	u8 mode = XDP_ATTACHED_NONE;
+	struct nlattr *xdp;
+	u32 prog_id = 0;
+	int err;
+
+	xdp = nla_nest_start_noflag(skb, IFLA_XDP_EGRESS);
+	if (!xdp)
+		return -EMSGSIZE;
+
+	err = rtnl_xdp_egress_report(skb, dev, &prog_id, &mode,
+				     XDP_ATTACHED_EGRESS_CORE,
+				     IFLA_XDP_EGRESS_CORE_PROG_ID,
+				     rtnl_xdp_egress_prog);
+	if (err)
+		goto err_cancel;
+
+	err = nla_put_u8(skb, IFLA_XDP_ATTACHED, mode);
+	if (err)
+		goto err_cancel;
+
+	if (prog_id && mode != XDP_ATTACHED_MULTI) {
+		err = nla_put_u32(skb, IFLA_XDP_PROG_ID, prog_id);
+		if (err)
+			goto err_cancel;
+	}
+
+	nla_nest_end(skb, xdp);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, xdp);
+	return err;
+}
+
 static u32 rtnl_get_event(unsigned long event)
 {
 	u32 rtnl_event_type = IFLA_EVENT_NONE;

@@ -1743,6 +1815,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 	if (rtnl_xdp_fill(skb, dev))
 		goto nla_put_failure;
 
+	if (rtnl_xdp_egress_fill(skb, dev))
+		goto nla_put_failure;
+
 	if (dev->rtnl_link_ops || rtnl_have_link_slave_info(dev)) {
 		if (rtnl_link_fill(skb, dev) < 0)
 			goto nla_put_failure;

@@ -1827,6 +1902,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_ALT_IFNAME]	= { .type = NLA_STRING,
 				    .len = ALTIFNAMSIZ - 1 },
 	[IFLA_PERM_ADDRESS]	= { .type = NLA_REJECT },
+	[IFLA_XDP_EGRESS]	= { .type = NLA_NESTED },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {

@@ -2482,7 +2558,8 @@ static int do_set_master(struct net_device *dev, int ifindex,
 #define DO_SETLINK_NOTIFY	0x03
 
 static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
-			  int *status, struct netlink_ext_ack *extack)
+			  int *status, bool egress,
+			  struct netlink_ext_ack *extack)
 {
 	struct nlattr *xdp[IFLA_XDP_MAX + 1];
 	u32 xdp_flags = 0;

@@ -2498,6 +2575,10 @@ static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
 
 	if (xdp[IFLA_XDP_FLAGS]) {
 		xdp_flags = nla_get_u32(xdp[IFLA_XDP_FLAGS]);
+		if (egress && xdp_flags & XDP_FLAGS_MODES) {
+			NL_SET_ERR_MSG(extack, "XDP_FLAGS_MODES not valid for egress");
+			goto out_einval;
+		}
 		if (xdp_flags & ~XDP_FLAGS_MASK)
 			goto out_einval;
 		if (hweight32(xdp_flags & XDP_FLAGS_MODES) > 1)

@@ -2515,7 +2596,7 @@ static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
 
 		err = dev_change_xdp_fd(dev, extack,
 					nla_get_s32(xdp[IFLA_XDP_FD]),
-					expected_fd, xdp_flags, false);
+					expected_fd, xdp_flags, egress);
 		if (err)
 			return err;
 

@@ -2821,7 +2902,14 @@ static int do_setlink(const struct sk_buff *skb,
 	}
 
 	if (tb[IFLA_XDP]) {
-		err = do_setlink_xdp(dev, tb[IFLA_XDP], &status, extack);
+		err = do_setlink_xdp(dev, tb[IFLA_XDP], &status, false, extack);
+		if (err)
+			goto errout;
+	}
+
+	if (tb[IFLA_XDP_EGRESS]) {
+		err = do_setlink_xdp(dev, tb[IFLA_XDP_EGRESS], &status, true,
+				     extack);

net/core/rtnetlink.c static int do_setlink(const struct sk_buff *skb, · line 2915

Quoted 3 lines

net/core/rtnetlink.c +0 -0

 		if (err)
 			goto errout;
 	}

This means that IFLA_XDP and IFLA_XDP_EGRESS can be present in the same netlink message, right? But then installation of the RX program could succeed, but userspace would still get an error if the egress program installation fails? That is probably not good?

Since I don't think we can atomically make sure both operations fail or succeed, maybe it's better to disallow both entries being present in the same netlink message?

Isn't the kernel returning both program types in the same message when dumping an interface? So do we really need a separate getter instead of just populating xdp_link_info with the egress ID in the existing getter?

Quoted 4 lines · Toke Høiland-Jørgensen

As I pointed out on the RFC patch, I'm concerned whether this will work right with freplace programs attaching to XDP programs. It may just be that I'm missing something, but in that case please explain why it works? :)

expected_attach_type is not unique to XDP. freplace is not unique to XDP. IF there is a problem, it is not unique to XDP, and any enhancements needed to freplace functionality will not be unique to XDP.

Quoted 231 lines · Toke Høiland-Jørgensen

Quoted 229 lines · David Ahern

From: David Ahern <[email protected]>

Running programs in the egress path, on skbs or xdp_frames, does not require driver specific resources like Rx path. Accordingly, the programs can be run in core code, so add xdp_egress_prog to net_device to hold a reference to an attached program.

For UAPI, add IFLA_XDP_EGRESS to if_link.h to specify egress programs, add a new attach flag, XDP_ATTACHED_EGRESS_CORE, to denote the attach point is at the core level (as opposed to driver or hardware) and add IFLA_XDP_EGRESS_CORE_PROG_ID for reporting the program id.

Add egress argument to do_setlink_xdp to denote processing of IFLA_XDP_EGRESS versus IFLA_XDP, and add a check that none of the existing modes (SKB, DRV or HW) are set since those modes are not valid. The expectation is that XDP_FLAGS_HW_MODE will be used later (e.g., offloading guest programs).

Add rtnl_xdp_egress_fill and helpers as the egress counterpart to the existing rtnl_xdp_fill.

Signed-off-by David Ahern <[email protected]>

---
 include/linux/netdevice.h          |  1 +
 include/uapi/linux/if_link.h       |  3 +
 net/core/rtnetlink.c               | 96 ++++++++++++++++++++++++++++--
 tools/include/uapi/linux/if_link.h |  3 +
 4 files changed, 99 insertions(+), 4 deletions(-)

include/linux/netdevice.h +1 -0

index d0bb9e09660a..3133247681fd 100644
@@ -1995,6 +1995,7 @@ struct net_device {
 	unsigned int		real_num_rx_queues;
 
 	struct bpf_prog __rcu	*xdp_prog;
+	struct bpf_prog __rcu	*xdp_egress_prog;
 	unsigned long		gro_flush_timeout;
 	rx_handler_func_t __rcu	*rx_handler;
 	void __rcu		*rx_handler_data;

include/uapi/linux/if_link.h +3 -0

index 127c704eeba9..b3c6cb2f0f0a 100644
@@ -170,6 +170,7 @@ enum {
 	IFLA_PROP_LIST,
 	IFLA_ALT_IFNAME, /* Alternative ifname */
 	IFLA_PERM_ADDRESS,
+	IFLA_XDP_EGRESS, /* nested attribute with 1 or more IFLA_XDP_ attrs */
 	__IFLA_MAX
 };
 

@@ -988,6 +989,7 @@ enum {
 	XDP_ATTACHED_SKB,
 	XDP_ATTACHED_HW,
 	XDP_ATTACHED_MULTI,
+	XDP_ATTACHED_EGRESS_CORE,
 };
 
 enum {

@@ -1000,6 +1002,7 @@ enum {
 	IFLA_XDP_SKB_PROG_ID,
 	IFLA_XDP_HW_PROG_ID,
 	IFLA_XDP_EXPECTED_FD,
+	IFLA_XDP_EGRESS_CORE_PROG_ID,
 	__IFLA_XDP_MAX,
 };
 

net/core/rtnetlink.c +92 -4

index dc44af16226a..e9bc5cee06c8 100644
@@ -1030,7 +1030,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_PORT_ID */
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
-	       + rtnl_xdp_size() /* IFLA_XDP */
+	       + rtnl_xdp_size() * 2 /* IFLA_XDP and IFLA_XDP_EGRESS */
 	       + nla_total_size(4)  /* IFLA_EVENT */
 	       + nla_total_size(4)  /* IFLA_NEW_NETNSID */
 	       + nla_total_size(4)  /* IFLA_NEW_IFINDEX */

@@ -1395,6 +1395,42 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
 	return 0;
 }
 
+static u32 rtnl_xdp_egress_prog(struct net_device *dev)
+{
+	const struct bpf_prog *prog;
+
+	ASSERT_RTNL();
+
+	prog = rtnl_dereference(dev->xdp_egress_prog);
+	if (!prog)
+		return 0;
+	return prog->aux->id;
+}
+
+static int rtnl_xdp_egress_report(struct sk_buff *skb, struct net_device *dev,
+				  u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
+				  u32 (*get_prog_id)(struct net_device *dev))
+{
+	u32 curr_id;
+	int err;
+
+	curr_id = get_prog_id(dev);
+	if (!curr_id)
+		return 0;
+
+	*prog_id = curr_id;
+	err = nla_put_u32(skb, attr, curr_id);
+	if (err)
+		return err;
+
+	if (*mode != XDP_ATTACHED_NONE)
+		*mode = XDP_ATTACHED_MULTI;
+	else
+		*mode = tgt_mode;
+
+	return 0;
+}
+
 static u32 rtnl_xdp_prog_skb(struct net_device *dev)
 {
 	const struct bpf_prog *generic_xdp_prog;

@@ -1486,6 +1522,42 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
 	return err;
 }
 
+static int rtnl_xdp_egress_fill(struct sk_buff *skb, struct net_device *dev)
+{
+	u8 mode = XDP_ATTACHED_NONE;
+	struct nlattr *xdp;
+	u32 prog_id = 0;
+	int err;
+
+	xdp = nla_nest_start_noflag(skb, IFLA_XDP_EGRESS);
+	if (!xdp)
+		return -EMSGSIZE;
+
+	err = rtnl_xdp_egress_report(skb, dev, &prog_id, &mode,
+				     XDP_ATTACHED_EGRESS_CORE,
+				     IFLA_XDP_EGRESS_CORE_PROG_ID,
+				     rtnl_xdp_egress_prog);
+	if (err)
+		goto err_cancel;
+
+	err = nla_put_u8(skb, IFLA_XDP_ATTACHED, mode);
+	if (err)
+		goto err_cancel;
+
+	if (prog_id && mode != XDP_ATTACHED_MULTI) {
+		err = nla_put_u32(skb, IFLA_XDP_PROG_ID, prog_id);
+		if (err)
+			goto err_cancel;
+	}
+
+	nla_nest_end(skb, xdp);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, xdp);
+	return err;
+}
+
 static u32 rtnl_get_event(unsigned long event)
 {
 	u32 rtnl_event_type = IFLA_EVENT_NONE;

@@ -1743,6 +1815,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
 	if (rtnl_xdp_fill(skb, dev))
 		goto nla_put_failure;
 
+	if (rtnl_xdp_egress_fill(skb, dev))
+		goto nla_put_failure;
+
 	if (dev->rtnl_link_ops || rtnl_have_link_slave_info(dev)) {
 		if (rtnl_link_fill(skb, dev) < 0)
 			goto nla_put_failure;

@@ -1827,6 +1902,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_ALT_IFNAME]	= { .type = NLA_STRING,
 				    .len = ALTIFNAMSIZ - 1 },
 	[IFLA_PERM_ADDRESS]	= { .type = NLA_REJECT },
+	[IFLA_XDP_EGRESS]	= { .type = NLA_NESTED },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {

@@ -2482,7 +2558,8 @@ static int do_set_master(struct net_device *dev, int ifindex,
 #define DO_SETLINK_NOTIFY	0x03
 
 static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
-			  int *status, struct netlink_ext_ack *extack)
+			  int *status, bool egress,
+			  struct netlink_ext_ack *extack)
 {
 	struct nlattr *xdp[IFLA_XDP_MAX + 1];
 	u32 xdp_flags = 0;

@@ -2498,6 +2575,10 @@ static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
 
 	if (xdp[IFLA_XDP_FLAGS]) {
 		xdp_flags = nla_get_u32(xdp[IFLA_XDP_FLAGS]);
+		if (egress && xdp_flags & XDP_FLAGS_MODES) {
+			NL_SET_ERR_MSG(extack, "XDP_FLAGS_MODES not valid for egress");
+			goto out_einval;
+		}
 		if (xdp_flags & ~XDP_FLAGS_MASK)
 			goto out_einval;
 		if (hweight32(xdp_flags & XDP_FLAGS_MODES) > 1)

@@ -2515,7 +2596,7 @@ static int do_setlink_xdp(struct net_device *dev, struct nlattr *tb,
 
 		err = dev_change_xdp_fd(dev, extack,
 					nla_get_s32(xdp[IFLA_XDP_FD]),
-					expected_fd, xdp_flags, false);
+					expected_fd, xdp_flags, egress);
 		if (err)
 			return err;
 

@@ -2821,7 +2902,14 @@ static int do_setlink(const struct sk_buff *skb,
 	}
 
 	if (tb[IFLA_XDP]) {
-		err = do_setlink_xdp(dev, tb[IFLA_XDP], &status, extack);
+		err = do_setlink_xdp(dev, tb[IFLA_XDP], &status, false, extack);
+		if (err)
+			goto errout;
+	}
+
+	if (tb[IFLA_XDP_EGRESS]) {
+		err = do_setlink_xdp(dev, tb[IFLA_XDP_EGRESS], &status, true,
+				     extack);

Quoted 8 lines

net/core/rtnetlink.c static int do_setlink(const struct sk_buff *skb, · line 2915

Quoted 3 lines

net/core/rtnetlink.c +0 -0

 		if (err)
 			goto errout;
 	}

This means that IFLA_XDP and IFLA_XDP_EGRESS can be present in the same netlink message, right? But then installation of the RX program could succeed, but userspace would still get an error if the egress program installation fails? That is probably not good?

That's a good catch.

Quoted 1 line

Quoted 4 lines

Since I don't think we can atomically make sure both operations fail or succeed, maybe it's better to disallow both entries being present in the same netlink message?

I think so since there is no way to undo the setlink for IFLA_XDP if IFLA_XDP_EGRESS fails - exisitng program references are gone.

Although, existing do_setlink leaves the device in an incomplete state on any failure...

Quoted 3 lines · Toke Høiland-Jørgensen

Isn't the kernel returning both program types in the same message when dumping an interface? So do we really need a separate getter instead of just populating xdp_link_info with the egress ID in the existing getter?

That might work with some refactoring of get_xdp_info. I'll take a look.

Quoted 6 lines · David Ahern

Quoted 4 lines · Toke Høiland-Jørgensen

As I pointed out on the RFC patch, I'm concerned whether this will work right with freplace programs attaching to XDP programs. It may just be that I'm missing something, but in that case please explain why it works? :)

Quoted 3 lines

expected_attach_type is not unique to XDP. freplace is not unique to XDP. IF there is a problem, it is not unique to XDP, and any enhancements needed to freplace functionality will not be unique to XDP.

Still needs to be fixed, though :)

Also, at least looking through all the is_valid_access functions in filter.c, they all seem to "fail safe". I.e., specific expected_attach_type values can permit the program access to additional ranges. In which case an freplace program that doesn't have the right attach type will just be rejected if it tries to access such a field. Whereas here you're *disallowing* something based on a particular expected_attach_type, so you can end up with an egress program that should have been rejected by the verifier but isn't because it's missing the attach_type.

Quoted 8 lines · Toke Høiland-Jørgensen

Quoted 6 lines · David Ahern

Quoted 4 lines · Toke Høiland-Jørgensen

As I pointed out on the RFC patch, I'm concerned whether this will work right with freplace programs attaching to XDP programs. It may just be that I'm missing something, but in that case please explain why it works? :)

Quoted 5 lines

Quoted 3 lines

expected_attach_type is not unique to XDP. freplace is not unique to XDP. IF there is a problem, it is not unique to XDP, and any enhancements needed to freplace functionality will not be unique to XDP.

Still needs to be fixed, though :)

one problem at a time. I have a long list of items that are directly relevant to what I want to do.

Quoted 9 lines

Also, at least looking through all the is_valid_access functions in filter.c, they all seem to "fail safe". I.e., specific expected_attach_type values can permit the program access to additional ranges. In which case an freplace program that doesn't have the right attach type will just be rejected if it tries to access such a field. Whereas here you're *disallowing* something based on a particular expected_attach_type, so you can end up with an egress program that should have been rejected by the verifier but isn't because it's missing the attach_type.

There are 6 existing valid access checks on expected_attach_type doing the exact same thing - validating access based on attach type.

Not saying a fix to freplace *has* to be part of this series; just saying that I would be more comfortable if that was fixed before we merge this.

Quoted 3 lines · Toke Høiland-Jørgensen

Not saying a fix to freplace *has* to be part of this series; just saying that I would be more comfortable if that was fixed before we merge this.

You don't have a test case that fails, you just think there is a problem based on code analysis. Have you taken this presumption up with the author of the freplace code?

And as I said in the beginning, I'm perfectly happy to be told why I'm wrong; but so far you have just been arguing that I'm out of scope ;)

Quoted 2 lines · Toke Høiland-Jørgensen

And as I said in the beginning, I'm perfectly happy to be told why I'm wrong; but so far you have just been arguing that I'm out of scope ;)

you are arguing about a suspected bug with existing code that is no way touched or modified by this patch set, so yes it is out of scope.

The proper way to discuss a bug in existing code is a thread focused on that topic, not buried inside comments on a patch set.

Your patch is relying on the (potentially buggy) behaviour, so I don't think it's out of scope to mention it in this context.

Quoted 6 lines · David Ahern

Quoted 2 lines · Toke Høiland-Jørgensen

And as I said in the beginning, I'm perfectly happy to be told why I'm wrong; but so far you have just been arguing that I'm out of scope ;)

you are arguing about a suspected bug with existing code that is no way touched or modified by this patch set, so yes it is out of scope.

Your patch is relying on the (potentially buggy) behaviour, so I don't think it's out of scope to mention it in this context.

From: David Ahern <[email protected]>

Patch adds egress XDP support in libbpf.

New section name hint, xdp_egress, is added to set expected attach type at program load. Programs can use xdp_egress as the prefix in the SEC statement to load the program with the BPF_XDP_EGRESS attach type set.

egress is added to bpf_xdp_set_link_opts to specify egress type for use with bpf_set_link_xdp_fd_opts. Update library side to check for flag and set nla_type to IFLA_XDP_EGRESS.

Add egress version of bpf_get_link_xdp* info and id apis with core code refactored to handle both rx and tx paths.

Signed-off-by Prashant Bhole <[email protected]>

Co-developed-by David Ahern <[email protected]>

Signed-off-by David Ahern <[email protected]>

---
 tools/lib/bpf/libbpf.c   |  2 ++
 tools/lib/bpf/libbpf.h   |  9 +++++-
 tools/lib/bpf/libbpf.map |  2 ++
 tools/lib/bpf/netlink.c  | 63 +++++++++++++++++++++++++++++++++++-----
 4 files changed, 67 insertions(+), 9 deletions(-)

tools/lib/bpf/libbpf.c +2 -0

index 8f480e29a6b0..32fc970495d9 100644
@@ -6366,6 +6366,8 @@ static const struct bpf_sec_def section_defs[] = {
                .is_attach_btf = true,
                .expected_attach_type = BPF_LSM_MAC,
                .attach_fn = attach_lsm),
+       BPF_EAPROG_SEC("xdp_egress",            BPF_PROG_TYPE_XDP,
+                                               BPF_XDP_EGRESS),
        BPF_PROG_SEC("xdp",                     BPF_PROG_TYPE_XDP),
        BPF_PROG_SEC("perf_event",              BPF_PROG_TYPE_PERF_EVENT),
        BPF_PROG_SEC("lwt_in",                  BPF_PROG_TYPE_LWT_IN),

tools/lib/bpf/libbpf.h +0 -0

index f1dacecb1619..3feb1242f78e 100644

tools/lib/bpf/libbpf.h +6 -1

 };
-#define bpf_xdp_set_link_opts__last_field old_fd
+#define bpf_xdp_set_link_opts__last_field egress
 
 LIBBPF_API int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags);
 LIBBPF_API int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,

@@ -468,6 +470,11 @@ LIBBPF_API int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,
 LIBBPF_API int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags);
 LIBBPF_API int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
                                     size_t info_size, __u32 flags);
+LIBBPF_API int bpf_get_link_xdp_egress_id(int ifindex, __u32 *prog_id,
+                                         __u32 flags);
+LIBBPF_API int bpf_get_link_xdp_egress_info(int ifindex,
+                                           struct xdp_link_info *info,
+                                           size_t info_size, __u32 flags);
 
 struct perf_buffer;
 

tools/lib/bpf/libbpf.map +0 -0

index bb8831605b25..51576c8a02fe 100644

tools/lib/bpf/libbpf.map +0 -0

 } LIBBPF_0.0.7;

tools/lib/bpf/libbpf.map +2 -1

+                                            nla_type);
 }
 
 int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
 {
-       return __bpf_set_link_xdp_fd_replace(ifindex, fd, 0, flags);
+       return __bpf_set_link_xdp_fd_replace(ifindex, fd, 0, flags, IFLA_XDP);
 }
 

tools/lib/bpf/libbpf.map +14 -0

+int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
+                         size_t info_size, __u32 flags)
+{
+       return __bpf_get_link_xdp_info(ifindex, info, info_size, flags,
+                                      IFLA_XDP);
+}
+
+int bpf_get_link_xdp_egress_info(int ifindex, struct xdp_link_info *info,
+                                size_t info_size, __u32 flags)
+{
+       return __bpf_get_link_xdp_info(ifindex, info, info_size, flags,
+                                      IFLA_XDP_EGRESS);
+}
+
 static __u32 get_xdp_id(struct xdp_link_info *info, __u32 flags)
 {
        if (info->attach_mode != XDP_ATTACHED_MULTI && !flags)

tools/lib/bpf/libbpf.map +15 -1

+{
+       struct xdp_link_info info;
+       int ret;
+
+       /* egress path does not support SKB, DRV or HW mode */
+       if (flags & XDP_FLAGS_MODES)
+               return -EINVAL;
+
+       ret = bpf_get_link_xdp_egress_info(ifindex, &info, sizeof(info), flags);
+       if (!ret)
+               *prog_id = get_xdp_id(&info, flags);
+
+       return ret;
+}
+
 int libbpf_nl_get_link(int sock, unsigned int nl_pid,
                       libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
 {
--

2.21.1 (Apple Git-122.3)

Quoted 22 lines

tools/lib/bpf/netlink.c +14 -0

+int bpf_get_link_xdp_info(int ifindex, struct xdp_link_info *info,
+                         size_t info_size, __u32 flags)
+{
+       return __bpf_get_link_xdp_info(ifindex, info, info_size, flags,
+                                      IFLA_XDP);
+}
+
+int bpf_get_link_xdp_egress_info(int ifindex, struct xdp_link_info *info,
+                                size_t info_size, __u32 flags)
+{
+       return __bpf_get_link_xdp_info(ifindex, info, info_size, flags,
+                                      IFLA_XDP_EGRESS);
+}
+
 static __u32 get_xdp_id(struct xdp_link_info *info, __u32 flags)
 {
        if (info->attach_mode != XDP_ATTACHED_MULTI && !flags)

@@ -345,6 +376,22 @@ int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags)
        return ret;
 }
 

tools/lib/bpf/netlink.c int bpf_get_link_xdp_id(int ifindex, __u32 *prog_id, __u32 flags) · line 379

Quoted 1 line

tools/lib/bpf/netlink.c +1 -0

+int bpf_get_link_xdp_egress_id(int ifindex, __u32 *prog_id, __u32 flags)

Is bpf_get_link_xdp_egress_id() even needed? This is a special case of bpf_get_link_xdp_egress_info(), I don't think we have to add another API to support it specifically.

Also, just curious, would it be better to have a generalized XDP/XDP_EGRESS xdp_info() functions instead of two separate ones?

From: David Ahern <[email protected]>

Add NET_ATTACH_TYPE_XDP_EGRESS and update attach_type_strings to allow a user to specify 'xdp_egress' as the attach or detach point.

libbpf handles egress config via bpf_set_link_xdp_fd_opts, so update do_attach_detach_xdp to use it. Specifically, the new API requires old_fd to be set based on any currently loaded program, so use bpf_get_link_xdp_id and bpf_get_link_xdp_egress_id to get an fd to any existing program.

Update 'net show' command to dump egress programs.

Signed-off-by David Ahern <[email protected]>

---

Quoted 2 lines · Toke Høiland-Jørgensen

Your patch is relying on the (potentially buggy) behaviour, so I don't think it's out of scope to mention it in this context.

This is getting ridiculous. I am in no way, shape, or form relying on freplace.

I guess we'll see tomorrow the outcome of your investigations.

Quoted 11 lines · Toke Høiland-Jørgensen

Quoted 6 lines · David Ahern

Quoted 2 lines · Toke Høiland-Jørgensen

And as I said in the beginning, I'm perfectly happy to be told why I'm wrong; but so far you have just been arguing that I'm out of scope ;)

you are arguing about a suspected bug with existing code that is no way touched or modified by this patch set, so yes it is out of scope.

Your patch is relying on the (potentially buggy) behaviour, so I don't think it's out of scope to mention it in this context.

Sorry for slow reply.
I'm swamped with other things atm.

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

A selftest to prove or disprove is necessary, of course.

Thanks Toke for bringing it to my attention.

Quoted 16 lines · Alexei Starovoitov

Quoted 11 lines · Toke Høiland-Jørgensen

Quoted 6 lines · David Ahern

Quoted 2 lines · Toke Høiland-Jørgensen

And as I said in the beginning, I'm perfectly happy to be told why I'm wrong; but so far you have just been arguing that I'm out of scope ;)

you are arguing about a suspected bug with existing code that is no way touched or modified by this patch set, so yes it is out of scope.

Your patch is relying on the (potentially buggy) behaviour, so I don't think it's out of scope to mention it in this context.

Sorry for slow reply.
I'm swamped with other things atm.

Quoted 9 lines

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

Not quite: the check in bpf_tracing_prog_attach() that enforces prog->expected_attach_type==0 also needs to go. So 5 lines :)

Quoted 29 lines · Toke Høiland-Jørgensen

Quoted 25 lines · Alexei Starovoitov

Quoted 11 lines · Toke Høiland-Jørgensen

Quoted 6 lines · David Ahern

Quoted 2 lines · Toke Høiland-Jørgensen

And as I said in the beginning, I'm perfectly happy to be told why I'm wrong; but so far you have just been arguing that I'm out of scope ;)

you are arguing about a suspected bug with existing code that is no way touched or modified by this patch set, so yes it is out of scope.

Your patch is relying on the (potentially buggy) behaviour, so I don't think it's out of scope to mention it in this context.

Sorry for slow reply.
I'm swamped with other things atm.

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

Quoted 2 lines

Not quite: the check in bpf_tracing_prog_attach() that enforces prog->expected_attach_type==0 also needs to go. So 5 lines :)

prog_ext's expected_attach_type needs to stay zero. It needs to be inherited from tgt prog. Hence one line:
prog->expected_attach_type = tgt_prog->expected_attach_type;

Thanks for the bpftool update. Can you please also update the man pages (bpftool-prog, bpftool-net) and bash completion?

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

Quoted 6 lines

Quoted 2 lines

Not quite: the check in bpf_tracing_prog_attach() that enforces prog->expected_attach_type==0 also needs to go. So 5 lines :)

prog_ext's expected_attach_type needs to stay zero. It needs to be inherited from tgt prog. Hence one line:
prog->expected_attach_type = tgt_prog->expected_attach_type;

Not sure I follow you here? I ended up with the patch below - without the first hunk I can't attach freplace funcs to an xdp egress prog (since the expected_attach_type will have been propagated from verification time), and so that check will fail. Or am I missing something?

(quoted diff) +0 -1

-Toke

kernel/bpf/syscall.c +0 -0

index d85f37239540..40c3103c7233 100644

Quoted 33 lines · Toke Høiland-Jørgensen

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

Quoted 6 lines

Quoted 2 lines

Not quite: the check in bpf_tracing_prog_attach() that enforces prog->expected_attach_type==0 also needs to go. So 5 lines :)

prog_ext's expected_attach_type needs to stay zero. It needs to be inherited from tgt prog. Hence one line:
prog->expected_attach_type = tgt_prog->expected_attach_type;

Not sure I follow you here? I ended up with the patch below - without the first hunk I can't attach freplace funcs to an xdp egress prog (since the expected_attach_type will have been propagated from verification time), and so that check will fail. Or am I missing something?

(quoted diff) +0 -1

-Toke

kernel/bpf/syscall.c +0 -0

index d85f37239540..40c3103c7233 100644

kernel/bpf/syscall.c static int bpf_tracing_prog_attach(struct bpf_prog *prog) · line 2384

Quoted 8 lines

kernel/bpf/syscall.c +0 -4

@@ -2381,10 +2381,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                }
                break;
        case BPF_PROG_TYPE_EXT:
-               if (prog->expected_attach_type != 0) {
-                       err = -EINVAL;
-                       goto out_put_prog;
-               }
                break;

ahh. that extra check.
I think it's better to keep it for extra safety.
Here all expected_attach_type have clear checks depending on prog_type. There is no other place where it's that obvious.
The verifier does similar thing earlier, but it's not that clear. I think the better fix would to set expected_attach_type = 0 for PROG_TYPE_EXT at the end of do_check, since we're overriding this field temporarily during verification.

Quoted 43 lines · Alexei Starovoitov

Quoted 33 lines · Toke Høiland-Jørgensen

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

Quoted 6 lines

Quoted 2 lines

Not quite: the check in bpf_tracing_prog_attach() that enforces prog->expected_attach_type==0 also needs to go. So 5 lines :)

prog_ext's expected_attach_type needs to stay zero. It needs to be inherited from tgt prog. Hence one line:
prog->expected_attach_type = tgt_prog->expected_attach_type;

Not sure I follow you here? I ended up with the patch below - without the first hunk I can't attach freplace funcs to an xdp egress prog (since the expected_attach_type will have been propagated from verification time), and so that check will fail. Or am I missing something?

(quoted diff) +0 -1

-Toke

kernel/bpf/syscall.c +0 -0

index d85f37239540..40c3103c7233 100644

kernel/bpf/syscall.c static int bpf_tracing_prog_attach(struct bpf_prog *prog) · line 2384

Quoted 8 lines

kernel/bpf/syscall.c +0 -4

@@ -2381,10 +2381,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                }
                break;
        case BPF_PROG_TYPE_EXT:
-               if (prog->expected_attach_type != 0) {
-                       err = -EINVAL;
-                       goto out_put_prog;
-               }
                break;

Quoted 8 lines

ahh. that extra check.
I think it's better to keep it for extra safety.
Here all expected_attach_type have clear checks depending on prog_type. There is no other place where it's that obvious.
The verifier does similar thing earlier, but it's not that clear. I think the better fix would to set expected_attach_type = 0 for PROG_TYPE_EXT at the end of do_check, since we're overriding this field temporarily during verification.

OK, sure, can do. I do agree it's better to keep the check. I'll send a proper patch tomorrow, then.

As far as a adding a selftest for this, I think the most natural thing would be to add it on top of David's tests for xdp_egress, since that's what hit this - would you be OK with that? And if so, should I send the main patch straight away and hold off on the selftest, or should I split them, or hold off on the whole thing?

I think the issue is not related to xdp egress.

Hence I'd like to push the fix along with selftest into bpf tree. The selftest can be:
void noinline do_bind((struct bpf_sock_addr *ctx)
{
struct sockaddr_in sa = {};

bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)); return 0;
}
SEC("cgroup/connect4")
int connect_v4_prog(struct bpf_sock_addr *ctx) {
return do_bind(ctx);
}

and freplace would replace do_bind() with do_new_bind()
that also calls bpf_bind().
I think without the fix freplace will fail to load, because availability of bpf_bind() depends on correct prog->expected_attach_type.

I haven't looked at the crash you mentioned in the other email related to xdp egress set. That could be different issue. I hope it's the same thing :)

Quoted 54 lines · Toke Høiland-Jørgensen

Quoted 43 lines · Alexei Starovoitov

Quoted 33 lines · Toke Høiland-Jørgensen

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Looks like there is indeed a bug in prog_type_ext handling code that is doing
env->ops = bpf_verifier_ops[tgt_prog->type];
I'm not sure whether the verifier can simply add:
prog->expected_attach_type = tgt_prog->expected_attach_type; and be done with it.
Likely yes, since expected_attach_type must be zero at that point that is enforced by bpf_prog_load_check_attach().
So I suspect it's a single line fix.

Quoted 6 lines

Quoted 2 lines

Not quite: the check in bpf_tracing_prog_attach() that enforces prog->expected_attach_type==0 also needs to go. So 5 lines :)

prog_ext's expected_attach_type needs to stay zero. It needs to be inherited from tgt prog. Hence one line:
prog->expected_attach_type = tgt_prog->expected_attach_type;

Not sure I follow you here? I ended up with the patch below - without the first hunk I can't attach freplace funcs to an xdp egress prog (since the expected_attach_type will have been propagated from verification time), and so that check will fail. Or am I missing something?

(quoted diff) +0 -1

-Toke

kernel/bpf/syscall.c +0 -0

index d85f37239540..40c3103c7233 100644

kernel/bpf/syscall.c static int bpf_tracing_prog_attach(struct bpf_prog *prog) · line 2384

Quoted 8 lines

kernel/bpf/syscall.c +0 -4

@@ -2381,10 +2381,6 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)
                }
                break;
        case BPF_PROG_TYPE_EXT:
-               if (prog->expected_attach_type != 0) {
-                       err = -EINVAL;
-                       goto out_put_prog;
-               }
                break;

Quoted 8 lines

ahh. that extra check.
I think it's better to keep it for extra safety.
Here all expected_attach_type have clear checks depending on prog_type. There is no other place where it's that obvious.
The verifier does similar thing earlier, but it's not that clear. I think the better fix would to set expected_attach_type = 0 for PROG_TYPE_EXT at the end of do_check, since we're overriding this field temporarily during verification.

Quoted 8 lines

OK, sure, can do. I do agree it's better to keep the check. I'll send a proper patch tomorrow, then.

As far as a adding a selftest for this, I think the most natural thing would be to add it on top of David's tests for xdp_egress, since that's what hit this - would you be OK with that? And if so, should I send the main patch straight away and hold off on the selftest, or should I split them, or hold off on the whole thing?

I think the issue is not related to xdp egress.
Hence I'd like to push the fix along with selftest into bpf tree. The selftest can be:
void noinline do_bind((struct bpf_sock_addr *ctx)
{
struct sockaddr_in sa = {};

bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)); return 0;
}
SEC("cgroup/connect4")
int connect_v4_prog(struct bpf_sock_addr *ctx) {
return do_bind(ctx);
}

and freplace would replace do_bind() with do_new_bind() that also calls bpf_bind().
I think without the fix freplace will fail to load, because availability of bpf_bind() depends on correct prog->expected_attach_type.

I haven't looked at the crash you mentioned in the other email related to xdp egress set. That could be different issue. I hope it's the same thing :)