bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v3 0/4] XDP: Support atomic replacement of XDP interface attachments
@ 2020-03-24 18:12 Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-24 18:12 UTC (permalink / raw)
  To: netdev
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

This series adds support for atomically replacing the XDP program loaded on an
interface. This is achieved by means of a new netlink attribute that can specify
the expected previous program to replace on the interface. If set, the kernel
will compare this "expected id" attribute with the program currently loaded on
the interface, and reject the operation if it does not match.

With this primitive, userspace applications can avoid stepping on each other's
toes when simultaneously updating the loaded XDP program.

Changelog:

v3:
- Pass existing ID instead of FD (Jakub)
- Use opts struct for new libbpf function (Andrii)

v2:
- Fix checkpatch nits and add .strict_start_type to netlink policy (Jakub)

---

Toke Høiland-Jørgensen (4):
      xdp: Support specifying expected existing program when attaching XDP
      tools: Add EXPECTED_ID-related definitions in if_link.h
      libbpf: Add function to set link XDP fd while specifying old program
      selftests/bpf: Add tests for attaching XDP programs


 include/linux/netdevice.h                          |  2 +-
 include/uapi/linux/if_link.h                       |  4 +-
 net/core/dev.c                                     | 14 ++--
 net/core/rtnetlink.c                               | 13 ++++
 tools/include/uapi/linux/if_link.h                 |  4 +-
 tools/lib/bpf/libbpf.h                             |  8 +++
 tools/lib/bpf/libbpf.map                           |  1 +
 tools/lib/bpf/netlink.c                            | 34 +++++++++-
 .../testing/selftests/bpf/prog_tests/xdp_attach.c  | 74 ++++++++++++++++++++++
 9 files changed, 145 insertions(+), 9 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_attach.c


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-24 18:12 [PATCH bpf-next v3 0/4] XDP: Support atomic replacement of XDP interface attachments Toke Høiland-Jørgensen
@ 2020-03-24 18:12 ` Toke Høiland-Jørgensen
  2020-03-24 19:40   ` Jakub Kicinski
                     ` (2 more replies)
  2020-03-24 18:12 ` [PATCH bpf-next v3 2/4] tools: Add EXPECTED_ID-related definitions in if_link.h Toke Høiland-Jørgensen
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-24 18:12 UTC (permalink / raw)
  To: netdev
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

From: Toke Høiland-Jørgensen <toke@redhat.com>

While it is currently possible for userspace to specify that an existing
XDP program should not be replaced when attaching to an interface, there is
no mechanism to safely replace a specific XDP program with another.

This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
set along with IFLA_XDP_FD. If set, the kernel will check that the program
currently loaded on the interface matches the expected one, and fail the
operation if it does not. This corresponds to a 'cmpxchg' memory operation.
Setting the new attribute with a negative value means that no program is
expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
flag.

A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
request checking of the EXPECTED_ID attribute. This is needed for userspace
to discover whether the kernel supports the new attribute.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/linux/netdevice.h    |    2 +-
 include/uapi/linux/if_link.h |    4 +++-
 net/core/dev.c               |   14 +++++++++-----
 net/core/rtnetlink.c         |   13 +++++++++++++
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 654808bfad83..a14199ea9501 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3768,7 +3768,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 
 typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
 int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
-		      int fd, u32 flags);
+		      int fd, u32 expected_id, u32 flags);
 u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
 		    enum bpf_netdev_command cmd);
 int xdp_umem_query(struct net_device *dev, u16 queue_id);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 61e0801c82df..7182569773f9 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -972,11 +972,12 @@ enum {
 #define XDP_FLAGS_SKB_MODE		(1U << 1)
 #define XDP_FLAGS_DRV_MODE		(1U << 2)
 #define XDP_FLAGS_HW_MODE		(1U << 3)
+#define XDP_FLAGS_EXPECT_ID		(1U << 4)
 #define XDP_FLAGS_MODES			(XDP_FLAGS_SKB_MODE | \
 					 XDP_FLAGS_DRV_MODE | \
 					 XDP_FLAGS_HW_MODE)
 #define XDP_FLAGS_MASK			(XDP_FLAGS_UPDATE_IF_NOEXIST | \
