bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/5] BPF link force-detach support
@ 2020-07-29 23:05 Andrii Nakryiko
  2020-07-29 23:05 ` [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command Andrii Nakryiko
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 23:05 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko,
	Toke Høiland-Jørgensen

This patch set adds new BPF link operation, LINK_DETACH, allowing processes
with BPF link FD to force-detach it from respective BPF hook, similarly how
BPF link is auto-detached when such BPF hook (e.g., cgroup, net_device, netns,
etc) is removed. This facility allows admin to forcefully undo BPF link
attachment, while process that created BPF link in the first place is left
intact.

Once force-detached, BPF link stays valid in the kernel as long as there is at
least one FD open against it. It goes into defunct state, just like
auto-detached BPF link.

bpftool also got `link detach` command to allow triggering this in
non-programmatic fashion.

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

Andrii Nakryiko (5):
  bpf: add support for forced LINK_DETACH command
  libbpf: add bpf_link detach APIs
  selftests/bpf: add link detach tests for cgroup, netns, and xdp
    bpf_links
  tools/bpftool: add `link detach` subcommand
  tools/bpftool: add documentation and bash-completion for `link detach`

 include/linux/bpf.h                           |  1 +
 include/uapi/linux/bpf.h                      |  5 ++
 kernel/bpf/cgroup.c                           | 15 +++++-
 kernel/bpf/net_namespace.c                    |  8 +++
 kernel/bpf/syscall.c                          | 26 ++++++++++
 net/core/dev.c                                | 11 +++-
 .../bpftool/Documentation/bpftool-link.rst    |  8 +++
 tools/bpf/bpftool/bash-completion/bpftool     |  4 +-
 tools/bpf/bpftool/link.c                      | 35 ++++++++++++-
 tools/include/uapi/linux/bpf.h                |  5 ++
 tools/lib/bpf/bpf.c                           | 10 ++++
 tools/lib/bpf/bpf.h                           |  2 +
 tools/lib/bpf/libbpf.c                        |  5 ++
 tools/lib/bpf/libbpf.h                        |  1 +
 tools/lib/bpf/libbpf.map                      |  2 +
 .../selftests/bpf/prog_tests/cgroup_link.c    | 20 +++++++-
 .../selftests/bpf/prog_tests/sk_lookup.c      | 51 +++++++++----------
 .../selftests/bpf/prog_tests/xdp_link.c       | 14 +++++
 tools/testing/selftests/bpf/testing_helpers.c | 14 +++++
 tools/testing/selftests/bpf/testing_helpers.h |  3 ++
 20 files changed, 206 insertions(+), 34 deletions(-)

-- 
2.24.1


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

* [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command
  2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
@ 2020-07-29 23:05 ` Andrii Nakryiko
  2020-07-30 17:43   ` Song Liu
  2020-07-29 23:05 ` [PATCH bpf-next 2/5] libbpf: add bpf_link detach APIs Andrii Nakryiko
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 23:05 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko,
	Toke Høiland-Jørgensen

Add LINK_DETACH command to force-detach bpf_link without destroying it. It has
the same behavior as auto-detaching of bpf_link due to cgroup dying for
bpf_cgroup_link or net_device being destroyed for bpf_xdp_link. In such case,
bpf_link is still a valid kernel object, but is defuncts and doesn't hold BPF
program attached to corresponding BPF hook. This functionality allows users
with enough access rights to manually force-detach attached bpf_link without
killing respective owner process.

