BPF Archive on lore.kernel.org
 help / color / 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
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ 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] 16+ 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
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 16+ 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	[flat|nested] 16+ 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
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 16+ 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	[flat|nested] 16+ 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
  2020-07-29 23:05 ` [PATCH bpf-next 5/5] tools/bpftool: add documentation and bash-completion for `link detach` Andrii Nakryiko
  4 siblings, 1 reply; 16+ 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	[flat|nested] 16+ 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
  4 siblings, 1 reply; 16+ 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	[flat|nested] 16+ 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
  4 siblings, 1 reply; 16+ 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	[flat|nested] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ 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; 16+ 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] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ 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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git