-					 XDP_FLAGS_MODES)
+					 XDP_FLAGS_MODES | XDP_FLAGS_EXPECT_ID)
 
 /* These are stored into IFLA_XDP_ATTACHED on dump. */
 enum {
@@ -996,6 +997,7 @@ enum {
 	IFLA_XDP_DRV_PROG_ID,
 	IFLA_XDP_SKB_PROG_ID,
 	IFLA_XDP_HW_PROG_ID,
+	IFLA_XDP_EXPECTED_ID,
 	__IFLA_XDP_MAX,
 };
 
diff --git a/net/core/dev.c b/net/core/dev.c
index d84541c24446..37db06d8074f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8655,18 +8655,20 @@ static void dev_xdp_uninstall(struct net_device *dev)
  *	@dev: device
  *	@extack: netlink extended ack
  *	@fd: new program fd or negative value to clear
+ *	@expected_id: ID of old program 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, u32 flags)
+		      int fd, u32 expected_id, u32 flags)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	enum bpf_netdev_command query;
 	struct bpf_prog *prog = NULL;
 	bpf_op_t bpf_op, bpf_chk;
 	bool offload;
+	u32 prog_id;
 	int err;
 
 	ASSERT_RTNL();
@@ -8684,15 +8686,17 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 	if (bpf_op == bpf_chk)
 		bpf_chk = generic_xdp_install;
 
+	prog_id = __dev_xdp_query(dev, bpf_op, query);
+	if (flags & XDP_FLAGS_EXPECT_ID && prog_id != expected_id) {
+		NL_SET_ERR_MSG(extack, "Active program does not match expected");
+		return -EEXIST;
+	}
 	if (fd >= 0) {
-		u32 prog_id;
-
 		if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
 			NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time");
 			return -EEXIST;
 		}
 