This patch implements LINK_DETACH for cgroup, xdp, and netns links, mostly
re-using existing link release handling code.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 include/linux/bpf.h        |  1 +
 include/uapi/linux/bpf.h   |  5 +++++
 kernel/bpf/cgroup.c        | 15 ++++++++++++++-
 kernel/bpf/net_namespace.c |  8 ++++++++
 kernel/bpf/syscall.c       | 26 ++++++++++++++++++++++++++
 net/core/dev.c             | 11 ++++++++++-
 6 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 40c5e206ecf2..cef4ef0d2b4e 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -793,6 +793,7 @@ struct bpf_link {
 struct bpf_link_ops {
 	void (*release)(struct bpf_link *link);
 	void (*dealloc)(struct bpf_link *link);
+	int (*detach)(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);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index eb5e0c38eb2c..b134e679e9db 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -117,6 +117,7 @@ enum bpf_cmd {
 	BPF_LINK_GET_NEXT_ID,
 	BPF_ENABLE_STATS,
 	BPF_ITER_CREATE,
+	BPF_LINK_DETACH,
 };
 
 enum bpf_map_type {
@@ -634,6 +635,10 @@ union bpf_attr {
 		__u32		old_prog_fd;
 	} link_update;
 
+	struct {
+		__u32		link_fd;
+	} link_detach;
+
 	struct { /* struct used by BPF_ENABLE_STATS command */
 		__u32		type;
 	} enable_stats;
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 957cce1d5168..83ff127ef7ae 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -814,6 +814,7 @@ static void bpf_cgroup_link_release(struct bpf_link *link)
 {
 	struct bpf_cgroup_link *cg_link =
 		container_of(link, struct bpf_cgroup_link, link);
+	struct cgroup *cg;
 
 	/* link might have been auto-detached by dying cgroup already,
 	 * in that case our work is done here
@@ -832,8 +833,12 @@ static void bpf_cgroup_link_release(struct bpf_link *link)
 	WARN_ON(__cgroup_bpf_detach(cg_link->cgroup, NULL, cg_link,
 				    cg_link->type));
 
+	cg = cg_link->cgroup;
+	cg_link->cgroup = NULL;
+
 	mutex_unlock(&cgroup_mutex);
-	cgroup_put(cg_link->cgroup);
+
+	cgroup_put(cg);
 }
 
 static void bpf_cgroup_link_dealloc(struct bpf_link *link)
@@ -844,6 +849,13 @@ static void bpf_cgroup_link_dealloc(struct bpf_link *link)
 	kfree(cg_link);
 }
 
+static int bpf_cgroup_link_detach(struct bpf_link *link)
+{
+	bpf_cgroup_link_release(link);
+
+	return 0;
+}
+
 static void bpf_cgroup_link_show_fdinfo(const struct bpf_link *link,
 					struct seq_file *seq)
 {
@@ -883,6 +895,7 @@ static int bpf_cgroup_link_fill_link_info(const struct bpf_link *link,
 static const struct bpf_link_ops bpf_cgroup_link_lops = {
 	.release = bpf_cgroup_link_release,
 	.dealloc = bpf_cgroup_link_dealloc,
+	.detach = bpf_cgroup_link_detach,
 	.update_prog = cgroup_bpf_replace,
 	.show_fdinfo = bpf_cgroup_link_show_fdinfo,
 	.fill_link_info = bpf_cgroup_link_fill_link_info,
diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
index 71405edd667c..542f275bf252 100644
--- a/kernel/bpf/net_namespace.c
+++ b/kernel/bpf/net_namespace.c
@@ -142,9 +142,16 @@ static void bpf_netns_link_release(struct bpf_link *link)
 	bpf_prog_array_free(old_array);
 
 out_unlock:
+	net_link->net = NULL;
 	mutex_unlock(&netns_bpf_mutex);
 }
 
+static int bpf_netns_link_detach(struct bpf_link *link)
+{
+	bpf_netns_link_release(link);
+	return 0;
+}
+
 static void bpf_netns_link_dealloc(struct bpf_link *link)
 {
 	struct bpf_netns_link *net_link =
@@ -228,6 +235,7 @@ static void bpf_netns_link_show_fdinfo(const struct bpf_link *link,
 static const struct bpf_link_ops bpf_netns_link_ops = {
 	.release = bpf_netns_link_release,
 	.dealloc = bpf_netns_link_dealloc,
+	.detach = bpf_netns_link_detach,
 	.update_prog = bpf_netns_link_update_prog,
 	.fill_link_info = bpf_netns_link_fill_info,
 	.show_fdinfo = bpf_netns_link_show_fdinfo,
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index cd3d599e9e90..2f343ce15747 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3991,6 +3991,29 @@ static int link_update(union bpf_attr *attr)
 	return ret;
 }
 
+#define BPF_LINK_DETACH_LAST_FIELD link_detach.link_fd
+
+static int link_detach(union bpf_attr *attr)
+{
+	struct bpf_link *link;
+	int ret;
+
+	if (CHECK_ATTR(BPF_LINK_DETACH))
+		return -EINVAL;
+
+	link = bpf_link_get_from_fd(attr->link_detach.link_fd);
+	if (IS_ERR(link))
+		return PTR_ERR(link);
+
+	if (link->ops->detach)
+		ret = link->ops->detach(link);
+	else
+		ret = -EOPNOTSUPP;
+
+	bpf_link_put(link);
+	return ret;
+}
+
 static int bpf_link_inc_not_zero(struct bpf_link *link)
 {
 	return atomic64_fetch_add_unless(&link->refcnt, 1, 0) ? 0 : -ENOENT;
@@ -4240,6 +4263,9 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
 	case BPF_ITER_CREATE:
 		err = bpf_iter_create(&attr);
 		break;
+	case BPF_LINK_DETACH:
+		err = link_detach(&attr);
+		break;
 	default:
 		err = -EINVAL;
 		break;
diff --git a/net/core/dev.c b/net/core/dev.c
index a2a57988880a..c8b911b10187 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8979,12 +8979,20 @@ static void bpf_xdp_link_release(struct bpf_link *link)
 	/* 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)
+	if (xdp_link->dev) {
 		WARN_ON(dev_xdp_detach_link(xdp_link->dev, NULL, xdp_link));
+		xdp_link->dev = NULL;
+	}
 
 	rtnl_unlock();
 }
 
+static int bpf_xdp_link_detach(struct bpf_link *link)
+{
+	bpf_xdp_link_release(link);
+	return 0;
+}
+
 static void bpf_xdp_link_dealloc(struct bpf_link *link)
 {
 	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
@@ -9066,6 +9074,7 @@ 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,
+	.detach = bpf_xdp_link_detach,
 	.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


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

* [PATCH bpf-next 2/5] libbpf: add bpf_link detach APIs
  2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
  2020-07-29 23:05 ` [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command Andrii Nakryiko
@ 2020-07-29 23:05 ` Andrii Nakryiko
  2020-07-30 17:51   ` Song Liu
  2020-07-29 23:05 ` [PATCH bpf-next 3/5] selftests/bpf: add link detach tests for cgroup, netns, and xdp bpf_links Andrii Nakryiko
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 23:05 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko,
	Toke Høiland-Jørgensen

Add low-level bpf_link_detach() API. Also add higher-level bpf_link__detach()
one.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/include/uapi/linux/bpf.h |  5 +++++
 tools/lib/bpf/bpf.c            | 10 ++++++++++
 tools/lib/bpf/bpf.h            |  2 ++
 tools/lib/bpf/libbpf.c         |  5 +++++
 tools/lib/bpf/libbpf.h         |  1 +
 tools/lib/bpf/libbpf.map       |  2 ++
 6 files changed, 25 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index eb5e0c38eb2c..b134e679e9db 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -117,6 +117,7 @@ enum bpf_cmd {
 	BPF_LINK_GET_NEXT_ID,
 	BPF_ENABLE_STATS,
 	BPF_ITER_CREATE,
+	BPF_LINK_DETACH,
 };
 
 enum bpf_map_type {
@@ -634,6 +635,10 @@ union bpf_attr {
 		__u32		old_prog_fd;
 	} link_update;
 
+	struct {
+		__u32		link_fd;
+	} link_detach;
+
 	struct { /* struct used by BPF_ENABLE_STATS command */
 		__u32		type;
 	} enable_stats;
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index e1bdf214f75f..eab14c97c15d 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -603,6 +603,16 @@ int bpf_link_create(int prog_fd, int target_fd,
 	return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr));
 }
 
+int bpf_link_detach(int link_fd)
+{
+	union bpf_attr attr;
+
+	memset(&attr, 0, sizeof(attr));
+	attr.link_detach.link_fd = link_fd;
+
+	return sys_bpf(BPF_LINK_DETACH, &attr, sizeof(attr));
+}
+
 int bpf_link_update(int link_fd, int new_prog_fd,
 		    const struct bpf_link_update_opts *opts)
 {
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 6d367e01d05e..28855fd5b5f4 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -178,6 +178,8 @@ LIBBPF_API int bpf_link_create(int prog_fd, int target_fd,
 			       enum bpf_attach_type attach_type,
 			       const struct bpf_link_create_opts *opts);
 
+LIBBPF_API int bpf_link_detach(int link_fd);
+
 struct bpf_link_update_opts {
 	size_t sz; /* size of this struct for forward/backward compatibility */
 	__u32 flags;	   /* extra flags */
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 54830d603fee..6c5c14f639e8 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7748,6 +7748,11 @@ struct bpf_link *bpf_link__open(const char *path)
 	return link;
 }
 
+int bpf_link__detach(struct bpf_link *link)
+{
+	return bpf_link_detach(link->fd) ? -errno : 0;
+}
+
 int bpf_link__pin(struct bpf_link *link, const char *path)
 {
 	int err;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 9924385462ab..3ed1399bfbbc 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -229,6 +229,7 @@ LIBBPF_API int bpf_link__unpin(struct bpf_link *link);
 LIBBPF_API int bpf_link__update_program(struct bpf_link *link,
 					struct bpf_program *prog);
 LIBBPF_API void bpf_link__disconnect(struct bpf_link *link);
+LIBBPF_API int bpf_link__detach(struct bpf_link *link);
 LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
 
 LIBBPF_API struct bpf_link *
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index ca49a6a7e5b2..099863411f7d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -273,6 +273,8 @@ LIBBPF_0.0.9 {
 
 LIBBPF_0.1.0 {
 	global:
+		bpf_link__detach;
+		bpf_link_detach;
 		bpf_map__ifindex;
 		bpf_map__key_size;
 		bpf_map__map_flags;
-- 
2.24.1


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

* [PATCH bpf-next 3/5] selftests/bpf: add link detach tests for cgroup, netns, and xdp bpf_links
  2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
  2020-07-29 23:05 ` [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command Andrii Nakryiko
  2020-07-29 23:05 ` [PATCH bpf-next 2/5] libbpf: add bpf_link detach APIs Andrii Nakryiko
@ 2020-07-29 23:05 ` Andrii Nakryiko
  2020-07-30 20:56   ` Song Liu
  2020-07-29 23:05 ` [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand Andrii Nakryiko
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 23:05 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko,
	Toke Høiland-Jørgensen

Add bpf_link__detach() testing to selftests for cgroup, netns, and xdp
bpf_links.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 .../selftests/bpf/prog_tests/cgroup_link.c    | 20 +++++++-
 .../selftests/bpf/prog_tests/sk_lookup.c      | 51 +++++++++----------
 .../selftests/bpf/prog_tests/xdp_link.c       | 14 +++++
 tools/testing/selftests/bpf/testing_helpers.c | 14 +++++
 tools/testing/selftests/bpf/testing_helpers.h |  3 ++
 5 files changed, 73 insertions(+), 29 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_link.c b/tools/testing/selftests/bpf/prog_tests/cgroup_link.c
index 6e04f8d1d15b..4d9b514b3fd9 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_link.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_link.c
@@ -2,6 +2,7 @@
 
 #include <test_progs.h>
 #include "cgroup_helpers.h"
+#include "testing_helpers.h"
 #include "test_cgroup_link.skel.h"
 
 static __u32 duration = 0;
@@ -37,7 +38,8 @@ void test_cgroup_link(void)
 	int last_cg = ARRAY_SIZE(cgs) - 1, cg_nr = ARRAY_SIZE(cgs);
 	DECLARE_LIBBPF_OPTS(bpf_link_update_opts, link_upd_opts);
 	struct bpf_link *links[ARRAY_SIZE(cgs)] = {}, *tmp_link;
-	__u32 prog_ids[ARRAY_SIZE(cgs)], prog_cnt = 0, attach_flags;
+	__u32 prog_ids[ARRAY_SIZE(cgs)], prog_cnt = 0, attach_flags, prog_id;
+	struct bpf_link_info info;
 	int i = 0, err, prog_fd;
 	bool detach_legacy = false;
 
@@ -219,6 +221,22 @@ void test_cgroup_link(void)
 	/* BPF programs should still get called */
 	ping_and_check(0, cg_nr);
 
+	prog_id = link_info_prog_id(links[0], &info);
+	CHECK(prog_id == 0, "link_info", "failed\n");
+	CHECK(info.cgroup.cgroup_id == 0, "cgroup_id", "unexpected %llu\n", info.cgroup.cgroup_id);
+
+	err = bpf_link__detach(links[0]);
+	if (CHECK(err, "link_detach", "failed %d\n", err))
+		goto cleanup;
+
+	/* cgroup_id should be zero in link_info */
+	prog_id = link_info_prog_id(links[0], &info);
+	CHECK(prog_id == 0, "link_info", "failed\n");
+	CHECK(info.cgroup.cgroup_id != 0, "cgroup_id", "unexpected %llu\n", info.cgroup.cgroup_id);
+
+	/* First BPF program shouldn't be called anymore */
+	ping_and_check(0, cg_nr - 1);
+
 	/* leave cgroup and remove them, don't detach programs */
 	cleanup_cgroup_environment();
 
diff --git a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
index 9bbd2b2b7630..c498e03c2777 100644
--- a/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
+++ b/tools/testing/selftests/bpf/prog_tests/sk_lookup.c
@@ -34,6 +34,7 @@
 #include "bpf_util.h"
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
+#include "testing_helpers.h"
 #include "test_sk_lookup.skel.h"
 
 /* External (address, port) pairs the client sends packets to. */
@@ -469,34 +470,10 @@ static int update_lookup_map(struct bpf_map *map, int index, int sock_fd)
 	return 0;
 }
 
-static __u32 link_info_prog_id(struct bpf_link *link)
-{
-	struct bpf_link_info info = {};
-	__u32 info_len = sizeof(info);
-	int link_fd, err;
-
-	link_fd = bpf_link__fd(link);
-	if (CHECK(link_fd < 0, "bpf_link__fd", "failed\n")) {
-		errno = -link_fd;
-		log_err("bpf_link__fd failed");
-		return 0;
-	}
-
-	err = bpf_obj_get_info_by_fd(link_fd, &info, &info_len);
-	if (CHECK(err, "bpf_obj_get_info_by_fd", "failed\n")) {
-		log_err("bpf_obj_get_info_by_fd");
-		return 0;
-	}
-	if (CHECK(info_len != sizeof(info), "bpf_obj_get_info_by_fd",
-		  "unexpected info len %u\n", info_len))
-		return 0;
-
-	return info.prog_id;
-}
-
 static void query_lookup_prog(struct test_sk_lookup *skel)
 {
 	struct bpf_link *link[3] = {};
+	struct bpf_link_info info;
 	__u32 attach_flags = 0;
 	__u32 prog_ids[3] = {};
 	__u32 prog_cnt = 3;
@@ -534,18 +511,36 @@ static void query_lookup_prog(struct test_sk_lookup *skel)
 	if (CHECK(prog_cnt != 3, "bpf_prog_query",
 		  "wrong program count on query: %u", prog_cnt))
 		goto detach;
-	prog_id = link_info_prog_id(link[0]);
+	prog_id = link_info_prog_id(link[0], &info);
 	CHECK(prog_ids[0] != prog_id, "bpf_prog_query",
 	      "invalid program #0 id on query: %u != %u\n",
 	      prog_ids[0], prog_id);
-	prog_id = link_info_prog_id(link[1]);
+	CHECK(info.netns.netns_ino == 0, "netns_ino",
+	      "unexpected netns_ino: %u\n", info.netns.netns_ino);
+	prog_id = link_info_prog_id(link[1], &info);
 	CHECK(prog_ids[1] != prog_id, "bpf_prog_query",
 	      "invalid program #1 id on query: %u != %u\n",
 	      prog_ids[1], prog_id);
-	prog_id = link_info_prog_id(link[2]);
+	CHECK(info.netns.netns_ino == 0, "netns_ino",
+	      "unexpected netns_ino: %u\n", info.netns.netns_ino);
+	prog_id = link_info_prog_id(link[2], &info);
 	CHECK(prog_ids[2] != prog_id, "bpf_prog_query",
 	      "invalid program #2 id on query: %u != %u\n",
 	      prog_ids[2], prog_id);
+	CHECK(info.netns.netns_ino == 0, "netns_ino",
+	      "unexpected netns_ino: %u\n", info.netns.netns_ino);
+
+	err = bpf_link__detach(link[0]);
+	if (CHECK(err, "link_detach", "failed %d\n", err))
+		goto detach;
+
+	/* prog id is still there, but netns_ino is zeroed out */
+	prog_id = link_info_prog_id(link[0], &info);
+	CHECK(prog_ids[0] != prog_id, "bpf_prog_query",
+	      "invalid program #0 id on query: %u != %u\n",
+	      prog_ids[0], prog_id);
+	CHECK(info.netns.netns_ino != 0, "netns_ino",
+	      "unexpected netns_ino: %u\n", info.netns.netns_ino);
 
 detach:
 	if (link[2])
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_link.c b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
index 52cba6795d40..6f814999b395 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_link.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_link.c
@@ -131,6 +131,20 @@ void test_xdp_link(void)
 	CHECK(link_info.xdp.ifindex != IFINDEX_LO, "link_ifindex",
 	      "got %u != exp %u\n", link_info.xdp.ifindex, IFINDEX_LO);
 
+	err = bpf_link__detach(link);
+	if (CHECK(err, "link_detach", "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.prog_id != id1, "link_prog_id",
+	      "got %u != exp %u\n", link_info.prog_id, id1);
+	/* ifindex should be zeroed out */
+	CHECK(link_info.xdp.ifindex != 0, "link_ifindex",
+	      "got %u != exp %u\n", link_info.xdp.ifindex, 0);
+
 cleanup:
 	test_xdp_link__destroy(skel1);
 	test_xdp_link__destroy(skel2);
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 0af6337a8962..800d503e5cb4 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -64,3 +64,17 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len)
 
 	return 0;
 }
+
+__u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info)
+{
+	__u32 info_len = sizeof(*info);
+	int err;
+
+	memset(info, 0, sizeof(*info));
+	err = bpf_obj_get_info_by_fd(bpf_link__fd(link), info, &info_len);
+	if (err) {
+		printf("failed to get link info: %d\n", -errno);
+		return 0;
+	}
+	return info->prog_id;
+}
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index 923b51762759..d4f8e749611b 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -1,5 +1,8 @@
 /* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
 /* Copyright (C) 2020 Facebook, Inc. */
 #include <stdbool.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
 
 int parse_num_list(const char *s, bool **set, int *set_len);
+__u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info);
-- 
2.24.1


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

* [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand
  2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
                   ` (2 preceding siblings ...)
  2020-07-29 23:05 ` [PATCH bpf-next 3/5] selftests/bpf: add link detach tests for cgroup, netns, and xdp bpf_links Andrii Nakryiko
@ 2020-07-29 23:05 ` Andrii Nakryiko
  2020-07-30 21:02   ` Song Liu
  2020-07-29 23:05 ` [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach` Andrii Nakryiko
  2020-08-10 15:01 ` [PATCH bpf-next 0/5] BPF link force-detach support Toke Høiland-Jørgensen
  5 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 23:05 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko,
	Toke Høiland-Jørgensen

Add ability to force-detach BPF link. Also add missing error message, if
specified link ID is wrong.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/bpf/bpftool/link.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index 326b8fdf0243..278befa34ed6 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -22,6 +22,8 @@ static const char * const link_type_name[] = {
 
 static int link_parse_fd(int *argc, char ***argv)
 {
+	int fd;
+
 	if (is_prefix(**argv, "id")) {
 		unsigned int id;
 		char *endptr;
@@ -35,7 +37,10 @@ static int link_parse_fd(int *argc, char ***argv)
 		}
 		NEXT_ARGP();
 
-		return bpf_link_get_fd_by_id(id);
+		fd = bpf_link_get_fd_by_id(id);
+		if (fd < 0)
+			p_err("failed to get link with ID %d: %d", id, -errno);
+		return fd;
 	} else if (is_prefix(**argv, "pinned")) {
 		char *path;
 
@@ -316,6 +321,32 @@ static int do_pin(int argc, char **argv)
 	return err;
 }
 
+static int do_detach(int argc, char **argv)
+{
+	int err, fd;
+
+	if (argc != 2)
+		return BAD_ARG();
+
+	fd = link_parse_fd(&argc, &argv);
+	if (fd < 0)
+		return 1;
+
+	err = bpf_link_detach(fd);
+	if (err)
+		err = -errno;
+	close(fd);
+	if (err) {
+		p_err("failed link detach: %d", err);
+		return 1;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	return 0;
+}
+
 static int do_help(int argc, char **argv)
 {
 	if (json_output) {
@@ -326,6 +357,7 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %1$s %2$s { show | list }   [LINK]\n"
 		"       %1$s %2$s pin        LINK  FILE\n"
+		"       %1$s %2$s detach     LINK\n"
 		"       %1$s %2$s help\n"
 		"\n"
 		"       " HELP_SPEC_LINK "\n"
@@ -341,6 +373,7 @@ static const struct cmd cmds[] = {
 	{ "list",	do_show },
 	{ "help",	do_help },
 	{ "pin",	do_pin },
+	{ "detach",	do_detach },
 	{ 0 }
 };
 
-- 
2.24.1


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

* [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach`
  2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
                   ` (3 preceding siblings ...)
  2020-07-29 23:05 ` [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand Andrii Nakryiko
@ 2020-07-29 23:05 ` Andrii Nakryiko
  2020-07-30 21:13   ` Song Liu
  2020-08-10 15:01 ` [PATCH bpf-next 0/5] BPF link force-detach support Toke Høiland-Jørgensen
  5 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 23:05 UTC (permalink / raw)
  To: bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko,
	Toke Høiland-Jørgensen

Add info on link detach sub-command to man page. Add detach to bash-completion
as well.

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/bpf/bpftool/Documentation/bpftool-link.rst | 8 ++++++++
 tools/bpf/bpftool/bash-completion/bpftool        | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-link.rst b/tools/bpf/bpftool/Documentation/bpftool-link.rst
index 38b0949a185b..4a52e7a93339 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-link.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-link.rst
@@ -21,6 +21,7 @@ LINK COMMANDS
 
 |	**bpftool** **link { show | list }** [*LINK*]
 |	**bpftool** **link pin** *LINK* *FILE*
+|	**bpftool** **link detach *LINK*
 |	**bpftool** **link help**
 |
 |	*LINK* := { **id** *LINK_ID* | **pinned** *FILE* }
@@ -49,6 +50,13 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
+	**bpftool link detach** *LINK*
+		  Force-detach link *LINK*. BPF link and its underlying BPF
+		  program will stay valid, but they will be detached from the
+		  respective BPF hook and BPF link will transition into
+		  a defunct state until last open file descriptor for that
+		  link is closed.
+
 	**bpftool link help**
 		  Print short help message.
 
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 257fa310ea2b..f53ed2f1a4aa 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -1122,7 +1122,7 @@ _bpftool()
             ;;
         link)
             case $command in
-                show|list|pin)
+                show|list|pin|detach)
                     case $prev in
                         id)
                             _bpftool_get_link_ids
@@ -1139,7 +1139,7 @@ _bpftool()
                     COMPREPLY=( $( compgen -W "$LINK_TYPE" -- "$cur" ) )
                     return 0
                     ;;
-                pin)
+                pin|detach)
                     if [[ $prev == "$command" ]]; then
                         COMPREPLY=( $( compgen -W "$LINK_TYPE" -- "$cur" ) )
                     else
-- 
2.24.1


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

* Re: [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command
  2020-07-29 23:05 ` [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command Andrii Nakryiko
@ 2020-07-30 17:43   ` Song Liu
  2020-07-30 19:03     ` Andrii Nakryiko
  0 siblings, 1 reply; 19+ messages in thread
From: Song Liu @ 2020-07-30 17:43 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, Netdev, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Kernel Team, Toke Høiland-Jørgensen



> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Add LINK_DETACH command to force-detach bpf_link without destroying it. It has
> the same behavior as auto-detaching of bpf_link due to cgroup dying for
> bpf_cgroup_link or net_device being destroyed for bpf_xdp_link. In such case,
> bpf_link is still a valid kernel object, but is defuncts and doesn't hold BPF
> program attached to corresponding BPF hook. This functionality allows users
> with enough access rights to manually force-detach attached bpf_link without
> killing respective owner process.
> 
> This patch implements LINK_DETACH for cgroup, xdp, and netns links, mostly
> re-using existing link release handling code.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

The code looks good to me. My only question is, do we need both 
bpf_link_ops->detach and bpf_link_ops->release? 

Thanks,
Song

[...]

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

* Re: [PATCH bpf-next 2/5] libbpf: add bpf_link detach APIs
  2020-07-29 23:05 ` [PATCH bpf-next 2/5] libbpf: add bpf_link detach APIs Andrii Nakryiko
@ 2020-07-30 17:51   ` Song Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Song Liu @ 2020-07-30 17:51 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko,
	Kernel Team, Toke Høiland-Jørgensen



> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Add low-level bpf_link_detach() API. Also add higher-level bpf_link__detach()
> one.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

> ---
> 

[...]

> 
> LIBBPF_0.1.0 {
> 	global:
> +		bpf_link__detach;
> +		bpf_link_detach;

I didn't realize capital_letter < '_' < small_letter until just now. :)

> 		bpf_map__ifindex;
> 		bpf_map__key_size;
> 		bpf_map__map_flags;
> -- 
> 2.24.1
> 


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

* Re: [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command
  2020-07-30 17:43   ` Song Liu
@ 2020-07-30 19:03     ` Andrii Nakryiko
  2020-07-30 20:53       ` Song Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-30 19:03 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, Netdev, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Toke Høiland-Jørgensen

On Thu, Jul 30, 2020 at 10:43 AM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Add LINK_DETACH command to force-detach bpf_link without destroying it. It has
> > the same behavior as auto-detaching of bpf_link due to cgroup dying for
> > bpf_cgroup_link or net_device being destroyed for bpf_xdp_link. In such case,
> > bpf_link is still a valid kernel object, but is defuncts and doesn't hold BPF
> > program attached to corresponding BPF hook. This functionality allows users
> > with enough access rights to manually force-detach attached bpf_link without
> > killing respective owner process.
> >
> > This patch implements LINK_DETACH for cgroup, xdp, and netns links, mostly
> > re-using existing link release handling code.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> The code looks good to me. My only question is, do we need both
> bpf_link_ops->detach and bpf_link_ops->release?

I think so. release() is mandatory for final clean up, after the last
FD was closed, so every type of bpf_link has to implement this.
detach() is optional, though, and potentially can do different things
than release(). It just so happens right now that three bpf_linkl
types can re-use release as-is (with minimal change to netns release
specifically for detach use case). So I think having two is better and
more flexible.

>
> Thanks,
> Song
>
> [...]

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

* Re: [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command
  2020-07-30 19:03     ` Andrii Nakryiko
@ 2020-07-30 20:53       ` Song Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Song Liu @ 2020-07-30 20:53 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Netdev, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team, Toke Høiland-Jørgensen



> On Jul 30, 2020, at 12:03 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Thu, Jul 30, 2020 at 10:43 AM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>>> 
>>> Add LINK_DETACH command to force-detach bpf_link without destroying it. It has
>>> the same behavior as auto-detaching of bpf_link due to cgroup dying for
>>> bpf_cgroup_link or net_device being destroyed for bpf_xdp_link. In such case,
>>> bpf_link is still a valid kernel object, but is defuncts and doesn't hold BPF
>>> program attached to corresponding BPF hook. This functionality allows users
>>> with enough access rights to manually force-detach attached bpf_link without
>>> killing respective owner process.
>>> 
>>> This patch implements LINK_DETACH for cgroup, xdp, and netns links, mostly
>>> re-using existing link release handling code.
>>> 
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> 
>> The code looks good to me. My only question is, do we need both
>> bpf_link_ops->detach and bpf_link_ops->release?
> 
> I think so. release() is mandatory for final clean up, after the last
> FD was closed, so every type of bpf_link has to implement this.
> detach() is optional, though, and potentially can do different things
> than release(). It just so happens right now that three bpf_linkl
> types can re-use release as-is (with minimal change to netns release
> specifically for detach use case). So I think having two is better and
> more flexible.

I see. Thanks for the explanation. 

Acked-by: Song Liu <songliubraving@fb.com>


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

* Re: [PATCH bpf-next 3/5] selftests/bpf: add link detach tests for cgroup, netns, and xdp bpf_links
  2020-07-29 23:05 ` [PATCH bpf-next 3/5] selftests/bpf: add link detach tests for cgroup, netns, and xdp bpf_links Andrii Nakryiko
@ 2020-07-30 20:56   ` Song Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Song Liu @ 2020-07-30 20:56 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko,
	Kernel Team, Toke Høiland-Jørgensen



> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Add bpf_link__detach() testing to selftests for cgroup, netns, and xdp
> bpf_links.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

[...]



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

* Re: [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand
  2020-07-29 23:05 ` [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand Andrii Nakryiko
@ 2020-07-30 21:02   ` Song Liu
  2020-07-30 21:14     ` Andrii Nakryiko
  0 siblings, 1 reply; 19+ messages in thread
From: Song Liu @ 2020-07-30 21:02 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko,
	Kernel Team, Toke Høiland-Jørgensen



> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Add ability to force-detach BPF link. Also add missing error message, if
> specified link ID is wrong.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

With two nitpicks below. 

[...]

> static int link_parse_fd(int *argc, char ***argv)
> {
> +	int fd;
> +
> 	if (is_prefix(**argv, "id")) {
> 		unsigned int id;
> 		char *endptr;
> @@ -35,7 +37,10 @@ static int link_parse_fd(int *argc, char ***argv)
> 		}
> 		NEXT_ARGP();
> 
> -		return bpf_link_get_fd_by_id(id);
> +		fd = bpf_link_get_fd_by_id(id);
> +		if (fd < 0)
> +			p_err("failed to get link with ID %d: %d", id, -errno);

How about we print strerror(errno) to match the rest of link.c?

[...]

> +static int do_detach(int argc, char **argv)
> +{
> +	int err, fd;
> +
> +	if (argc != 2)
> +		return BAD_ARG();
> +
> +	fd = link_parse_fd(&argc, &argv);
> +	if (fd < 0)
> +		return 1;
> +
> +	err = bpf_link_detach(fd);
> +	if (err)
> +		err = -errno;
> +	close(fd);
> +	if (err) {
> +		p_err("failed link detach: %d", err);

And strerror(err) here. 



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

* Re: [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach`
  2020-07-29 23:05 ` [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach` Andrii Nakryiko
@ 2020-07-30 21:13   ` Song Liu
  2020-07-30 21:16     ` Andrii Nakryiko
  0 siblings, 1 reply; 19+ messages in thread
From: Song Liu @ 2020-07-30 21:13 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: bpf, netdev, Alexei Starovoitov, daniel, andrii.nakryiko,
	Kernel Team, Toke Høiland-Jørgensen



> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> 
> Add info on link detach sub-command to man page. Add detach to bash-completion
> as well.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>

Acked-by: Song Liu <songliubraving@fb.com>

With one nitpick below. 

> ---

[...]

> @@ -49,6 +50,13 @@ DESCRIPTION
> 		  contain a dot character ('.'), which is reserved for future
> 		  extensions of *bpffs*.
> 
> +	**bpftool link detach** *LINK*
> +		  Force-detach link *LINK*. BPF link and its underlying BPF
> +		  program will stay valid, but they will be detached from the
> +		  respective BPF hook and BPF link will transition into
> +		  a defunct state until last open file descriptor for that

Shall we say "a defunct state when the last open file descriptor for that..."?


> +		  link is closed.
> +
> 	**bpftool link help**
> 		  Print short help message.
> 

[...]

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

* Re: [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand
  2020-07-30 21:02   ` Song Liu
@ 2020-07-30 21:14     ` Andrii Nakryiko
  0 siblings, 0 replies; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-30 21:14 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel,
	Kernel Team, Toke Høiland-Jørgensen

On Thu, Jul 30, 2020 at 2:02 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Add ability to force-detach BPF link. Also add missing error message, if
> > specified link ID is wrong.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> With two nitpicks below.
>
> [...]
>
> > static int link_parse_fd(int *argc, char ***argv)
> > {
> > +     int fd;
> > +
> >       if (is_prefix(**argv, "id")) {
> >               unsigned int id;
> >               char *endptr;
> > @@ -35,7 +37,10 @@ static int link_parse_fd(int *argc, char ***argv)
> >               }
> >               NEXT_ARGP();
> >
> > -             return bpf_link_get_fd_by_id(id);
> > +             fd = bpf_link_get_fd_by_id(id);
> > +             if (fd < 0)
> > +                     p_err("failed to get link with ID %d: %d", id, -errno);
>
> How about we print strerror(errno) to match the rest of link.c?

sure, will do, was lazy :)

>
> [...]
>
> > +static int do_detach(int argc, char **argv)
> > +{
> > +     int err, fd;
> > +
> > +     if (argc != 2)
> > +             return BAD_ARG();
> > +
> > +     fd = link_parse_fd(&argc, &argv);
> > +     if (fd < 0)
> > +             return 1;
> > +
> > +     err = bpf_link_detach(fd);
> > +     if (err)
> > +             err = -errno;
> > +     close(fd);
> > +     if (err) {
> > +             p_err("failed link detach: %d", err);
>
> And strerror(err) here.
>
>

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

* Re: [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach`
  2020-07-30 21:13   ` Song Liu
@ 2020-07-30 21:16     ` Andrii Nakryiko
  2020-07-30 21:50       ` Song Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-07-30 21:16 UTC (permalink / raw)
  To: Song Liu
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel,
	Kernel Team, Toke Høiland-Jørgensen

On Thu, Jul 30, 2020 at 2:13 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Add info on link detach sub-command to man page. Add detach to bash-completion
> > as well.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> With one nitpick below.
>
> > ---
>
> [...]
>
> > @@ -49,6 +50,13 @@ DESCRIPTION
> >                 contain a dot character ('.'), which is reserved for future
> >                 extensions of *bpffs*.
> >
> > +     **bpftool link detach** *LINK*
> > +               Force-detach link *LINK*. BPF link and its underlying BPF
> > +               program will stay valid, but they will be detached from the
> > +               respective BPF hook and BPF link will transition into
> > +               a defunct state until last open file descriptor for that
>
> Shall we say "a defunct state when the last open file descriptor for that..."?


No-no, it is in defunc state between LINK_DETACH and last FD being
closed. Once last FD is closed, BPF link will get destructed and freed
in kernel. So I think until is more precise here?

>
>
> > +               link is closed.
> > +
> >       **bpftool link help**
> >                 Print short help message.
> >
>
> [...]

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

* Re: [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach`
  2020-07-30 21:16     ` Andrii Nakryiko
@ 2020-07-30 21:50       ` Song Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Song Liu @ 2020-07-30 21:50 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, netdev, Alexei Starovoitov, daniel,
	Kernel Team, Toke Høiland-Jørgensen



> On Jul 30, 2020, at 2:16 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> 
> On Thu, Jul 30, 2020 at 2:13 PM Song Liu <songliubraving@fb.com> wrote:
>> 
>> 
>> 
>>> On Jul 29, 2020, at 4:05 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>>> 
>>> Add info on link detach sub-command to man page. Add detach to bash-completion
>>> as well.
>>> 
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>> 
>> Acked-by: Song Liu <songliubraving@fb.com>
>> 
>> With one nitpick below.
>> 
>>> ---
>> 
>> [...]
>> 
>>> @@ -49,6 +50,13 @@ DESCRIPTION
>>>                contain a dot character ('.'), which is reserved for future
>>>                extensions of *bpffs*.
>>> 
>>> +     **bpftool link detach** *LINK*
>>> +               Force-detach link *LINK*. BPF link and its underlying BPF
>>> +               program will stay valid, but they will be detached from the
>>> +               respective BPF hook and BPF link will transition into
>>> +               a defunct state until last open file descriptor for that
>> 
>> Shall we say "a defunct state when the last open file descriptor for that..."?
> 
> 
> No-no, it is in defunc state between LINK_DETACH and last FD being
> closed. Once last FD is closed, BPF link will get destructed and freed
> in kernel. So I think until is more precise here?

Ah, I see. I misunderstood "defunct state". Please ignore the comment. 

Thanks,
Song


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

* Re: [PATCH bpf-next 0/5] BPF link force-detach support
  2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
                   ` (4 preceding siblings ...)
  2020-07-29 23:05 ` [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach` Andrii Nakryiko
@ 2020-08-10 15:01 ` Toke Høiland-Jørgensen
  2020-08-10 18:43   ` Andrii Nakryiko
  5 siblings, 1 reply; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-08-10 15:01 UTC (permalink / raw)
  To: Andrii Nakryiko, bpf, netdev, ast, daniel
  Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko

Andrii Nakryiko <andriin@fb.com> writes:

> This patch set adds new BPF link operation, LINK_DETACH, allowing processes
> with BPF link FD to force-detach it from respective BPF hook, similarly how
> BPF link is auto-detached when such BPF hook (e.g., cgroup, net_device, netns,
> etc) is removed. This facility allows admin to forcefully undo BPF link
> attachment, while process that created BPF link in the first place is left
> intact.
>
> Once force-detached, BPF link stays valid in the kernel as long as there is at
> least one FD open against it. It goes into defunct state, just like
> auto-detached BPF link.
>
> bpftool also got `link detach` command to allow triggering this in
> non-programmatic fashion.

I know this was already merged, but just wanted to add a belated 'thanks
for adding this'!

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

BTW, I've noticed that you tend to drop Ccs on later versions of your
patch series (had to go and lookup v2 of this to check that it was in
fact merged). Is that intentional? :)

-Toke


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

* Re: [PATCH bpf-next 0/5] BPF link force-detach support
  2020-08-10 15:01 ` [PATCH bpf-next 0/5] BPF link force-detach support Toke Høiland-Jørgensen
@ 2020-08-10 18:43   ` Andrii Nakryiko
  2020-08-10 19:03     ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 19+ messages in thread
From: Andrii Nakryiko @ 2020-08-10 18:43 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

On Mon, Aug 10, 2020 at 8:01 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andriin@fb.com> writes:
>
> > This patch set adds new BPF link operation, LINK_DETACH, allowing processes
> > with BPF link FD to force-detach it from respective BPF hook, similarly how
> > BPF link is auto-detached when such BPF hook (e.g., cgroup, net_device, netns,
> > etc) is removed. This facility allows admin to forcefully undo BPF link
> > attachment, while process that created BPF link in the first place is left
> > intact.
> >
> > Once force-detached, BPF link stays valid in the kernel as long as there is at
> > least one FD open against it. It goes into defunct state, just like
> > auto-detached BPF link.
> >
> > bpftool also got `link detach` command to allow triggering this in
> > non-programmatic fashion.
>
> I know this was already merged, but just wanted to add a belated 'thanks
> for adding this'!
>

You are welcome!

> > Cc: Toke Høiland-Jørgensen <toke@redhat.com>
>
> BTW, I've noticed that you tend to drop Ccs on later versions of your
> patch series (had to go and lookup v2 of this to check that it was in
> fact merged). Is that intentional? :)

Hm.. not sure about whether I tend to do that. But in this it was
intentional and I dropped you from CC because I've seen enough
reminders about your vacation, didn't need more ;)

In general, though, I try to keep CC list short, otherwise vger blocks
my patches. People directly CC'd get them, but they never appear on
bpf@vger mailing list. So it probably happened a few times where I
started off with longer CC and had to drop people from it just to get
my patches into patchworks.

>
> -Toke
>

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

* Re: [PATCH bpf-next 0/5] BPF link force-detach support
  2020-08-10 18:43   ` Andrii Nakryiko
@ 2020-08-10 19:03     ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2020-08-10 19:03 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
	Daniel Borkmann, Kernel Team

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

>> BTW, I've noticed that you tend to drop Ccs on later versions of your
>> patch series (had to go and lookup v2 of this to check that it was in
>> fact merged). Is that intentional? :)
>
> Hm.. not sure about whether I tend to do that. But in this it was
> intentional and I dropped you from CC because I've seen enough
> reminders about your vacation, didn't need more ;)

Haha, that's fair ;)

> In general, though, I try to keep CC list short, otherwise vger blocks
> my patches. People directly CC'd get them, but they never appear on
> bpf@vger mailing list. So it probably happened a few times where I
> started off with longer CC and had to drop people from it just to get
> my patches into patchworks.

Ah, I have had that happen to me (patches not showing up on vger), but
had no idea that could be related to a long Cc list. Good to know! And
thanks for the explanation - it's not a huge issue for me (I do
generally keep up with netdev@ and bpf@ although as you can no doubt
imagine I'm a wee bit behind right now), was just wondering...

-Toke


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

end of thread, other threads:[~2020-08-10 19:04 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-29 23:05 [PATCH bpf-next 0/5] BPF link force-detach support Andrii Nakryiko
2020-07-29 23:05 ` [PATCH bpf-next 1/5] bpf: add support for forced LINK_DETACH command Andrii Nakryiko
2020-07-30 17:43   ` Song Liu
2020-07-30 19:03     ` Andrii Nakryiko
2020-07-30 20:53       ` Song Liu
2020-07-29 23:05 ` [PATCH bpf-next 2/5] libbpf: add bpf_link detach APIs Andrii Nakryiko
2020-07-30 17:51   ` Song Liu
2020-07-29 23:05 ` [PATCH bpf-next 3/5] selftests/bpf: add link detach tests for cgroup, netns, and xdp bpf_links Andrii Nakryiko
2020-07-30 20:56   ` Song Liu
2020-07-29 23:05 ` [PATCH bpf-next 4/5] tools/bpftool: add `link detach` subcommand Andrii Nakryiko
2020-07-30 21:02   ` Song Liu
2020-07-30 21:14     ` Andrii Nakryiko
2020-07-29 23:05 ` [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach` Andrii Nakryiko
2020-07-30 21:13   ` Song Liu
2020-07-30 21:16     ` Andrii Nakryiko
2020-07-30 21:50       ` Song Liu
2020-08-10 15:01 ` [PATCH bpf-next 0/5] BPF link force-detach support Toke Høiland-Jørgensen
2020-08-10 18:43   ` Andrii Nakryiko
2020-08-10 19:03     ` 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).