-		prog_id = __dev_xdp_query(dev, bpf_op, query);
 		if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && prog_id) {
 			NL_SET_ERR_MSG(extack, "XDP program already attached");
 			return -EBUSY;
@@ -8715,7 +8719,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 			return 0;
 		}
 	} else {
-		if (!__dev_xdp_query(dev, bpf_op, query))
+		if (!prog_id)
 			return 0;
 	}
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 14e6ea21c378..dd6d4d85b284 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1872,7 +1872,9 @@ static const struct nla_policy ifla_port_policy[IFLA_PORT_MAX+1] = {
 };
 
 static const struct nla_policy ifla_xdp_policy[IFLA_XDP_MAX + 1] = {
+	[IFLA_XDP_UNSPEC]	= { .strict_start_type = IFLA_XDP_EXPECTED_ID },
 	[IFLA_XDP_FD]		= { .type = NLA_S32 },
+	[IFLA_XDP_EXPECTED_ID]	= { .type = NLA_U32 },
 	[IFLA_XDP_ATTACHED]	= { .type = NLA_U8 },
 	[IFLA_XDP_FLAGS]	= { .type = NLA_U32 },
 	[IFLA_XDP_PROG_ID]	= { .type = NLA_U32 },
@@ -2799,8 +2801,19 @@ static int do_setlink(const struct sk_buff *skb,
 		}
 
 		if (xdp[IFLA_XDP_FD]) {
+			u32 expected_id = 0;
+
+			if (xdp_flags & XDP_FLAGS_EXPECT_ID) {
+				if (!xdp[IFLA_XDP_EXPECTED_ID]) {
+					err = -EINVAL;
+					goto errout;
+				}
+				expected_id = nla_get_u32(xdp[IFLA_XDP_EXPECTED_ID]);
+			}
+
 			err = dev_change_xdp_fd(dev, extack,
 						nla_get_s32(xdp[IFLA_XDP_FD]),
+						expected_id,
 						xdp_flags);
 			if (err)
 				goto errout;


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf-next v3 2/4] tools: Add EXPECTED_ID-related definitions in if_link.h
  2020-03-24 18:12 [PATCH bpf-next v3 0/4] XDP: Support atomic replacement of XDP interface attachments Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
@ 2020-03-24 18:12 ` Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 3/4] libbpf: Add function to set link XDP fd while specifying old program Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 4/4] selftests/bpf: Add tests for attaching XDP programs Toke Høiland-Jørgensen
  3 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-24 18:12 UTC (permalink / raw)
  To: netdev
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

From: Toke Høiland-Jørgensen <toke@redhat.com>

This adds the IFLA_XDP_EXPECTED_ID netlink attribute definition and the
XDP_FLAGS_EXPECT_ID flag to if_link.h in tools/include.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/include/uapi/linux/if_link.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h
index 024af2d1d0af..a6c14ae083e3 100644
--- a/tools/include/uapi/linux/if_link.h
+++ b/tools/include/uapi/linux/if_link.h
@@ -960,11 +960,12 @@ enum {
 #define XDP_FLAGS_SKB_MODE		(1U << 1)
 #define XDP_FLAGS_DRV_MODE		(1U << 2)
 #define XDP_FLAGS_HW_MODE		(1U << 3)
+#define XDP_FLAGS_EXPECT_ID		(1U << 4)
 #define XDP_FLAGS_MODES			(XDP_FLAGS_SKB_MODE | \
 					 XDP_FLAGS_DRV_MODE | \
 					 XDP_FLAGS_HW_MODE)
 #define XDP_FLAGS_MASK			(XDP_FLAGS_UPDATE_IF_NOEXIST | \
-					 XDP_FLAGS_MODES)
+					 XDP_FLAGS_MODES | XDP_FLAGS_EXPECT_ID)
 
 /* These are stored into IFLA_XDP_ATTACHED on dump. */
 enum {
@@ -984,6 +985,7 @@ enum {
 	IFLA_XDP_DRV_PROG_ID,
 	IFLA_XDP_SKB_PROG_ID,
 	IFLA_XDP_HW_PROG_ID,
+	IFLA_XDP_EXPECTED_ID,
 	__IFLA_XDP_MAX,
 };
 


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf-next v3 3/4] libbpf: Add function to set link XDP fd while specifying old program
  2020-03-24 18:12 [PATCH bpf-next v3 0/4] XDP: Support atomic replacement of XDP interface attachments Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 2/4] tools: Add EXPECTED_ID-related definitions in if_link.h Toke Høiland-Jørgensen
@ 2020-03-24 18:12 ` Toke Høiland-Jørgensen
  2020-03-24 18:12 ` [PATCH bpf-next v3 4/4] selftests/bpf: Add tests for attaching XDP programs Toke Høiland-Jørgensen
  3 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-24 18:12 UTC (permalink / raw)
  To: netdev
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

From: Toke Høiland-Jørgensen <toke@redhat.com>

This adds a new function to set the XDP fd while specifying the ID of the
program to replace, using the newly added IFLA_XDP_EXPECTED_ID netlink
parameter. The new function uses the opts struct mechanism to be extendable
in the future.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.h   |    8 ++++++++
 tools/lib/bpf/libbpf.map |    1 +
 tools/lib/bpf/netlink.c  |   34 +++++++++++++++++++++++++++++++++-
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index d38d7a629417..2d77bc28b518 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -444,7 +444,15 @@ struct xdp_link_info {
 	__u8 attach_mode;
 };
 
+struct bpf_xdp_set_link_opts {
+	size_t sz;
+	__u32 old_id;
+};
+#define bpf_xdp_set_link_opts__last_field old_id
+
 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,
+					const struct bpf_xdp_set_link_opts *opts);
 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);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 5129283c0284..dcc87db3ca8a 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -244,4 +244,5 @@ LIBBPF_0.0.8 {
 		bpf_link__pin_path;
 		bpf_link__unpin;
 		bpf_program__set_attach_target;
+		bpf_set_link_xdp_fd_opts;
 } LIBBPF_0.0.7;
diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
index 431bd25c6cdb..2ae0cf1956f2 100644
--- a/tools/lib/bpf/netlink.c
+++ b/tools/lib/bpf/netlink.c
@@ -132,7 +132,8 @@ static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq,
 	return ret;
 }
 
-int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
+static int __bpf_set_link_xdp_fd_replace(int ifindex, int fd, __u32 old_id,
+					 __u32 flags)
 {
 	int sock, seq = 0, ret;
 	struct nlattr *nla, *nla_xdp;
@@ -178,6 +179,14 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
 		nla->nla_len += nla_xdp->nla_len;
 	}
 
+	if (flags & XDP_FLAGS_EXPECT_ID) {
+		nla_xdp = (struct nlattr *)((char *)nla + nla->nla_len);
+		nla_xdp->nla_type = IFLA_XDP_EXPECTED_ID;
+		nla_xdp->nla_len = NLA_HDRLEN + sizeof(old_id);
+		memcpy((char *)nla_xdp + NLA_HDRLEN, &old_id, sizeof(old_id));
+		nla->nla_len += nla_xdp->nla_len;
+	}
+
 	req.nh.nlmsg_len += NLA_ALIGN(nla->nla_len);
 
 	if (send(sock, &req, req.nh.nlmsg_len, 0) < 0) {
@@ -191,6 +200,29 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
 	return ret;
 }
 
+int bpf_set_link_xdp_fd_opts(int ifindex, int fd, __u32 flags,
+			     const struct bpf_xdp_set_link_opts *opts)
+{
+	__u32 old_id;
+
+	if (!OPTS_VALID(opts, bpf_xdp_set_link_opts))
+		return -EINVAL;
+
+	old_id = OPTS_GET(opts, old_id, 0);
+
+	if (old_id)
+		flags |=  XDP_FLAGS_EXPECT_ID;
+
+	return __bpf_set_link_xdp_fd_replace(ifindex, fd,
+					     old_id,
+					     flags);
+}
+
+int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
+{
+	return __bpf_set_link_xdp_fd_replace(ifindex, fd, 0, flags);
+}
+
 static int __dump_link_nlmsg(struct nlmsghdr *nlh,
 			     libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
 {


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH bpf-next v3 4/4] selftests/bpf: Add tests for attaching XDP programs
  2020-03-24 18:12 [PATCH bpf-next v3 0/4] XDP: Support atomic replacement of XDP interface attachments Toke Høiland-Jørgensen
                   ` (2 preceding siblings ...)
  2020-03-24 18:12 ` [PATCH bpf-next v3 3/4] libbpf: Add function to set link XDP fd while specifying old program Toke Høiland-Jørgensen
@ 2020-03-24 18:12 ` Toke Høiland-Jørgensen
  3 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-24 18:12 UTC (permalink / raw)
  To: netdev
  Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
	Song Liu, Yonghong Song, Andrii Nakryiko, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

From: Toke Høiland-Jørgensen <toke@redhat.com>

This adds tests for the various replacement operations using
IFLA_XDP_EXPECTED_ID.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 .../testing/selftests/bpf/prog_tests/xdp_attach.c  |   74 ++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_attach.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
new file mode 100644
index 000000000000..190df7599107
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+#define IFINDEX_LO 1
+#define XDP_FLAGS_EXPECT_ID		(1U << 4)
+
+void test_xdp_attach(void)
+{
+	struct bpf_object *obj1, *obj2, *obj3;
+	const char *file = "./test_xdp.o";
+	struct bpf_prog_info info = {};
+	__u32 duration = 0, id1, id2;
+	__u32 len = sizeof(info);
+	int err, fd1, fd2, fd3;
+	DECLARE_LIBBPF_OPTS(bpf_xdp_set_link_opts, opts);
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj1, &fd1);
+	if (CHECK_FAIL(err))
+		return;
+	err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj2, &fd2);
+	if (CHECK_FAIL(err))
+		goto out_1;
+	err = bpf_prog_load(file, BPF_PROG_TYPE_XDP, &obj3, &fd3);
+	if (CHECK_FAIL(err))
+		goto out_2;
+
+	err = bpf_obj_get_info_by_fd(fd1, &info, &len);
+	if (CHECK_FAIL(err))
+		goto out_2;
+	id1 = info.id;
+
+	memset(&info, 0, sizeof(info));
+	err = bpf_obj_get_info_by_fd(fd2, &info, &len);
+	if (CHECK_FAIL(err))
+		goto out_2;
+	id2 = info.id;
+
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, fd1, XDP_FLAGS_EXPECT_ID,
+				       &opts);
+	if (CHECK(err, "load_ok", "initial load failed"))
+		goto out_close;
+
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, fd2, XDP_FLAGS_EXPECT_ID,
+				       &opts);
+	if (CHECK(!err, "load_fail", "load with expected id didn't fail"))
+		goto out;
+
+	opts.old_id = id1;
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, fd2, 0, &opts);
+	if (CHECK(err, "replace_ok", "replace valid old_id failed"))
+		goto out;
+
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, fd3, 0, &opts);
+	if (CHECK(!err, "replace_fail", "replace invalid old_id didn't fail"))
+		goto out;
+
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, -1, 0, &opts);
+	if (CHECK(!err, "remove_fail", "remove invalid old_id didn't fail"))
+		goto out;
+
+	opts.old_id = id2;
+	err = bpf_set_link_xdp_fd_opts(IFINDEX_LO, -1, 0, &opts);
+	if (CHECK(err, "remove_ok", "remove valid old_id failed"))
+		goto out;
+
+out:
+	bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0);
+out_close:
+	bpf_object__close(obj3);
+out_2:
+	bpf_object__close(obj2);
+out_1:
+	bpf_object__close(obj1);
+}


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
@ 2020-03-24 19:40   ` Jakub Kicinski
  2020-03-25  0:54   ` Andrii Nakryiko
  2020-03-25  1:46   ` Alexei Starovoitov
  2 siblings, 0 replies; 11+ messages in thread
From: Jakub Kicinski @ 2020-03-24 19:40 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: netdev, bpf, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	David S. Miller, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

On Tue, 24 Mar 2020 19:12:53 +0100 Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> While it is currently possible for userspace to specify that an existing
> XDP program should not be replaced when attaching to an interface, there is
> no mechanism to safely replace a specific XDP program with another.
> 
> This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
> set along with IFLA_XDP_FD. If set, the kernel will check that the program
> currently loaded on the interface matches the expected one, and fail the
> operation if it does not. This corresponds to a 'cmpxchg' memory operation.
> Setting the new attribute with a negative value means that no program is
> expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
> flag.
> 
> A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
> request checking of the EXPECTED_ID attribute. This is needed for userspace
> to discover whether the kernel supports the new attribute.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Thanks!

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
  2020-03-24 19:40   ` Jakub Kicinski
@ 2020-03-25  0:54   ` Andrii Nakryiko
  2020-03-25  1:43     ` Jakub Kicinski
  2020-03-25  1:46   ` Alexei Starovoitov
  2 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-03-25  0:54 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend, Lorenz Bauer, Andrey Ignatov

On Tue, Mar 24, 2020 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> From: Toke Høiland-Jørgensen <toke@redhat.com>
>
> While it is currently possible for userspace to specify that an existing
> XDP program should not be replaced when attaching to an interface, there is
> no mechanism to safely replace a specific XDP program with another.
>
> This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
> set along with IFLA_XDP_FD. If set, the kernel will check that the program
> currently loaded on the interface matches the expected one, and fail the
> operation if it does not. This corresponds to a 'cmpxchg' memory operation.
> Setting the new attribute with a negative value means that no program is
> expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
> flag.
>
> A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
> request checking of the EXPECTED_ID attribute. This is needed for userspace
> to discover whether the kernel supports the new attribute.

Doesn't it feel inconsistent in UAPI that FD is used to specify XDP
program to be attached, but ID is used to specify expected XDP
program? Especially that the same cgroup use case is using
(consistently) prog FDs. Or is it another case where XDP needs its own
special way?

>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/linux/netdevice.h    |    2 +-
>  include/uapi/linux/if_link.h |    4 +++-
>  net/core/dev.c               |   14 +++++++++-----
>  net/core/rtnetlink.c         |   13 +++++++++++++
>  4 files changed, 26 insertions(+), 7 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-25  0:54   ` Andrii Nakryiko
@ 2020-03-25  1:43     ` Jakub Kicinski
  2020-03-25 16:48       ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2020-03-25  1:43 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Toke Høiland-Jørgensen, Networking, bpf,
	Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
	Yonghong Song, Andrii Nakryiko, David S. Miller,
	Jesper Dangaard Brouer, John Fastabend, Lorenz Bauer,
	Andrey Ignatov

On Tue, 24 Mar 2020 17:54:07 -0700 Andrii Nakryiko wrote:
> On Tue, Mar 24, 2020 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >
> > From: Toke Høiland-Jørgensen <toke@redhat.com>
> >
> > While it is currently possible for userspace to specify that an existing
> > XDP program should not be replaced when attaching to an interface, there is
> > no mechanism to safely replace a specific XDP program with another.
> >
> > This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
> > set along with IFLA_XDP_FD. If set, the kernel will check that the program
> > currently loaded on the interface matches the expected one, and fail the
> > operation if it does not. This corresponds to a 'cmpxchg' memory operation.
> > Setting the new attribute with a negative value means that no program is
> > expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
> > flag.
> >
> > A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
> > request checking of the EXPECTED_ID attribute. This is needed for userspace
> > to discover whether the kernel supports the new attribute.  
> 
> Doesn't it feel inconsistent in UAPI that FD is used to specify XDP
> program to be attached, but ID is used to specify expected XDP
> program? Especially that the same cgroup use case is using
> (consistently) prog FDs. Or is it another case where XDP needs its own
> special way?

There was a comment during review of v1, I wish you spoke up then.

The prog ID is what dump returns, so the consistency can go either way
(note that this API predates object IDs). Since XDP uses IDs internally
it's just simpler to take prog ID.

But it's a detail, so if you feel strongly I don't really mind.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
  2020-03-24 19:40   ` Jakub Kicinski
  2020-03-25  0:54   ` Andrii Nakryiko
@ 2020-03-25  1:46   ` Alexei Starovoitov
  2020-03-25 16:48     ` Toke Høiland-Jørgensen
  2 siblings, 1 reply; 11+ messages in thread
From: Alexei Starovoitov @ 2020-03-25  1:46 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: netdev, bpf, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend, Lorenz Bauer, Andrey Ignatov

On Tue, Mar 24, 2020 at 07:12:53PM +0100, Toke Høiland-Jørgensen wrote:
> From: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> While it is currently possible for userspace to specify that an existing
> XDP program should not be replaced when attaching to an interface, there is
> no mechanism to safely replace a specific XDP program with another.
> 
> This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
> set along with IFLA_XDP_FD. If set, the kernel will check that the program
> currently loaded on the interface matches the expected one, and fail the
> operation if it does not. This corresponds to a 'cmpxchg' memory operation.
> Setting the new attribute with a negative value means that no program is
> expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
> flag.
> 
> A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
> request checking of the EXPECTED_ID attribute. This is needed for userspace
> to discover whether the kernel supports the new attribute.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

Over the years of defining apis to attach bpf progs to different kernel
components the key mistake we made is that we tried to use corresponding
subsystem way of doing thing and it failed every time. What made the problem
worse that this failure we only learned after many years. Attaching xdp via
netlink felt great back then. Attaching clsbpf via tc felt awesome too. Doing
cgroup-bpf via bpf syscall with three different attaching ways also felt great.
All of these attaching things turned out to be broken because all of them
failed to provide the concept of ownership of the attachment. Which caused 10k
clsbpf progs on one netdev, 64 cgroup-bpf progs in one cgroup, nuked xdp progs.
Hence we have to add the ownership model first. Doing mini extensions to
existing apis is not addressing the giant issue of existing apis.

The idea of this patch is to do atomic replacement of xdp prog. It's a good
idea and useful feature, but I don't want to extend existing broken apis to do
add this feature. atomic replacement has to come with thought through owner
model first.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-25  1:46   ` Alexei Starovoitov
@ 2020-03-25 16:48     ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-25 16:48 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: netdev, bpf, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend, Lorenz Bauer, Andrey Ignatov

Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:

> On Tue, Mar 24, 2020 at 07:12:53PM +0100, Toke Høiland-Jørgensen wrote:
>> From: Toke Høiland-Jørgensen <toke@redhat.com>
>> 
>> While it is currently possible for userspace to specify that an existing
>> XDP program should not be replaced when attaching to an interface, there is
>> no mechanism to safely replace a specific XDP program with another.
>> 
>> This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
>> set along with IFLA_XDP_FD. If set, the kernel will check that the program
>> currently loaded on the interface matches the expected one, and fail the
>> operation if it does not. This corresponds to a 'cmpxchg' memory operation.
>> Setting the new attribute with a negative value means that no program is
>> expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
>> flag.
>> 
>> A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
>> request checking of the EXPECTED_ID attribute. This is needed for userspace
>> to discover whether the kernel supports the new attribute.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Over the years of defining apis to attach bpf progs to different kernel
> components the key mistake we made is that we tried to use corresponding
> subsystem way of doing thing and it failed every time. What made the problem
> worse that this failure we only learned after many years. Attaching xdp via
> netlink felt great back then. Attaching clsbpf via tc felt awesome too. Doing
> cgroup-bpf via bpf syscall with three different attaching ways also felt great.
> All of these attaching things turned out to be broken because all of them
> failed to provide the concept of ownership of the attachment. Which caused 10k
> clsbpf progs on one netdev, 64 cgroup-bpf progs in one cgroup, nuked xdp progs.
> Hence we have to add the ownership model first. Doing mini extensions to
> existing apis is not addressing the giant issue of existing apis.
>
> The idea of this patch is to do atomic replacement of xdp prog. It's a good
> idea and useful feature, but I don't want to extend existing broken apis to do
> add this feature. atomic replacement has to come with thought through owner
> model first.

Setting aside the question of which is the best abstraction to represent
an attachment, it seems to me that the actual behavioural problem (XDP
programs being overridden by mistake) would be solvable by this patch,
assuming well-behaved userspace applications.

If you do need kernel support, we could extend the netlink API to accept
bpf_link FDs in addition to prog FDs. Then applications that fit the
bpf_link model could use netlink to setup the initial link (and any
other device configuration they need to do anyway), and any subsequent
replacements would be done by LINK_UPDATE bpf() operations. The lack of
CAP_NET_ADMIN could even restrict applications from removing the
bpf_link attachment from the netdev, without preventing them from
swapping out the bpf_prog attached to it.

This would also keep netlink solely in charge of netdev configuration,
and prevent any issues with a netdev being locked due to a bpf_link
attachment.

-Toke


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP
  2020-03-25  1:43     ` Jakub Kicinski
@ 2020-03-25 16:48       ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 11+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-03-25 16:48 UTC (permalink / raw)
  To: Jakub Kicinski, Andrii Nakryiko
  Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann,
	Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
	David S. Miller, Jesper Dangaard Brouer, John Fastabend,
	Lorenz Bauer, Andrey Ignatov

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 24 Mar 2020 17:54:07 -0700 Andrii Nakryiko wrote:
>> On Tue, Mar 24, 2020 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >
>> > From: Toke Høiland-Jørgensen <toke@redhat.com>
>> >
>> > While it is currently possible for userspace to specify that an existing
>> > XDP program should not be replaced when attaching to an interface, there is
>> > no mechanism to safely replace a specific XDP program with another.
>> >
>> > This patch adds a new netlink attribute, IFLA_XDP_EXPECTED_ID, which can be
>> > set along with IFLA_XDP_FD. If set, the kernel will check that the program
>> > currently loaded on the interface matches the expected one, and fail the
>> > operation if it does not. This corresponds to a 'cmpxchg' memory operation.
>> > Setting the new attribute with a negative value means that no program is
>> > expected to be attached, which corresponds to setting the UPDATE_IF_NOEXIST
>> > flag.
>> >
>> > A new companion flag, XDP_FLAGS_EXPECT_ID, is also added to explicitly
>> > request checking of the EXPECTED_ID attribute. This is needed for userspace
>> > to discover whether the kernel supports the new attribute.  
>> 
>> Doesn't it feel inconsistent in UAPI that FD is used to specify XDP
>> program to be attached, but ID is used to specify expected XDP
>> program? Especially that the same cgroup use case is using
>> (consistently) prog FDs. Or is it another case where XDP needs its own
>> special way?
>
> There was a comment during review of v1, I wish you spoke up then.
>
> The prog ID is what dump returns, so the consistency can go either way
> (note that this API predates object IDs). Since XDP uses IDs internally
> it's just simpler to take prog ID.
>
> But it's a detail, so if you feel strongly I don't really mind.

Using an FD instead of an ID does make this more extensible (such as
supporting bpf_link FDs in the future; see my other reply to Alexei). So
I'll respin this, and switch it back to EXPECTED_FD.

-Toke


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-03-25 16:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 18:12 [PATCH bpf-next v3 0/4] XDP: Support atomic replacement of XDP interface attachments Toke Høiland-Jørgensen
2020-03-24 18:12 ` [PATCH bpf-next v3 1/4] xdp: Support specifying expected existing program when attaching XDP Toke Høiland-Jørgensen
2020-03-24 19:40   ` Jakub Kicinski
2020-03-25  0:54   ` Andrii Nakryiko
2020-03-25  1:43     ` Jakub Kicinski
2020-03-25 16:48       ` Toke Høiland-Jørgensen
2020-03-25  1:46   ` Alexei Starovoitov
2020-03-25 16:48     ` Toke Høiland-Jørgensen
2020-03-24 18:12 ` [PATCH bpf-next v3 2/4] tools: Add EXPECTED_ID-related definitions in if_link.h Toke Høiland-Jørgensen
2020-03-24 18:12 ` [PATCH bpf-next v3 3/4] libbpf: Add function to set link XDP fd while specifying old program Toke Høiland-Jørgensen
2020-03-24 18:12 ` [PATCH bpf-next v3 4/4] selftests/bpf: Add tests for attaching XDP programs Toke Høiland-Jørgensen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).