[PATCH bpf-next v2 0/6] Notify user space when a struct

BPF List
 help / color / mirror / Atom feed

* [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered
@ 2024-05-07  5:55 Kui-Feng Lee
  2024-05-07  5:55 ` [PATCH bpf-next v2 1/6] bpf: pass bpf_struct_ops_link to callbacks in bpf_struct_ops Kui-Feng Lee
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:55 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

The subsystems managing struct_ops objects may need to detach a
struct_ops object due to errors or other reasons. It would be useful
to notify user space programs so that error recovery or logging can be
carried out.

This patch set enables the detach feature for struct_ops links and
send an event to epoll when a link is detached.  Subsystems could call
link->ops->detach() to detach a link and notify user space programs
through epoll.

The signatures of callback functions in "struct bpf_struct_ops" have
been changed as well to pass an extra link argument to
subsystems. Subsystems could detach the links received from reg() and
update() callbacks if there is. This also provides a way that
subsystems can distinguish registrations for an object that has been
registered multiple times for several links.

However, bpf struct_ops maps without BPF_F_LINK have no any link.
Subsystems will receive NULL link pointer for this case.

---
Changes from v1:

 - Pass a link to reg, unreg, and update callbacks.

 - Provide a function to detach a link from underlying subsystems.

 - Add a kfunc to minic detachments from subsystems, and provide a
   flexible way to control when to do detachments.

 - Add two tests to detach a link from the subsystem after the refcount
   of the link drops to zero.

v1: https://lore.kernel.org/all/20240429213609.487820-1-thinker.li@gmail.com/

Kui-Feng Lee (6):
  bpf: pass bpf_struct_ops_link to callbacks in bpf_struct_ops.
  bpf: enable detaching links of struct_ops objects.
  bpf: support epoll from bpf struct_ops links.
  selftests/bpf: test struct_ops with epoll
  selftests/bpf: detach a struct_ops link from the subsystem managing
    it.
  selftests/bpf: make sure bpf_testmod handling racing link destroying
    well.

 include/linux/bpf.h                           |   7 +-
 kernel/bpf/bpf_struct_ops.c                   |  77 ++++++--
 kernel/bpf/syscall.c                          |  11 ++
 net/bpf/bpf_dummy_struct_ops.c                |   4 +-
 net/ipv4/bpf_tcp_ca.c                         |   6 +-
 .../bpf/bpf_test_no_cfi/bpf_test_no_cfi.c     |   4 +-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  40 ++++-
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |   1 +
 .../bpf/prog_tests/test_struct_ops_module.c   | 166 ++++++++++++++++++
 .../selftests/bpf/progs/struct_ops_detach.c   |  37 ++++
 10 files changed, 329 insertions(+), 24 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_detach.c

-- 
2.34.1

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

* [PATCH bpf-next v2 1/6] bpf: pass bpf_struct_ops_link to callbacks in bpf_struct_ops.
  2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
@ 2024-05-07  5:55 ` Kui-Feng Lee
  2024-05-07  5:55 ` [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects Kui-Feng Lee
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:55 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

Pass an additional pointer of bpf_struct_ops_link to callback function reg,
unreg, and update provided by subsystems defined in bpf_struct_ops. A
bpf_struct_ops_map can be registered for multiple links. Passing a pointer
of bpf_struct_ops_link helps subsystems to distinguish them.

This pointer will be used in the later patches to let the subsystem
initiate a detachment on a link that was registered to it previously.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h                                    |  6 +++---
 kernel/bpf/bpf_struct_ops.c                            | 10 +++++-----
 net/bpf/bpf_dummy_struct_ops.c                         |  4 ++--
 net/ipv4/bpf_tcp_ca.c                                  |  6 +++---
 .../selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c    |  4 ++--
 tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c  |  6 +++---
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 90094400cc63..b600767ebe02 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1730,9 +1730,9 @@ struct bpf_struct_ops {
 	int (*init_member)(const struct btf_type *t,
 			   const struct btf_member *member,
 			   void *kdata, const void *udata);
-	int (*reg)(void *kdata);
-	void (*unreg)(void *kdata);
-	int (*update)(void *kdata, void *old_kdata);
+	int (*reg)(void *kdata, struct bpf_link *link);
+	void (*unreg)(void *kdata, struct bpf_link *link);
+	int (*update)(void *kdata, void *old_kdata, struct bpf_link *link);
 	int (*validate)(void *kdata);
 	void *cfi_stubs;
 	struct module *owner;
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 86c7884abaf8..390f8c155135 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -757,7 +757,7 @@ static long bpf_struct_ops_map_update_elem(struct bpf_map *map, void *key,
 		goto unlock;
 	}
 
-	err = st_ops->reg(kdata);
+	err = st_ops->reg(kdata, NULL);
 	if (likely(!err)) {
 		/* This refcnt increment on the map here after
 		 * 'st_ops->reg()' is secure since the state of the
@@ -805,7 +805,7 @@ static long bpf_struct_ops_map_delete_elem(struct bpf_map *map, void *key)
 			     BPF_STRUCT_OPS_STATE_TOBEFREE);
 	switch (prev_state) {
 	case BPF_STRUCT_OPS_STATE_INUSE:
-		st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data);
+		st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, NULL);
 		bpf_map_put(map);
 		return 0;
 	case BPF_STRUCT_OPS_STATE_TOBEFREE:
@@ -1060,7 +1060,7 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
 		/* st_link->map can be NULL if
 		 * bpf_struct_ops_link_create() fails to register.
 		 */
-		st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data);
+		st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, st_link);
 		bpf_map_put(&st_map->map);
 	}
 	kfree(st_link);
@@ -1125,7 +1125,7 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
 		goto err_out;
 	}
 
-	err = st_map->st_ops_desc->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data);
+	err = st_map->st_ops_desc->st_ops->update(st_map->kvalue.data, old_st_map->kvalue.data, link);
 	if (err)
 		goto err_out;
 
@@ -1176,7 +1176,7 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
 	if (err)
 		goto err_out;
 
-	err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data);
+	err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
 	if (err) {
 		bpf_link_cleanup(&link_primer);
 		link = NULL;
diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c
index 891cdf61c65a..3ea52b05adfb 100644
--- a/net/bpf/bpf_dummy_struct_ops.c
+++ b/net/bpf/bpf_dummy_struct_ops.c
@@ -272,12 +272,12 @@ static int bpf_dummy_init_member(const struct btf_type *t,
 	return -EOPNOTSUPP;
 }
 
-static int bpf_dummy_reg(void *kdata)
+static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
 {
 	return -EOPNOTSUPP;
 }
 
-static void bpf_dummy_unreg(void *kdata)
+static void bpf_dummy_unreg(void *kdata, struct bpf_link *link)
 {
 }
 
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 7f518ea5f4ac..dd97f7ebcd29 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -257,17 +257,17 @@ static int bpf_tcp_ca_check_member(const struct btf_type *t,
 	return 0;
 }
 
-static int bpf_tcp_ca_reg(void *kdata)
+static int bpf_tcp_ca_reg(void *kdata, struct bpf_link *link)
 {
 	return tcp_register_congestion_control(kdata);
 }
 
-static void bpf_tcp_ca_unreg(void *kdata)
+static void bpf_tcp_ca_unreg(void *kdata, struct bpf_link *link)
 {
 	tcp_unregister_congestion_control(kdata);
 }
 
-static int bpf_tcp_ca_update(void *kdata, void *old_kdata)
+static int bpf_tcp_ca_update(void *kdata, void *old_kdata, struct bpf_link *link)
 {
 	return tcp_update_congestion_control(kdata, old_kdata);
 }
diff --git a/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c b/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
index b1dd889d5d7d..948eb3962732 100644
--- a/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
+++ b/tools/testing/selftests/bpf/bpf_test_no_cfi/bpf_test_no_cfi.c
@@ -22,12 +22,12 @@ static int dummy_init_member(const struct btf_type *t,
 	return 0;
 }
 
-static int dummy_reg(void *kdata)
+static int dummy_reg(void *kdata, struct bpf_link *link)
 {
 	return 0;
 }
 
-static void dummy_unreg(void *kdata)
+static void dummy_unreg(void *kdata, struct bpf_link *link)
 {
 }
 
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index eb2b78552ca2..e24a18bfee14 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -565,7 +565,7 @@ static const struct bpf_verifier_ops bpf_testmod_verifier_ops = {
 	.is_valid_access = bpf_testmod_ops_is_valid_access,
 };
 
-static int bpf_dummy_reg(void *kdata)
+static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
 {
 	struct bpf_testmod_ops *ops = kdata;
 
@@ -580,7 +580,7 @@ static int bpf_dummy_reg(void *kdata)
 	return 0;
 }
 
-static void bpf_dummy_unreg(void *kdata)
+static void bpf_dummy_unreg(void *kdata, struct bpf_link *link)
 {
 }
 
@@ -616,7 +616,7 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
 	.owner = THIS_MODULE,
 };
 
-static int bpf_dummy_reg2(void *kdata)
+static int bpf_dummy_reg2(void *kdata, struct bpf_link *link)
 {
 	struct bpf_testmod_ops2 *ops = kdata;
 
-- 
2.34.1


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

* [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.
  2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
  2024-05-07  5:55 ` [PATCH bpf-next v2 1/6] bpf: pass bpf_struct_ops_link to callbacks in bpf_struct_ops Kui-Feng Lee
@ 2024-05-07  5:55 ` Kui-Feng Lee
  2024-05-08 23:22   ` Martin KaFai Lau
  2024-05-07  5:55 ` [PATCH bpf-next v2 3/6] bpf: support epoll from bpf struct_ops links Kui-Feng Lee
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:55 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

Implement the detach callback in bpf_link_ops for struct_ops. The
subsystems that struct_ops objects are registered to can use this callback
to detach the links being passed to them.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 kernel/bpf/bpf_struct_ops.c | 50 ++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 390f8c155135..bd2602982e4d 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -1057,9 +1057,6 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
 	st_map = (struct bpf_struct_ops_map *)
 		rcu_dereference_protected(st_link->map, true);
 	if (st_map) {
-		/* st_link->map can be NULL if
-		 * bpf_struct_ops_link_create() fails to register.
-		 */
 		st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, st_link);
 		bpf_map_put(&st_map->map);
 	}
@@ -1075,7 +1072,8 @@ static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
 	st_link = container_of(link, struct bpf_struct_ops_link, link);
 	rcu_read_lock();
 	map = rcu_dereference(st_link->map);
-	seq_printf(seq, "map_id:\t%d\n", map->id);
+	if (map)
+		seq_printf(seq, "map_id:\t%d\n", map->id);
 	rcu_read_unlock();
 }
 
@@ -1088,7 +1086,8 @@ static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
 	st_link = container_of(link, struct bpf_struct_ops_link, link);
 	rcu_read_lock();
 	map = rcu_dereference(st_link->map);
-	info->struct_ops.map_id = map->id;
+	if (map)
+		info->struct_ops.map_id = map->id;
 	rcu_read_unlock();
 	return 0;
 }
@@ -1113,6 +1112,10 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
 	mutex_lock(&update_mutex);
 
 	old_map = rcu_dereference_protected(st_link->map, lockdep_is_held(&update_mutex));
+	if (!old_map) {
+		err = -EINVAL;
+		goto err_out;
+	}
 	if (expected_old_map && old_map != expected_old_map) {
 		err = -EPERM;
 		goto err_out;
@@ -1139,8 +1142,37 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
 	return err;
 }
 
+static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
+{
+	struct bpf_struct_ops_link *st_link = container_of(link, struct bpf_struct_ops_link, link);
+	struct bpf_struct_ops_map *st_map;
+	struct bpf_map *map;
+
+	mutex_lock(&update_mutex);
+
+	map = rcu_dereference_protected(st_link->map, true);
+	if (!map) {
+		mutex_unlock(&update_mutex);
+		return -EINVAL;
+	}
+	st_map = container_of(map, struct bpf_struct_ops_map, map);
+
+	st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
+
+	rcu_assign_pointer(st_link->map, NULL);
+	/* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
+	 * bpf_map_inc() in bpf_struct_ops_map_link_update().
+	 */
+	bpf_map_put(&st_map->map);
+
+	mutex_unlock(&update_mutex);
+
+	return 0;
+}
+
 static const struct bpf_link_ops bpf_struct_ops_map_lops = {
 	.dealloc = bpf_struct_ops_map_link_dealloc,
+	.detach = bpf_struct_ops_map_link_detach,
 	.show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
 	.fill_link_info = bpf_struct_ops_map_link_fill_link_info,
 	.update_map = bpf_struct_ops_map_link_update,
@@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
 	if (err)
 		goto err_out;
 
+	/* Init link->map before calling reg() in case being detached
+	 * immediately.
+	 */
+	RCU_INIT_POINTER(link->map, map);
+
 	err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
 	if (err) {
+		rcu_assign_pointer(link->map, NULL);
 		bpf_link_cleanup(&link_primer);
+		/* The link has been free by bpf_link_cleanup() */
 		link = NULL;
 		goto err_out;
 	}
-	RCU_INIT_POINTER(link->map, map);
 
 	return bpf_link_settle(&link_primer);
 
-- 
2.34.1


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

* Re: [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.
  2024-05-07  5:55 ` [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects Kui-Feng Lee
@ 2024-05-08 23:22   ` Martin KaFai Lau
  2024-05-09  0:14     ` Kui-Feng Lee
  0 siblings, 1 reply; 18+ messages in thread
From: Martin KaFai Lau @ 2024-05-08 23:22 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii

On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
> Implement the detach callback in bpf_link_ops for struct_ops. The
> subsystems that struct_ops objects are registered to can use this callback
> to detach the links being passed to them.

The user space can also use the detach. The subsystem is merely reusing the 
similar detach callback if it stores the link during reg().

> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   kernel/bpf/bpf_struct_ops.c | 50 ++++++++++++++++++++++++++++++++-----
>   1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index 390f8c155135..bd2602982e4d 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -1057,9 +1057,6 @@ static void bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
>   	st_map = (struct bpf_struct_ops_map *)
>   		rcu_dereference_protected(st_link->map, true);
>   	if (st_map) {
> -		/* st_link->map can be NULL if
> -		 * bpf_struct_ops_link_create() fails to register.
> -		 */
>   		st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, st_link);
>   		bpf_map_put(&st_map->map);
>   	}
> @@ -1075,7 +1072,8 @@ static void bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
>   	st_link = container_of(link, struct bpf_struct_ops_link, link);
>   	rcu_read_lock();
>   	map = rcu_dereference(st_link->map);
> -	seq_printf(seq, "map_id:\t%d\n", map->id);
> +	if (map)
> +		seq_printf(seq, "map_id:\t%d\n", map->id);
>   	rcu_read_unlock();
>   }
>   
> @@ -1088,7 +1086,8 @@ static int bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
>   	st_link = container_of(link, struct bpf_struct_ops_link, link);
>   	rcu_read_lock();
>   	map = rcu_dereference(st_link->map);
> -	info->struct_ops.map_id = map->id;
> +	if (map)
> +		info->struct_ops.map_id = map->id;
>   	rcu_read_unlock();
>   	return 0;
>   }
> @@ -1113,6 +1112,10 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>   	mutex_lock(&update_mutex);
>   
>   	old_map = rcu_dereference_protected(st_link->map, lockdep_is_held(&update_mutex));
> +	if (!old_map) {
> +		err = -EINVAL;
> +		goto err_out;
> +	}
>   	if (expected_old_map && old_map != expected_old_map) {
>   		err = -EPERM;
>   		goto err_out;
> @@ -1139,8 +1142,37 @@ static int bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>   	return err;
>   }
>   
> +static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
> +{
> +	struct bpf_struct_ops_link *st_link = container_of(link, struct bpf_struct_ops_link, link);
> +	struct bpf_struct_ops_map *st_map;
> +	struct bpf_map *map;
> +
> +	mutex_lock(&update_mutex);
> +
> +	map = rcu_dereference_protected(st_link->map, true);

nit. s/true/lockdep_is_held(&update_mutex)/

> +	if (!map) {
> +		mutex_unlock(&update_mutex);
> +		return -EINVAL;
> +	}
> +	st_map = container_of(map, struct bpf_struct_ops_map, map);
> +
> +	st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
> +
> +	rcu_assign_pointer(st_link->map, NULL);
> +	/* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
> +	 * bpf_map_inc() in bpf_struct_ops_map_link_update().
> +	 */
> +	bpf_map_put(&st_map->map);
> +
> +	mutex_unlock(&update_mutex);
> +
> +	return 0;
> +}
> +
>   static const struct bpf_link_ops bpf_struct_ops_map_lops = {
>   	.dealloc = bpf_struct_ops_map_link_dealloc,
> +	.detach = bpf_struct_ops_map_link_detach,
>   	.show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
>   	.fill_link_info = bpf_struct_ops_map_link_fill_link_info,
>   	.update_map = bpf_struct_ops_map_link_update,
> @@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>   	if (err)
>   		goto err_out;
>   
> +	/* Init link->map before calling reg() in case being detached
> +	 * immediately.
> +	 */
> +	RCU_INIT_POINTER(link->map, map);
> +
>   	err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
>   	if (err) {
> +		rcu_assign_pointer(link->map, NULL);

nit. RCU_INIT_POINTER(link->map, NULL) is fine.

There is a merge conflict with patch 4 also.

pw-bot: cr

>   		bpf_link_cleanup(&link_primer);
> +		/* The link has been free by bpf_link_cleanup() */
>   		link = NULL;
>   		goto err_out;
>   	}
> -	RCU_INIT_POINTER(link->map, map);
>   
>   	return bpf_link_settle(&link_primer);
>   


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

* Re: [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.
  2024-05-08 23:22   ` Martin KaFai Lau
@ 2024-05-09  0:14     ` Kui-Feng Lee
  2024-05-09  0:36       ` Martin KaFai Lau
  2024-05-09  0:46       ` Kui-Feng Lee
  0 siblings, 2 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-09  0:14 UTC (permalink / raw)
  To: Martin KaFai Lau, Kui-Feng Lee
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii



On 5/8/24 16:22, Martin KaFai Lau wrote:
> On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
>> Implement the detach callback in bpf_link_ops for struct_ops. The
>> subsystems that struct_ops objects are registered to can use this 
>> callback
>> to detach the links being passed to them.
> 
> The user space can also use the detach. The subsystem is merely reusing 
> the similar detach callback if it stores the link during reg().

Sure!

> 
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   kernel/bpf/bpf_struct_ops.c | 50 ++++++++++++++++++++++++++++++++-----
>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>> index 390f8c155135..bd2602982e4d 100644
>> --- a/kernel/bpf/bpf_struct_ops.c
>> +++ b/kernel/bpf/bpf_struct_ops.c
>> @@ -1057,9 +1057,6 @@ static void 
>> bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
>>       st_map = (struct bpf_struct_ops_map *)
>>           rcu_dereference_protected(st_link->map, true);
>>       if (st_map) {
>> -        /* st_link->map can be NULL if
>> -         * bpf_struct_ops_link_create() fails to register.
>> -         */
>>           st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, 
>> st_link);
>>           bpf_map_put(&st_map->map);
>>       }
>> @@ -1075,7 +1072,8 @@ static void 
>> bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>       rcu_read_lock();
>>       map = rcu_dereference(st_link->map);
>> -    seq_printf(seq, "map_id:\t%d\n", map->id);
>> +    if (map)
>> +        seq_printf(seq, "map_id:\t%d\n", map->id);
>>       rcu_read_unlock();
>>   }
>> @@ -1088,7 +1086,8 @@ static int 
>> bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>       rcu_read_lock();
>>       map = rcu_dereference(st_link->map);
>> -    info->struct_ops.map_id = map->id;
>> +    if (map)
>> +        info->struct_ops.map_id = map->id;
>>       rcu_read_unlock();
>>       return 0;
>>   }
>> @@ -1113,6 +1112,10 @@ static int 
>> bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>>       mutex_lock(&update_mutex);
>>       old_map = rcu_dereference_protected(st_link->map, 
>> lockdep_is_held(&update_mutex));
>> +    if (!old_map) {
>> +        err = -EINVAL;
>> +        goto err_out;
>> +    }
>>       if (expected_old_map && old_map != expected_old_map) {
>>           err = -EPERM;
>>           goto err_out;
>> @@ -1139,8 +1142,37 @@ static int 
>> bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>>       return err;
>>   }
>> +static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
>> +{
>> +    struct bpf_struct_ops_link *st_link = container_of(link, struct 
>> bpf_struct_ops_link, link);
>> +    struct bpf_struct_ops_map *st_map;
>> +    struct bpf_map *map;
>> +
>> +    mutex_lock(&update_mutex);
>> +
>> +    map = rcu_dereference_protected(st_link->map, true);
> 
> nit. s/true/lockdep_is_held(&update_mutex)/


I thought it is protected by the refcount holding by the caller.
WDYT?


> 
>> +    if (!map) {
>> +        mutex_unlock(&update_mutex);
>> +        return -EINVAL;
>> +    }
>> +    st_map = container_of(map, struct bpf_struct_ops_map, map);
>> +
>> +    st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
>> +
>> +    rcu_assign_pointer(st_link->map, NULL);
>> +    /* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
>> +     * bpf_map_inc() in bpf_struct_ops_map_link_update().
>> +     */
>> +    bpf_map_put(&st_map->map);
>> +
>> +    mutex_unlock(&update_mutex);
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct bpf_link_ops bpf_struct_ops_map_lops = {
>>       .dealloc = bpf_struct_ops_map_link_dealloc,
>> +    .detach = bpf_struct_ops_map_link_detach,
>>       .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
>>       .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
>>       .update_map = bpf_struct_ops_map_link_update,
>> @@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union bpf_attr 
>> *attr)
>>       if (err)
>>           goto err_out;
>> +    /* Init link->map before calling reg() in case being detached
>> +     * immediately.
>> +     */
>> +    RCU_INIT_POINTER(link->map, map);
>> +
>>       err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, 
>> &link->link);
>>       if (err) {
>> +        rcu_assign_pointer(link->map, NULL);
> 
> nit. RCU_INIT_POINTER(link->map, NULL) is fine.

Got it!

> 
> There is a merge conflict with patch 4 also.

What do you mean here? Do you mean the patch 4 can not be applied on top
of the patch 2?

> 
> pw-bot: cr
> 
>>           bpf_link_cleanup(&link_primer);
>> +        /* The link has been free by bpf_link_cleanup() */
>>           link = NULL;
>>           goto err_out;
>>       }
>> -    RCU_INIT_POINTER(link->map, map);
>>       return bpf_link_settle(&link_primer);
> 

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

* Re: [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.
  2024-05-09  0:14     ` Kui-Feng Lee
@ 2024-05-09  0:36       ` Martin KaFai Lau
  2024-05-09 16:59         ` Kui-Feng Lee
  2024-05-09  0:46       ` Kui-Feng Lee
  1 sibling, 1 reply; 18+ messages in thread
From: Martin KaFai Lau @ 2024-05-09  0:36 UTC (permalink / raw)
  To: Kui-Feng Lee, Kui-Feng Lee; +Cc: kuifeng, bpf, ast, song, kernel-team, andrii

On 5/8/24 5:14 PM, Kui-Feng Lee wrote:
> 
> 
> On 5/8/24 16:22, Martin KaFai Lau wrote:
>> On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
>>> Implement the detach callback in bpf_link_ops for struct_ops. The
>>> subsystems that struct_ops objects are registered to can use this callback
>>> to detach the links being passed to them.
>>
>> The user space can also use the detach. The subsystem is merely reusing the 
>> similar detach callback if it stores the link during reg().
> 
> Sure!
> 
>>
>>>
>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>> ---
>>>   kernel/bpf/bpf_struct_ops.c | 50 ++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index 390f8c155135..bd2602982e4d 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -1057,9 +1057,6 @@ static void bpf_struct_ops_map_link_dealloc(struct 
>>> bpf_link *link)
>>>       st_map = (struct bpf_struct_ops_map *)
>>>           rcu_dereference_protected(st_link->map, true);
>>>       if (st_map) {
>>> -        /* st_link->map can be NULL if
>>> -         * bpf_struct_ops_link_create() fails to register.
>>> -         */
>>>           st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, st_link);
>>>           bpf_map_put(&st_map->map);
>>>       }
>>> @@ -1075,7 +1072,8 @@ static void bpf_struct_ops_map_link_show_fdinfo(const 
>>> struct bpf_link *link,
>>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>>       rcu_read_lock();
>>>       map = rcu_dereference(st_link->map);
>>> -    seq_printf(seq, "map_id:\t%d\n", map->id);
>>> +    if (map)
>>> +        seq_printf(seq, "map_id:\t%d\n", map->id);
>>>       rcu_read_unlock();
>>>   }
>>> @@ -1088,7 +1086,8 @@ static int bpf_struct_ops_map_link_fill_link_info(const 
>>> struct bpf_link *link,
>>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>>       rcu_read_lock();
>>>       map = rcu_dereference(st_link->map);
>>> -    info->struct_ops.map_id = map->id;
>>> +    if (map)
>>> +        info->struct_ops.map_id = map->id;
>>>       rcu_read_unlock();
>>>       return 0;
>>>   }
>>> @@ -1113,6 +1112,10 @@ static int bpf_struct_ops_map_link_update(struct 
>>> bpf_link *link, struct bpf_map
>>>       mutex_lock(&update_mutex);
>>>       old_map = rcu_dereference_protected(st_link->map, 
>>> lockdep_is_held(&update_mutex));
>>> +    if (!old_map) {
>>> +        err = -EINVAL;
>>> +        goto err_out;
>>> +    }
>>>       if (expected_old_map && old_map != expected_old_map) {
>>>           err = -EPERM;
>>>           goto err_out;
>>> @@ -1139,8 +1142,37 @@ static int bpf_struct_ops_map_link_update(struct 
>>> bpf_link *link, struct bpf_map
>>>       return err;
>>>   }
>>> +static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
>>> +{
>>> +    struct bpf_struct_ops_link *st_link = container_of(link, struct 
>>> bpf_struct_ops_link, link);
>>> +    struct bpf_struct_ops_map *st_map;
>>> +    struct bpf_map *map;
>>> +
>>> +    mutex_lock(&update_mutex);
>>> +
>>> +    map = rcu_dereference_protected(st_link->map, true);
>>
>> nit. s/true/lockdep_is_held(&update_mutex)/
> 
> 
> I thought it is protected by the refcount holding by the caller.
> WDYT?

st_link->map is the one with __rcu tag and "!map" is tested next. I don't see 
how these imply the map pointer is protected by refcount. Can you explain?

> 
> 
>>
>>> +    if (!map) {
>>> +        mutex_unlock(&update_mutex);
>>> +        return -EINVAL;
>>> +    }
>>> +    st_map = container_of(map, struct bpf_struct_ops_map, map);
>>> +
>>> +    st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
>>> +
>>> +    rcu_assign_pointer(st_link->map, NULL);
>>> +    /* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
>>> +     * bpf_map_inc() in bpf_struct_ops_map_link_update().
>>> +     */
>>> +    bpf_map_put(&st_map->map);
>>> +
>>> +    mutex_unlock(&update_mutex);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static const struct bpf_link_ops bpf_struct_ops_map_lops = {
>>>       .dealloc = bpf_struct_ops_map_link_dealloc,
>>> +    .detach = bpf_struct_ops_map_link_detach,
>>>       .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
>>>       .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
>>>       .update_map = bpf_struct_ops_map_link_update,
>>> @@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>>>       if (err)
>>>           goto err_out;
>>> +    /* Init link->map before calling reg() in case being detached
>>> +     * immediately.
>>> +     */
>>> +    RCU_INIT_POINTER(link->map, map);
>>> +
>>>       err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
>>>       if (err) {
>>> +        rcu_assign_pointer(link->map, NULL);
>>
>> nit. RCU_INIT_POINTER(link->map, NULL) is fine.
> 
> Got it!
> 
>>
>> There is a merge conflict with patch 4 also.
> 
> What do you mean here? Do you mean the patch 4 can not be applied on top
> of the patch 2?

Please monitor the bpf CI report.

bpf CI complains: 
https://patchwork.kernel.org/project/netdevbpf/patch/20240507055600.2382627-2-thinker.li@gmail.com/

snippet of the error:

Applying: bpf: enable detaching links of struct_ops objects.
Applying: bpf: support epoll from bpf struct_ops links.
Applying: selftests/bpf: test struct_ops with epoll
Patch failed at 0004 selftests/bpf: test struct_ops with epoll

> 
>>
>> pw-bot: cr
>>
>>>           bpf_link_cleanup(&link_primer);
>>> +        /* The link has been free by bpf_link_cleanup() */
>>>           link = NULL;
>>>           goto err_out;
>>>       }
>>> -    RCU_INIT_POINTER(link->map, map);
>>>       return bpf_link_settle(&link_primer);
>>


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

* Re: [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.
  2024-05-09  0:36       ` Martin KaFai Lau
@ 2024-05-09 16:59         ` Kui-Feng Lee
  0 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-09 16:59 UTC (permalink / raw)
  To: Martin KaFai Lau, Kui-Feng Lee
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii



On 5/8/24 17:36, Martin KaFai Lau wrote:
> On 5/8/24 5:14 PM, Kui-Feng Lee wrote:
>>
>>
>> On 5/8/24 16:22, Martin KaFai Lau wrote:
>>> On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
>>>> Implement the detach callback in bpf_link_ops for struct_ops. The
>>>> subsystems that struct_ops objects are registered to can use this 
>>>> callback
>>>> to detach the links being passed to them.
>>>
>>> The user space can also use the detach. The subsystem is merely 
>>> reusing the similar detach callback if it stores the link during reg().
>>
>> Sure!
>>
>>>
>>>>
>>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>>> ---
>>>>   kernel/bpf/bpf_struct_ops.c | 50 
>>>> ++++++++++++++++++++++++++++++++-----
>>>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>>> index 390f8c155135..bd2602982e4d 100644
>>>> --- a/kernel/bpf/bpf_struct_ops.c
>>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>>> @@ -1057,9 +1057,6 @@ static void 
>>>> bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
>>>>       st_map = (struct bpf_struct_ops_map *)
>>>>           rcu_dereference_protected(st_link->map, true);
>>>>       if (st_map) {
>>>> -        /* st_link->map can be NULL if
>>>> -         * bpf_struct_ops_link_create() fails to register.
>>>> -         */
>>>>           st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, 
>>>> st_link);
>>>>           bpf_map_put(&st_map->map);
>>>>       }
>>>> @@ -1075,7 +1072,8 @@ static void 
>>>> bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
>>>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>>>       rcu_read_lock();
>>>>       map = rcu_dereference(st_link->map);
>>>> -    seq_printf(seq, "map_id:\t%d\n", map->id);
>>>> +    if (map)
>>>> +        seq_printf(seq, "map_id:\t%d\n", map->id);
>>>>       rcu_read_unlock();
>>>>   }
>>>> @@ -1088,7 +1086,8 @@ static int 
>>>> bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
>>>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>>>       rcu_read_lock();
>>>>       map = rcu_dereference(st_link->map);
>>>> -    info->struct_ops.map_id = map->id;
>>>> +    if (map)
>>>> +        info->struct_ops.map_id = map->id;
>>>>       rcu_read_unlock();
>>>>       return 0;
>>>>   }
>>>> @@ -1113,6 +1112,10 @@ static int 
>>>> bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>>>>       mutex_lock(&update_mutex);
>>>>       old_map = rcu_dereference_protected(st_link->map, 
>>>> lockdep_is_held(&update_mutex));
>>>> +    if (!old_map) {
>>>> +        err = -EINVAL;
>>>> +        goto err_out;
>>>> +    }
>>>>       if (expected_old_map && old_map != expected_old_map) {
>>>>           err = -EPERM;
>>>>           goto err_out;
>>>> @@ -1139,8 +1142,37 @@ static int 
>>>> bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>>>>       return err;
>>>>   }
>>>> +static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
>>>> +{
>>>> +    struct bpf_struct_ops_link *st_link = container_of(link, struct 
>>>> bpf_struct_ops_link, link);
>>>> +    struct bpf_struct_ops_map *st_map;
>>>> +    struct bpf_map *map;
>>>> +
>>>> +    mutex_lock(&update_mutex);
>>>> +
>>>> +    map = rcu_dereference_protected(st_link->map, true);
>>>
>>> nit. s/true/lockdep_is_held(&update_mutex)/
>>
>>
>> I thought it is protected by the refcount holding by the caller.
>> WDYT?
> 
> st_link->map is the one with __rcu tag and "!map" is tested next. I 
> don't see how these imply the map pointer is protected by refcount. Can 
> you explain?
> 
Ok! You are right. I confused links with maps.

>>
>>
>>>
>>>> +    if (!map) {
>>>> +        mutex_unlock(&update_mutex);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    st_map = container_of(map, struct bpf_struct_ops_map, map);
>>>> +
>>>> +    st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
>>>> +
>>>> +    rcu_assign_pointer(st_link->map, NULL);
>>>> +    /* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
>>>> +     * bpf_map_inc() in bpf_struct_ops_map_link_update().
>>>> +     */
>>>> +    bpf_map_put(&st_map->map);
>>>> +
>>>> +    mutex_unlock(&update_mutex);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static const struct bpf_link_ops bpf_struct_ops_map_lops = {
>>>>       .dealloc = bpf_struct_ops_map_link_dealloc,
>>>> +    .detach = bpf_struct_ops_map_link_detach,
>>>>       .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
>>>>       .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
>>>>       .update_map = bpf_struct_ops_map_link_update,
>>>> @@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union 
>>>> bpf_attr *attr)
>>>>       if (err)
>>>>           goto err_out;
>>>> +    /* Init link->map before calling reg() in case being detached
>>>> +     * immediately.
>>>> +     */
>>>> +    RCU_INIT_POINTER(link->map, map);
>>>> +
>>>>       err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, 
>>>> &link->link);
>>>>       if (err) {
>>>> +        rcu_assign_pointer(link->map, NULL);
>>>
>>> nit. RCU_INIT_POINTER(link->map, NULL) is fine.
>>
>> Got it!
>>
>>>
>>> There is a merge conflict with patch 4 also.
>>
>> What do you mean here? Do you mean the patch 4 can not be applied on top
>> of the patch 2?
> 
> Please monitor the bpf CI report.
> 
> bpf CI complains: 
> https://patchwork.kernel.org/project/netdevbpf/patch/20240507055600.2382627-2-thinker.li@gmail.com/
> 
> snippet of the error:
> 
> Applying: bpf: enable detaching links of struct_ops objects.
> Applying: bpf: support epoll from bpf struct_ops links.
> Applying: selftests/bpf: test struct_ops with epoll
> Patch failed at 0004 selftests/bpf: test struct_ops with epoll

Yes! I found it when I rebased local repository.

> 
>>
>>>
>>> pw-bot: cr
>>>
>>>>           bpf_link_cleanup(&link_primer);
>>>> +        /* The link has been free by bpf_link_cleanup() */
>>>>           link = NULL;
>>>>           goto err_out;
>>>>       }
>>>> -    RCU_INIT_POINTER(link->map, map);
>>>>       return bpf_link_settle(&link_primer);
>>>
> 

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

* Re: [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects.
  2024-05-09  0:14     ` Kui-Feng Lee
  2024-05-09  0:36       ` Martin KaFai Lau
@ 2024-05-09  0:46       ` Kui-Feng Lee
  1 sibling, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-09  0:46 UTC (permalink / raw)
  To: Martin KaFai Lau, Kui-Feng Lee
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii



On 5/8/24 17:14, Kui-Feng Lee wrote:
> 
> 
> On 5/8/24 16:22, Martin KaFai Lau wrote:
>> On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
>>> Implement the detach callback in bpf_link_ops for struct_ops. The
>>> subsystems that struct_ops objects are registered to can use this 
>>> callback
>>> to detach the links being passed to them.
>>
>> The user space can also use the detach. The subsystem is merely 
>> reusing the similar detach callback if it stores the link during reg().
> 
> Sure!
> 
>>
>>>
>>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>>> ---
>>>   kernel/bpf/bpf_struct_ops.c | 50 ++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 44 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
>>> index 390f8c155135..bd2602982e4d 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -1057,9 +1057,6 @@ static void 
>>> bpf_struct_ops_map_link_dealloc(struct bpf_link *link)
>>>       st_map = (struct bpf_struct_ops_map *)
>>>           rcu_dereference_protected(st_link->map, true);
>>>       if (st_map) {
>>> -        /* st_link->map can be NULL if
>>> -         * bpf_struct_ops_link_create() fails to register.
>>> -         */
>>>           st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, 
>>> st_link);
>>>           bpf_map_put(&st_map->map);
>>>       }
>>> @@ -1075,7 +1072,8 @@ static void 
>>> bpf_struct_ops_map_link_show_fdinfo(const struct bpf_link *link,
>>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>>       rcu_read_lock();
>>>       map = rcu_dereference(st_link->map);
>>> -    seq_printf(seq, "map_id:\t%d\n", map->id);
>>> +    if (map)
>>> +        seq_printf(seq, "map_id:\t%d\n", map->id);
>>>       rcu_read_unlock();
>>>   }
>>> @@ -1088,7 +1086,8 @@ static int 
>>> bpf_struct_ops_map_link_fill_link_info(const struct bpf_link *link,
>>>       st_link = container_of(link, struct bpf_struct_ops_link, link);
>>>       rcu_read_lock();
>>>       map = rcu_dereference(st_link->map);
>>> -    info->struct_ops.map_id = map->id;
>>> +    if (map)
>>> +        info->struct_ops.map_id = map->id;
>>>       rcu_read_unlock();
>>>       return 0;
>>>   }
>>> @@ -1113,6 +1112,10 @@ static int 
>>> bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>>>       mutex_lock(&update_mutex);
>>>       old_map = rcu_dereference_protected(st_link->map, 
>>> lockdep_is_held(&update_mutex));
>>> +    if (!old_map) {
>>> +        err = -EINVAL;
>>> +        goto err_out;
>>> +    }
>>>       if (expected_old_map && old_map != expected_old_map) {
>>>           err = -EPERM;
>>>           goto err_out;
>>> @@ -1139,8 +1142,37 @@ static int 
>>> bpf_struct_ops_map_link_update(struct bpf_link *link, struct bpf_map
>>>       return err;
>>>   }
>>> +static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
>>> +{
>>> +    struct bpf_struct_ops_link *st_link = container_of(link, struct 
>>> bpf_struct_ops_link, link);
>>> +    struct bpf_struct_ops_map *st_map;
>>> +    struct bpf_map *map;
>>> +
>>> +    mutex_lock(&update_mutex);
>>> +
>>> +    map = rcu_dereference_protected(st_link->map, true);
>>
>> nit. s/true/lockdep_is_held(&update_mutex)/
> 
> 
> I thought it is protected by the refcount holding by the caller.
> WDYT?
> 
> 
>>
>>> +    if (!map) {
>>> +        mutex_unlock(&update_mutex);
>>> +        return -EINVAL;
>>> +    }
>>> +    st_map = container_of(map, struct bpf_struct_ops_map, map);
>>> +
>>> +    st_map->st_ops_desc->st_ops->unreg(&st_map->kvalue.data, link);
>>> +
>>> +    rcu_assign_pointer(st_link->map, NULL);
>>> +    /* Pair with bpf_map_get() in bpf_struct_ops_link_create() or
>>> +     * bpf_map_inc() in bpf_struct_ops_map_link_update().
>>> +     */
>>> +    bpf_map_put(&st_map->map);
>>> +
>>> +    mutex_unlock(&update_mutex);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static const struct bpf_link_ops bpf_struct_ops_map_lops = {
>>>       .dealloc = bpf_struct_ops_map_link_dealloc,
>>> +    .detach = bpf_struct_ops_map_link_detach,
>>>       .show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
>>>       .fill_link_info = bpf_struct_ops_map_link_fill_link_info,
>>>       .update_map = bpf_struct_ops_map_link_update,
>>> @@ -1176,13 +1208,19 @@ int bpf_struct_ops_link_create(union bpf_attr 
>>> *attr)
>>>       if (err)
>>>           goto err_out;
>>> +    /* Init link->map before calling reg() in case being detached
>>> +     * immediately.
>>> +     */
>>> +    RCU_INIT_POINTER(link->map, map);
>>> +
>>>       err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, 
>>> &link->link);
>>>       if (err) {
>>> +        rcu_assign_pointer(link->map, NULL);
>>
>> nit. RCU_INIT_POINTER(link->map, NULL) is fine.
> 
> Got it!
> 
>>
>> There is a merge conflict with patch 4 also.
> 
> What do you mean here? Do you mean the patch 4 can not be applied on top
> of the patch 2?

I have seen it after rebasing my local repository.

> 
>>
>> pw-bot: cr
>>
>>>           bpf_link_cleanup(&link_primer);
>>> +        /* The link has been free by bpf_link_cleanup() */
>>>           link = NULL;
>>>           goto err_out;
>>>       }
>>> -    RCU_INIT_POINTER(link->map, map);
>>>       return bpf_link_settle(&link_primer);
>>

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

* [PATCH bpf-next v2 3/6] bpf: support epoll from bpf struct_ops links.
  2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
  2024-05-07  5:55 ` [PATCH bpf-next v2 1/6] bpf: pass bpf_struct_ops_link to callbacks in bpf_struct_ops Kui-Feng Lee
  2024-05-07  5:55 ` [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects Kui-Feng Lee
@ 2024-05-07  5:55 ` Kui-Feng Lee
  2024-05-07  5:55 ` [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll Kui-Feng Lee
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:55 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

Add epoll support to bpf struct_ops links to trigger EPOLLHUP event upon
detachment.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/linux/bpf.h         |  1 +
 kernel/bpf/bpf_struct_ops.c | 17 +++++++++++++++++
 kernel/bpf/syscall.c        | 11 +++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b600767ebe02..5f7496ef8b7c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1612,6 +1612,7 @@ struct bpf_link_ops {
 			      struct bpf_link_info *info);
 	int (*update_map)(struct bpf_link *link, struct bpf_map *new_map,
 			  struct bpf_map *old_map);
+	__poll_t (*poll)(struct file *file, struct poll_table_struct *pts);
 };
 
 struct bpf_tramp_link {
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index bd2602982e4d..f37844b1a94c 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -12,6 +12,7 @@
 #include <linux/mutex.h>
 #include <linux/btf_ids.h>
 #include <linux/rcupdate_wait.h>
+#include <linux/poll.h>
 
 struct bpf_struct_ops_value {
 	struct bpf_struct_ops_common_value common;
@@ -56,6 +57,7 @@ struct bpf_struct_ops_map {
 struct bpf_struct_ops_link {
 	struct bpf_link link;
 	struct bpf_map __rcu *map;
+	wait_queue_head_t wait_hup;
 };
 
 static DEFINE_MUTEX(update_mutex);
@@ -1167,15 +1169,28 @@ static int bpf_struct_ops_map_link_detach(struct bpf_link *link)
 
 	mutex_unlock(&update_mutex);
 
+	wake_up_interruptible_poll(&st_link->wait_hup, EPOLLHUP);
+
 	return 0;
 }
 
+static __poll_t bpf_struct_ops_map_link_poll(struct file *file,
+					     struct poll_table_struct *pts)
+{
+	struct bpf_struct_ops_link *st_link = file->private_data;
+
+	poll_wait(file, &st_link->wait_hup, pts);
+
+	return (st_link->map) ? 0 : EPOLLHUP;
+}
+
 static const struct bpf_link_ops bpf_struct_ops_map_lops = {
 	.dealloc = bpf_struct_ops_map_link_dealloc,
 	.detach = bpf_struct_ops_map_link_detach,
 	.show_fdinfo = bpf_struct_ops_map_link_show_fdinfo,
 	.fill_link_info = bpf_struct_ops_map_link_fill_link_info,
 	.update_map = bpf_struct_ops_map_link_update,
+	.poll = bpf_struct_ops_map_link_poll,
 };
 
 int bpf_struct_ops_link_create(union bpf_attr *attr)
@@ -1213,6 +1228,8 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
 	 */
 	RCU_INIT_POINTER(link->map, map);
 
+	init_waitqueue_head(&link->wait_hup);
+
 	err = st_map->st_ops_desc->st_ops->reg(st_map->kvalue.data, &link->link);
 	if (err) {
 		rcu_assign_pointer(link->map, NULL);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 13ad74ecf2cd..ad4f81ed27f0 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3150,6 +3150,16 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
 }
 #endif
 
+static __poll_t bpf_link_poll(struct file *file, struct poll_table_struct *pts)
+{
+	struct bpf_link *link = file->private_data;
+
+	if (link->ops->poll)
+		return link->ops->poll(file, pts);
+
+	return 0;
+}
+
 static const struct file_operations bpf_link_fops = {
 #ifdef CONFIG_PROC_FS
 	.show_fdinfo	= bpf_link_show_fdinfo,
@@ -3157,6 +3167,7 @@ static const struct file_operations bpf_link_fops = {
 	.release	= bpf_link_release,
 	.read		= bpf_dummy_read,
 	.write		= bpf_dummy_write,
+	.poll		= bpf_link_poll,
 };
 
 static int bpf_link_alloc_id(struct bpf_link *link)
-- 
2.34.1


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

* [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll
  2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
                   ` (2 preceding siblings ...)
  2024-05-07  5:55 ` [PATCH bpf-next v2 3/6] bpf: support epoll from bpf struct_ops links Kui-Feng Lee
@ 2024-05-07  5:55 ` Kui-Feng Lee
  2024-05-08 23:34   ` Martin KaFai Lau
  2024-05-07  5:55 ` [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it Kui-Feng Lee
  2024-05-07  5:56 ` [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well Kui-Feng Lee
  5 siblings, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:55 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

Verify whether a user space program is informed through epoll with EPOLLHUP
when a struct_ops object is detached.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 13 +++++
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  1 +
 .../bpf/prog_tests/test_struct_ops_module.c   | 57 +++++++++++++++++++
 .../selftests/bpf/progs/struct_ops_detach.c   | 31 ++++++++++
 4 files changed, 102 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_detach.c

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index e24a18bfee14..c89a6414c69f 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -10,6 +10,7 @@
 #include <linux/percpu-defs.h>
 #include <linux/sysfs.h>
 #include <linux/tracepoint.h>
+#include <linux/workqueue.h>
 #include "bpf_testmod.h"
 #include "bpf_testmod_kfunc.h"
 
@@ -498,6 +499,9 @@ __bpf_kfunc void bpf_kfunc_call_test_sleepable(void)
 {
 }
 
+static DEFINE_MUTEX(detach_mutex);
+static struct bpf_link *link_to_detach;
+
 BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
@@ -577,11 +581,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
 	if (ops->test_2)
 		ops->test_2(4, ops->data);
 
+	mutex_lock(&detach_mutex);
+	if (!link_to_detach)
+		link_to_detach = link;
+	mutex_unlock(&detach_mutex);
+
 	return 0;
 }
 
 static void bpf_dummy_unreg(void *kdata, struct bpf_link *link)
 {
+	mutex_lock(&detach_mutex);
+	if (link == link_to_detach)
+		link_to_detach = NULL;
+	mutex_unlock(&detach_mutex);
 }
 
 static int bpf_testmod_test_1(void)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
index ce5cd763561c..9f9b60880fd3 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -105,6 +105,7 @@ void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p);
 void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p);
 void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p);
 void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len);
+int bpf_dummy_do_link_detach(void) __ksym;
 
 void bpf_kfunc_common_test(void) __ksym;
 #endif /* _BPF_TESTMOD_KFUNC_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index bd39586abd5a..f39455b81664 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -2,8 +2,12 @@
 /* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
 #include <test_progs.h>
 #include <time.h>
+#include <network_helpers.h>
+
+#include <sys/epoll.h>
 
 #include "struct_ops_module.skel.h"
+#include "struct_ops_detach.skel.h"
 
 static void check_map_info(struct bpf_map_info *info)
 {
@@ -174,6 +178,57 @@ static void test_struct_ops_incompatible(void)
 	struct_ops_module__destroy(skel);
 }
 
+/* Detach a link from a user space program */
+static void test_detach_link(void)
+{
+	struct epoll_event ev, events[2];
+	struct struct_ops_detach *skel;
+	struct bpf_link *link = NULL;
+	int fd, epollfd = -1, nfds;
+	int err;
+
+	skel = struct_ops_detach__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_detach__open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
+	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+		goto cleanup;
+
+	fd = bpf_link__fd(link);
+	if (!ASSERT_GE(fd, 0, "link_fd"))
+		goto cleanup;
+
+	epollfd = epoll_create1(0);
+	if (!ASSERT_GE(epollfd, 0, "epoll_create1"))
+		goto cleanup;
+
+	ev.events = EPOLLHUP;
+	ev.data.fd = fd;
+	err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev);
+	if (!ASSERT_OK(err, "epoll_ctl"))
+		goto cleanup;
+
+	err = bpf_link__detach(link);
+	if (!ASSERT_OK(err, "detach_link"))
+		goto cleanup;
+
+	/* Wait for EPOLLHUP */
+	nfds = epoll_wait(epollfd, events, 2, 500);
+	if (!ASSERT_EQ(nfds, 1, "epoll_wait"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd"))
+		goto cleanup;
+	if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events"))
+		goto cleanup;
+
+cleanup:
+	close(epollfd);
+	bpf_link__destroy(link);
+	struct_ops_detach__destroy(skel);
+}
+
 void serial_test_struct_ops_module(void)
 {
 	if (test__start_subtest("test_struct_ops_load"))
@@ -182,5 +237,7 @@ void serial_test_struct_ops_module(void)
 		test_struct_ops_not_zeroed();
 	if (test__start_subtest("test_struct_ops_incompatible"))
 		test_struct_ops_incompatible();
+	if (test__start_subtest("test_detach_link"))
+		test_detach_link();
 }
 
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
new file mode 100644
index 000000000000..aeb355b3bea3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+int test_1_result = 0;
+int test_2_result = 0;
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1)
+{
+	test_1_result = 0xdeadbeef;
+	return 0;
+}
+
+SEC("struct_ops/test_2")
+void BPF_PROG(test_2, int a, int b)
+{
+	test_2_result = a + b;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_do_detach = {
+	.test_1 = (void *)test_1,
+	.test_2 = (void *)test_2,
+};
-- 
2.34.1


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

* Re: [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll
  2024-05-07  5:55 ` [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll Kui-Feng Lee
@ 2024-05-08 23:34   ` Martin KaFai Lau
  2024-05-09  0:22     ` Kui-Feng Lee
  0 siblings, 1 reply; 18+ messages in thread
From: Martin KaFai Lau @ 2024-05-08 23:34 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: bpf, ast, song, kernel-team, andrii, sinquersw, kuifeng

On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
> Verify whether a user space program is informed through epoll with EPOLLHUP
> when a struct_ops object is detached.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 13 +++++
>   .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  1 +
>   .../bpf/prog_tests/test_struct_ops_module.c   | 57 +++++++++++++++++++
>   .../selftests/bpf/progs/struct_ops_detach.c   | 31 ++++++++++
>   4 files changed, 102 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_detach.c
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index e24a18bfee14..c89a6414c69f 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -10,6 +10,7 @@
>   #include <linux/percpu-defs.h>
>   #include <linux/sysfs.h>
>   #include <linux/tracepoint.h>
> +#include <linux/workqueue.h>
>   #include "bpf_testmod.h"
>   #include "bpf_testmod_kfunc.h"
>   
> @@ -498,6 +499,9 @@ __bpf_kfunc void bpf_kfunc_call_test_sleepable(void)
>   {
>   }
>   
> +static DEFINE_MUTEX(detach_mutex);
> +static struct bpf_link *link_to_detach;
> +
>   BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
>   BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
>   BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
> @@ -577,11 +581,20 @@ static int bpf_dummy_reg(void *kdata, struct bpf_link *link)
>   	if (ops->test_2)
>   		ops->test_2(4, ops->data);
>   
> +	mutex_lock(&detach_mutex);
> +	if (!link_to_detach)
> +		link_to_detach = link;
> +	mutex_unlock(&detach_mutex);
> +
>   	return 0;
>   }
>   
>   static void bpf_dummy_unreg(void *kdata, struct bpf_link *link)
>   {
> +	mutex_lock(&detach_mutex);
> +	if (link == link_to_detach)
> +		link_to_detach = NULL;
> +	mutex_unlock(&detach_mutex);

The reg/unreg changes should belong to the next patch.

>   }
>   
>   static int bpf_testmod_test_1(void)
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> index ce5cd763561c..9f9b60880fd3 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
> @@ -105,6 +105,7 @@ void bpf_kfunc_call_test_fail1(struct prog_test_fail1 *p);
>   void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p);
>   void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p);
>   void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len);
> +int bpf_dummy_do_link_detach(void) __ksym;

The kfunc is not added in this patch either.

>   
>   void bpf_kfunc_common_test(void) __ksym;
>   #endif /* _BPF_TESTMOD_KFUNC_H */
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> index bd39586abd5a..f39455b81664 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> @@ -2,8 +2,12 @@
>   /* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>   #include <test_progs.h>
>   #include <time.h>
> +#include <network_helpers.h>

What is needed from network_herlpers.h?

> +
> +#include <sys/epoll.h>
>   
>   #include "struct_ops_module.skel.h"
> +#include "struct_ops_detach.skel.h"
>   
>   static void check_map_info(struct bpf_map_info *info)
>   {
> @@ -174,6 +178,57 @@ static void test_struct_ops_incompatible(void)
>   	struct_ops_module__destroy(skel);
>   }
>   
> +/* Detach a link from a user space program */
> +static void test_detach_link(void)
> +{
> +	struct epoll_event ev, events[2];
> +	struct struct_ops_detach *skel;
> +	struct bpf_link *link = NULL;
> +	int fd, epollfd = -1, nfds;
> +	int err;
> +
> +	skel = struct_ops_detach__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_detach__open_and_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
> +	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
> +		goto cleanup;
> +
> +	fd = bpf_link__fd(link);
> +	if (!ASSERT_GE(fd, 0, "link_fd"))
> +		goto cleanup;
> +
> +	epollfd = epoll_create1(0);
> +	if (!ASSERT_GE(epollfd, 0, "epoll_create1"))
> +		goto cleanup;
> +
> +	ev.events = EPOLLHUP;
> +	ev.data.fd = fd;
> +	err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev);
> +	if (!ASSERT_OK(err, "epoll_ctl"))
> +		goto cleanup;
> +
> +	err = bpf_link__detach(link);
> +	if (!ASSERT_OK(err, "detach_link"))
> +		goto cleanup;
> +
> +	/* Wait for EPOLLHUP */
> +	nfds = epoll_wait(epollfd, events, 2, 500);
> +	if (!ASSERT_EQ(nfds, 1, "epoll_wait"))
> +		goto cleanup;
> +
> +	if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd"))
> +		goto cleanup;
> +	if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events"))
> +		goto cleanup;
> +
> +cleanup:
> +	close(epollfd);

Better check epollfd since it is init to -1. There are cases that epollfd is -1 
here.

> +	bpf_link__destroy(link);
> +	struct_ops_detach__destroy(skel);
> +}
> +
>   void serial_test_struct_ops_module(void)
>   {
>   	if (test__start_subtest("test_struct_ops_load"))
> @@ -182,5 +237,7 @@ void serial_test_struct_ops_module(void)
>   		test_struct_ops_not_zeroed();
>   	if (test__start_subtest("test_struct_ops_incompatible"))
>   		test_struct_ops_incompatible();
> +	if (test__start_subtest("test_detach_link"))
> +		test_detach_link();
>   }
>   
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
> new file mode 100644
> index 000000000000..aeb355b3bea3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +#include <vmlinux.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include "../bpf_testmod/bpf_testmod.h"
> +#include "../bpf_testmod/bpf_testmod_kfunc.h"

The _kfunc.h should not be needed in this patch either.

> +
> +char _license[] SEC("license") = "GPL";
> +
> +int test_1_result = 0;
> +int test_2_result = 0;

Are these global vars tested? If not, can the test_1 and test_2 programs be 
removed? or some of them is not optional?

> +
> +SEC("struct_ops/test_1")
> +int BPF_PROG(test_1)
> +{
> +	test_1_result = 0xdeadbeef;
> +	return 0;
> +}
> +
> +SEC("struct_ops/test_2")
> +void BPF_PROG(test_2, int a, int b)
> +{
> +	test_2_result = a + b;
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_ops testmod_do_detach = {
> +	.test_1 = (void *)test_1,
> +	.test_2 = (void *)test_2,
> +};


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

* Re: [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll
  2024-05-08 23:34   ` Martin KaFai Lau
@ 2024-05-09  0:22     ` Kui-Feng Lee
  0 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-09  0:22 UTC (permalink / raw)
  To: Martin KaFai Lau, Kui-Feng Lee
  Cc: bpf, ast, song, kernel-team, andrii, kuifeng



On 5/8/24 16:34, Martin KaFai Lau wrote:
> On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
>> Verify whether a user space program is informed through epoll with 
>> EPOLLHUP
>> when a struct_ops object is detached.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 13 +++++
>>   .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  1 +
>>   .../bpf/prog_tests/test_struct_ops_module.c   | 57 +++++++++++++++++++
>>   .../selftests/bpf/progs/struct_ops_detach.c   | 31 ++++++++++
>>   4 files changed, 102 insertions(+)
>>   create mode 100644 
>> tools/testing/selftests/bpf/progs/struct_ops_detach.c
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c 
>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index e24a18bfee14..c89a6414c69f 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/percpu-defs.h>
>>   #include <linux/sysfs.h>
>>   #include <linux/tracepoint.h>
>> +#include <linux/workqueue.h>
>>   #include "bpf_testmod.h"
>>   #include "bpf_testmod_kfunc.h"
>> @@ -498,6 +499,9 @@ __bpf_kfunc void bpf_kfunc_call_test_sleepable(void)
>>   {
>>   }
>> +static DEFINE_MUTEX(detach_mutex);
>> +static struct bpf_link *link_to_detach;
>> +
>>   BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
>>   BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
>> @@ -577,11 +581,20 @@ static int bpf_dummy_reg(void *kdata, struct 
>> bpf_link *link)
>>       if (ops->test_2)
>>           ops->test_2(4, ops->data);
>> +    mutex_lock(&detach_mutex);
>> +    if (!link_to_detach)
>> +        link_to_detach = link;
>> +    mutex_unlock(&detach_mutex);
>> +
>>       return 0;
>>   }
>>   static void bpf_dummy_unreg(void *kdata, struct bpf_link *link)
>>   {
>> +    mutex_lock(&detach_mutex);
>> +    if (link == link_to_detach)
>> +        link_to_detach = NULL;
>> +    mutex_unlock(&detach_mutex);
> 
> The reg/unreg changes should belong to the next patch.
Sure!

> 
>>   }
>>   static int bpf_testmod_test_1(void)
>> diff --git 
>> a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h 
>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
>> index ce5cd763561c..9f9b60880fd3 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
>> @@ -105,6 +105,7 @@ void bpf_kfunc_call_test_fail1(struct 
>> prog_test_fail1 *p);
>>   void bpf_kfunc_call_test_fail2(struct prog_test_fail2 *p);
>>   void bpf_kfunc_call_test_fail3(struct prog_test_fail3 *p);
>>   void bpf_kfunc_call_test_mem_len_fail1(void *mem, int len);
>> +int bpf_dummy_do_link_detach(void) __ksym;
> 
> The kfunc is not added in this patch either.

Sure!

> 
>>   void bpf_kfunc_common_test(void) __ksym;
>>   #endif /* _BPF_TESTMOD_KFUNC_H */
>> diff --git 
>> a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c 
>> b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> index bd39586abd5a..f39455b81664 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> @@ -2,8 +2,12 @@
>>   /* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>>   #include <test_progs.h>
>>   #include <time.h>
>> +#include <network_helpers.h>
> 
> What is needed from network_herlpers.h?
> 
>> +
>> +#include <sys/epoll.h>
>>   #include "struct_ops_module.skel.h"
>> +#include "struct_ops_detach.skel.h"
>>   static void check_map_info(struct bpf_map_info *info)
>>   {
>> @@ -174,6 +178,57 @@ static void test_struct_ops_incompatible(void)
>>       struct_ops_module__destroy(skel);
>>   }
>> +/* Detach a link from a user space program */
>> +static void test_detach_link(void)
>> +{
>> +    struct epoll_event ev, events[2];
>> +    struct struct_ops_detach *skel;
>> +    struct bpf_link *link = NULL;
>> +    int fd, epollfd = -1, nfds;
>> +    int err;
>> +
>> +    skel = struct_ops_detach__open_and_load();
>> +    if (!ASSERT_OK_PTR(skel, "struct_ops_detach__open_and_load"))
>> +        return;
>> +
>> +    link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
>> +    if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
>> +        goto cleanup;
>> +
>> +    fd = bpf_link__fd(link);
>> +    if (!ASSERT_GE(fd, 0, "link_fd"))
>> +        goto cleanup;
>> +
>> +    epollfd = epoll_create1(0);
>> +    if (!ASSERT_GE(epollfd, 0, "epoll_create1"))
>> +        goto cleanup;
>> +
>> +    ev.events = EPOLLHUP;
>> +    ev.data.fd = fd;
>> +    err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev);
>> +    if (!ASSERT_OK(err, "epoll_ctl"))
>> +        goto cleanup;
>> +
>> +    err = bpf_link__detach(link);
>> +    if (!ASSERT_OK(err, "detach_link"))
>> +        goto cleanup;
>> +
>> +    /* Wait for EPOLLHUP */
>> +    nfds = epoll_wait(epollfd, events, 2, 500);
>> +    if (!ASSERT_EQ(nfds, 1, "epoll_wait"))
>> +        goto cleanup;
>> +
>> +    if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd"))
>> +        goto cleanup;
>> +    if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events"))
>> +        goto cleanup;
>> +
>> +cleanup:
>> +    close(epollfd);
> 
> Better check epollfd since it is init to -1. There are cases that 
> epollfd is -1 here.

Ok! Although close(-1) doesn't cause any issue, it makes sense doing
check before calling it.

> 
>> +    bpf_link__destroy(link);
>> +    struct_ops_detach__destroy(skel);
>> +}
>> +
>>   void serial_test_struct_ops_module(void)
>>   {
>>       if (test__start_subtest("test_struct_ops_load"))
>> @@ -182,5 +237,7 @@ void serial_test_struct_ops_module(void)
>>           test_struct_ops_not_zeroed();
>>       if (test__start_subtest("test_struct_ops_incompatible"))
>>           test_struct_ops_incompatible();
>> +    if (test__start_subtest("test_detach_link"))
>> +        test_detach_link();
>>   }
>> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c 
>> b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
>> new file mode 100644
>> index 000000000000..aeb355b3bea3
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
>> @@ -0,0 +1,31 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +#include "../bpf_testmod/bpf_testmod.h"
>> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
> 
> The _kfunc.h should not be needed in this patch either.

Sure!

> 
>> +
>> +char _license[] SEC("license") = "GPL";
>> +
>> +int test_1_result = 0;
>> +int test_2_result = 0;
> 
> Are these global vars tested? If not, can the test_1 and test_2 programs 
> be removed? or some of them is not optional?

Yes, they are optional. I will remove these functions.

> 
>> +
>> +SEC("struct_ops/test_1")
>> +int BPF_PROG(test_1)
>> +{
>> +    test_1_result = 0xdeadbeef;
>> +    return 0;
>> +}
>> +
>> +SEC("struct_ops/test_2")
>> +void BPF_PROG(test_2, int a, int b)
>> +{
>> +    test_2_result = a + b;
>> +}
>> +
>> +SEC(".struct_ops.link")
>> +struct bpf_testmod_ops testmod_do_detach = {
>> +    .test_1 = (void *)test_1,
>> +    .test_2 = (void *)test_2,
>> +};
> 

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

* [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it.
  2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
                   ` (3 preceding siblings ...)
  2024-05-07  5:55 ` [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll Kui-Feng Lee
@ 2024-05-07  5:55 ` Kui-Feng Lee
  2024-05-08 23:50   ` Martin KaFai Lau
  2024-05-07  5:56 ` [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well Kui-Feng Lee
  5 siblings, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:55 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

Not only a user space program can detach a struct_ops link, the subsystem
managing a link can also detach the link. This patch add a kfunc to
simulate detaching a link by the subsystem managing it.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 21 ++++++
 .../bpf/prog_tests/test_struct_ops_module.c   | 65 +++++++++++++++++++
 .../selftests/bpf/progs/struct_ops_detach.c   |  6 ++
 3 files changed, 92 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index c89a6414c69f..0bf1acc1767a 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -502,6 +502,26 @@ __bpf_kfunc void bpf_kfunc_call_test_sleepable(void)
 static DEFINE_MUTEX(detach_mutex);
 static struct bpf_link *link_to_detach;
 
+__bpf_kfunc int bpf_dummy_do_link_detach(void)
+{
+	struct bpf_link *link;
+	int ret = -ENOENT;
+
+	mutex_lock(&detach_mutex);
+	link = link_to_detach;
+	/* Make sure the link is still valid by increasing its refcnt */
+	if (link && !atomic64_inc_not_zero(&link->refcnt))
+		link = NULL;
+	mutex_unlock(&detach_mutex);
+
+	if (link) {
+		ret = link->ops->detach(link);
+		bpf_link_put(link);
+	}
+
+	return ret;
+}
+
 BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
 BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
@@ -529,6 +549,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_offset)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_sleepable, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, bpf_dummy_do_link_detach)
 BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
 
 static int bpf_testmod_ops_init(struct btf *btf)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index f39455b81664..9f6657b53a93 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -229,6 +229,69 @@ static void test_detach_link(void)
 	struct_ops_detach__destroy(skel);
 }
 
+/* Detach a link from the subsystem that the link was registered to */
+static void test_subsystem_detach(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		    .data_in = &pkt_v4,
+		    .data_size_in = sizeof(pkt_v4));
+	struct epoll_event ev, events[2];
+	struct struct_ops_detach *skel;
+	struct bpf_link *link = NULL;
+	int fd, epollfd = -1, nfds;
+	int prog_fd;
+	int err;
+
+	skel = struct_ops_detach__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
+	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+		goto cleanup;
+
+	fd = bpf_link__fd(link);
+	if (!ASSERT_GE(fd, 0, "link_fd"))
+		goto cleanup;
+
+	prog_fd = bpf_program__fd(skel->progs.start_detach);
+	if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
+		goto cleanup;
+
+	/* Do detachment from the registered subsystem */
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "start_detach_run"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(topts.retval, 0, "start_detach_run retval"))
+		goto cleanup;
+
+	epollfd = epoll_create1(0);
+	if (!ASSERT_GE(epollfd, 0, "epoll_create1"))
+		goto cleanup;
+
+	ev.events = EPOLLHUP;
+	ev.data.fd = fd;
+	err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev);
+	if (!ASSERT_OK(err, "epoll_ctl"))
+		goto cleanup;
+
+	/* Wait for EPOLLHUP */
+	nfds = epoll_wait(epollfd, events, 2, 5000);
+	if (!ASSERT_EQ(nfds, 1, "epoll_wait"))
+		goto cleanup;
+
+	if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd"))
+		goto cleanup;
+	if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events"))
+		goto cleanup;
+
+cleanup:
+	close(epollfd);
+	bpf_link__destroy(link);
+	struct_ops_detach__destroy(skel);
+}
+
 void serial_test_struct_ops_module(void)
 {
 	if (test__start_subtest("test_struct_ops_load"))
@@ -239,5 +302,7 @@ void serial_test_struct_ops_module(void)
 		test_struct_ops_incompatible();
 	if (test__start_subtest("test_detach_link"))
 		test_detach_link();
+	if (test__start_subtest("test_subsystem_detach"))
+		test_subsystem_detach();
 }
 
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
index aeb355b3bea3..139f9a5c5601 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_detach.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
@@ -29,3 +29,9 @@ struct bpf_testmod_ops testmod_do_detach = {
 	.test_1 = (void *)test_1,
 	.test_2 = (void *)test_2,
 };
+
+SEC("tc")
+int start_detach(void *skb)
+{
+	return bpf_dummy_do_link_detach();
+}
-- 
2.34.1


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

* Re: [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it.
  2024-05-07  5:55 ` [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it Kui-Feng Lee
@ 2024-05-08 23:50   ` Martin KaFai Lau
  2024-05-09  5:50     ` Kui-Feng Lee
  0 siblings, 1 reply; 18+ messages in thread
From: Martin KaFai Lau @ 2024-05-08 23:50 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii

On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
> Not only a user space program can detach a struct_ops link, the subsystem
> managing a link can also detach the link. This patch add a kfunc to
> simulate detaching a link by the subsystem managing it.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 21 ++++++
>   .../bpf/prog_tests/test_struct_ops_module.c   | 65 +++++++++++++++++++
>   .../selftests/bpf/progs/struct_ops_detach.c   |  6 ++
>   3 files changed, 92 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> index c89a6414c69f..0bf1acc1767a 100644
> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
> @@ -502,6 +502,26 @@ __bpf_kfunc void bpf_kfunc_call_test_sleepable(void)
>   static DEFINE_MUTEX(detach_mutex);
>   static struct bpf_link *link_to_detach;
>   
> +__bpf_kfunc int bpf_dummy_do_link_detach(void)
> +{
> +	struct bpf_link *link;
> +	int ret = -ENOENT;
> +
> +	mutex_lock(&detach_mutex);
> +	link = link_to_detach;
> +	/* Make sure the link is still valid by increasing its refcnt */
> +	if (link && !atomic64_inc_not_zero(&link->refcnt))

It is better to reuse the bpf_link_inc_not_zero().

> +		link = NULL;
> +	mutex_unlock(&detach_mutex);
> +
> +	if (link) {
> +		ret = link->ops->detach(link);
> +		bpf_link_put(link);
> +	}
> +
> +	return ret;
> +}
> +
>   BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
>   BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
>   BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
> @@ -529,6 +549,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
>   BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
>   BTF_ID_FLAGS(func, bpf_kfunc_call_test_offset)
>   BTF_ID_FLAGS(func, bpf_kfunc_call_test_sleepable, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_dummy_do_link_detach)

It should need KF_SLEEPABLE. mutex is used.

>   BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
>   
>   static int bpf_testmod_ops_init(struct btf *btf)
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> index f39455b81664..9f6657b53a93 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> @@ -229,6 +229,69 @@ static void test_detach_link(void)
>   	struct_ops_detach__destroy(skel);
>   }
>   
> +/* Detach a link from the subsystem that the link was registered to */
> +static void test_subsystem_detach(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
> +		    .data_in = &pkt_v4,
> +		    .data_size_in = sizeof(pkt_v4));
> +	struct epoll_event ev, events[2];
> +	struct struct_ops_detach *skel;
> +	struct bpf_link *link = NULL;
> +	int fd, epollfd = -1, nfds;
> +	int prog_fd;
> +	int err;
> +
> +	skel = struct_ops_detach__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
> +	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
> +		goto cleanup;
> +
> +	fd = bpf_link__fd(link);
> +	if (!ASSERT_GE(fd, 0, "link_fd"))
> +		goto cleanup;
> +
> +	prog_fd = bpf_program__fd(skel->progs.start_detach);
> +	if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
> +		goto cleanup;
> +
> +	/* Do detachment from the registered subsystem */
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	if (!ASSERT_OK(err, "start_detach_run"))
> +		goto cleanup;
> +
> +	if (!ASSERT_EQ(topts.retval, 0, "start_detach_run retval"))
> +		goto cleanup;
> +
> +	epollfd = epoll_create1(0);
> +	if (!ASSERT_GE(epollfd, 0, "epoll_create1"))
> +		goto cleanup;
> +
> +	ev.events = EPOLLHUP;
> +	ev.data.fd = fd;
> +	err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev);
> +	if (!ASSERT_OK(err, "epoll_ctl"))
> +		goto cleanup;
> +
> +	/* Wait for EPOLLHUP */
> +	nfds = epoll_wait(epollfd, events, 2, 5000);
> +	if (!ASSERT_EQ(nfds, 1, "epoll_wait"))
> +		goto cleanup;
> +
> +	if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd"))
> +		goto cleanup;
> +	if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events"))
> +		goto cleanup;
> +
> +cleanup:
> +	close(epollfd);
> +	bpf_link__destroy(link);
> +	struct_ops_detach__destroy(skel);
> +}
> +
>   void serial_test_struct_ops_module(void)
>   {
>   	if (test__start_subtest("test_struct_ops_load"))
> @@ -239,5 +302,7 @@ void serial_test_struct_ops_module(void)
>   		test_struct_ops_incompatible();
>   	if (test__start_subtest("test_detach_link"))
>   		test_detach_link();
> +	if (test__start_subtest("test_subsystem_detach"))
> +		test_subsystem_detach();
>   }
>   
> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
> index aeb355b3bea3..139f9a5c5601 100644
> --- a/tools/testing/selftests/bpf/progs/struct_ops_detach.c
> +++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
> @@ -29,3 +29,9 @@ struct bpf_testmod_ops testmod_do_detach = {
>   	.test_1 = (void *)test_1,
>   	.test_2 = (void *)test_2,
>   };
> +
> +SEC("tc")

The bpf_dummy_do_link_detach() uses a mutex. There is no lockdep splat in the test?

> +int start_detach(void *skb)
> +{
> +	return bpf_dummy_do_link_detach();
> +}


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

* Re: [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it.
  2024-05-08 23:50   ` Martin KaFai Lau
@ 2024-05-09  5:50     ` Kui-Feng Lee
  0 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-09  5:50 UTC (permalink / raw)
  To: Martin KaFai Lau, Kui-Feng Lee
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii



On 5/8/24 16:50, Martin KaFai Lau wrote:
> On 5/6/24 10:55 PM, Kui-Feng Lee wrote:
>> Not only a user space program can detach a struct_ops link, the subsystem
>> managing a link can also detach the link. This patch add a kfunc to
>> simulate detaching a link by the subsystem managing it.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 21 ++++++
>>   .../bpf/prog_tests/test_struct_ops_module.c   | 65 +++++++++++++++++++
>>   .../selftests/bpf/progs/struct_ops_detach.c   |  6 ++
>>   3 files changed, 92 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c 
>> b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index c89a6414c69f..0bf1acc1767a 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -502,6 +502,26 @@ __bpf_kfunc void bpf_kfunc_call_test_sleepable(void)
>>   static DEFINE_MUTEX(detach_mutex);
>>   static struct bpf_link *link_to_detach;
>> +__bpf_kfunc int bpf_dummy_do_link_detach(void)
>> +{
>> +    struct bpf_link *link;
>> +    int ret = -ENOENT;
>> +
>> +    mutex_lock(&detach_mutex);
>> +    link = link_to_detach;
>> +    /* Make sure the link is still valid by increasing its refcnt */
>> +    if (link && !atomic64_inc_not_zero(&link->refcnt))
> 
> It is better to reuse the bpf_link_inc_not_zero().

I will export this function to be used by modules.

> 
>> +        link = NULL;
>> +    mutex_unlock(&detach_mutex);
>> +
>> +    if (link) {
>> +        ret = link->ops->detach(link);
>> +        bpf_link_put(link);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
>>   BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
>> @@ -529,6 +549,7 @@ BTF_ID_FLAGS(func, 
>> bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test_offset)
>>   BTF_ID_FLAGS(func, bpf_kfunc_call_test_sleepable, KF_SLEEPABLE)
>> +BTF_ID_FLAGS(func, bpf_dummy_do_link_detach)
> 
> It should need KF_SLEEPABLE. mutex is used.

To simplify it, spinlock will be used instead.

> 
>>   BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
>>   static int bpf_testmod_ops_init(struct btf *btf)
>> diff --git 
>> a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c 
>> b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> index f39455b81664..9f6657b53a93 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> @@ -229,6 +229,69 @@ static void test_detach_link(void)
>>       struct_ops_detach__destroy(skel);
>>   }
>> +/* Detach a link from the subsystem that the link was registered to */
>> +static void test_subsystem_detach(void)
>> +{
>> +    LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +            .data_in = &pkt_v4,
>> +            .data_size_in = sizeof(pkt_v4));
>> +    struct epoll_event ev, events[2];
>> +    struct struct_ops_detach *skel;
>> +    struct bpf_link *link = NULL;
>> +    int fd, epollfd = -1, nfds;
>> +    int prog_fd;
>> +    int err;
>> +
>> +    skel = struct_ops_detach__open_and_load();
>> +    if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
>> +        return;
>> +
>> +    link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
>> +    if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
>> +        goto cleanup;
>> +
>> +    fd = bpf_link__fd(link);
>> +    if (!ASSERT_GE(fd, 0, "link_fd"))
>> +        goto cleanup;
>> +
>> +    prog_fd = bpf_program__fd(skel->progs.start_detach);
>> +    if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
>> +        goto cleanup;
>> +
>> +    /* Do detachment from the registered subsystem */
>> +    err = bpf_prog_test_run_opts(prog_fd, &topts);
>> +    if (!ASSERT_OK(err, "start_detach_run"))
>> +        goto cleanup;
>> +
>> +    if (!ASSERT_EQ(topts.retval, 0, "start_detach_run retval"))
>> +        goto cleanup;
>> +
>> +    epollfd = epoll_create1(0);
>> +    if (!ASSERT_GE(epollfd, 0, "epoll_create1"))
>> +        goto cleanup;
>> +
>> +    ev.events = EPOLLHUP;
>> +    ev.data.fd = fd;
>> +    err = epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev);
>> +    if (!ASSERT_OK(err, "epoll_ctl"))
>> +        goto cleanup;
>> +
>> +    /* Wait for EPOLLHUP */
>> +    nfds = epoll_wait(epollfd, events, 2, 5000);
>> +    if (!ASSERT_EQ(nfds, 1, "epoll_wait"))
>> +        goto cleanup;
>> +
>> +    if (!ASSERT_EQ(events[0].data.fd, fd, "epoll_wait_fd"))
>> +        goto cleanup;
>> +    if (!ASSERT_TRUE(events[0].events & EPOLLHUP, "events[0].events"))
>> +        goto cleanup;
>> +
>> +cleanup:
>> +    close(epollfd);
>> +    bpf_link__destroy(link);
>> +    struct_ops_detach__destroy(skel);
>> +}
>> +
>>   void serial_test_struct_ops_module(void)
>>   {
>>       if (test__start_subtest("test_struct_ops_load"))
>> @@ -239,5 +302,7 @@ void serial_test_struct_ops_module(void)
>>           test_struct_ops_incompatible();
>>       if (test__start_subtest("test_detach_link"))
>>           test_detach_link();
>> +    if (test__start_subtest("test_subsystem_detach"))
>> +        test_subsystem_detach();
>>   }
>> diff --git a/tools/testing/selftests/bpf/progs/struct_ops_detach.c 
>> b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
>> index aeb355b3bea3..139f9a5c5601 100644
>> --- a/tools/testing/selftests/bpf/progs/struct_ops_detach.c
>> +++ b/tools/testing/selftests/bpf/progs/struct_ops_detach.c
>> @@ -29,3 +29,9 @@ struct bpf_testmod_ops testmod_do_detach = {
>>       .test_1 = (void *)test_1,
>>       .test_2 = (void *)test_2,
>>   };
>> +
>> +SEC("tc")
> 
> The bpf_dummy_do_link_detach() uses a mutex. There is no lockdep splat 
> in the test?
> 
>> +int start_detach(void *skb)
>> +{
>> +    return bpf_dummy_do_link_detach();
>> +}
> 

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

* [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well.
  2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
                   ` (4 preceding siblings ...)
  2024-05-07  5:55 ` [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it Kui-Feng Lee
@ 2024-05-07  5:56 ` Kui-Feng Lee
  2024-05-09  0:04   ` Martin KaFai Lau
  5 siblings, 1 reply; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-07  5:56 UTC (permalink / raw)
  To: bpf, ast, martin.lau, song, kernel-team, andrii
  Cc: sinquersw, kuifeng, Kui-Feng Lee

Subsystems that manage struct_ops objects may attempt to detach a link when
the link has been released or is about to be released. The test in
this patch demonstrate to developers the correct way to handle this
situation using a locking mechanism and atomic64_inc_not_zero().

A subsystem must ensure that a link is valid when detaching the link. In
order to achieve that, the subsystem may need to obtain a lock to safeguard
a table that holds the pointer to the link being detached. However, the
subsystem cannot invoke link->ops->detach() while holding the lock because
other tasks may be in the process of unregistering, which could lead to a
deadlock. This is why atomic64_inc_not_zero() is used to maintain the
link's validity. (Refer to bpf_dummy_do_link_detach() in the previous patch
for more details.)

This test make sure the pattern mentioned above work correctly.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../bpf/prog_tests/test_struct_ops_module.c   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index 9f6657b53a93..1e37037cfd8a 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -292,6 +292,48 @@ static void test_subsystem_detach(void)
 	struct_ops_detach__destroy(skel);
 }
 
+/* A subsystem detachs a link while the link is going to be free. */
+static void test_subsystem_detach_free(void)
+{
+	LIBBPF_OPTS(bpf_test_run_opts, topts,
+		    .data_in = &pkt_v4,
+		    .data_size_in = sizeof(pkt_v4));
+	struct struct_ops_detach *skel;
+	struct bpf_link *link = NULL;
+	int prog_fd;
+	int err;
+
+	skel = struct_ops_detach__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
+	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+		goto cleanup;
+
+	bpf_link__destroy(link);
+
+	prog_fd = bpf_program__fd(skel->progs.start_detach);
+	if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
+		goto cleanup;
+
+	/* Do detachment from the registered subsystem */
+	err = bpf_prog_test_run_opts(prog_fd, &topts);
+	if (!ASSERT_OK(err, "start_detach_run"))
+		goto cleanup;
+
+	/* The link may have zero refcount value and may have been
+	 * unregistered, so the detachment from the subsystem should fail.
+	 */
+	ASSERT_EQ(topts.retval, (u32)-ENOENT, "start_detach_run retval");
+
+	/* Sync RCU to make sure the link is freed without any crash */
+	ASSERT_OK(kern_sync_rcu(), "sync rcu");
+
+cleanup:
+	struct_ops_detach__destroy(skel);
+}
+
 void serial_test_struct_ops_module(void)
 {
 	if (test__start_subtest("test_struct_ops_load"))
@@ -304,5 +346,7 @@ void serial_test_struct_ops_module(void)
 		test_detach_link();
 	if (test__start_subtest("test_subsystem_detach"))
 		test_subsystem_detach();
+	if (test__start_subtest("test_subsystem_detach_free"))
+		test_subsystem_detach_free();
 }
 
-- 
2.34.1


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

* Re: [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well.
  2024-05-07  5:56 ` [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well Kui-Feng Lee
@ 2024-05-09  0:04   ` Martin KaFai Lau
  2024-05-09 17:02     ` Kui-Feng Lee
  0 siblings, 1 reply; 18+ messages in thread
From: Martin KaFai Lau @ 2024-05-09  0:04 UTC (permalink / raw)
  To: Kui-Feng Lee; +Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team, andrii

On 5/6/24 10:56 PM, Kui-Feng Lee wrote:
> Subsystems that manage struct_ops objects may attempt to detach a link when
> the link has been released or is about to be released. The test in
> this patch demonstrate to developers the correct way to handle this
> situation using a locking mechanism and atomic64_inc_not_zero().
> 
> A subsystem must ensure that a link is valid when detaching the link. In
> order to achieve that, the subsystem may need to obtain a lock to safeguard
> a table that holds the pointer to the link being detached. However, the
> subsystem cannot invoke link->ops->detach() while holding the lock because
> other tasks may be in the process of unregistering, which could lead to a
> deadlock. This is why atomic64_inc_not_zero() is used to maintain the

Other tasks un-registering in parallel is not the reason for deadlock. The 
deadlock is because the link detach will call unreg() which usually will acquire 
the same lock (the detach_mutex here) and there is lock ordering with the 
update_mutex also. Hence, the link detach must be done after releasing the 
detach_mutex. After releasing the detach_mutex, the link is protected by its refcnt.

I think the above should be put as comments in bpf_dummy_do_link_detach for the 
subsystem to reference later.

> link's validity. (Refer to bpf_dummy_do_link_detach() in the previous patch
> for more details.)
> 
> This test make sure the pattern mentioned above work correctly.
> 
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>   .../bpf/prog_tests/test_struct_ops_module.c   | 44 +++++++++++++++++++
>   1 file changed, 44 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> index 9f6657b53a93..1e37037cfd8a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
> @@ -292,6 +292,48 @@ static void test_subsystem_detach(void)
>   	struct_ops_detach__destroy(skel);
>   }
>   
> +/* A subsystem detachs a link while the link is going to be free. */
> +static void test_subsystem_detach_free(void)
> +{
> +	LIBBPF_OPTS(bpf_test_run_opts, topts,
> +		    .data_in = &pkt_v4,
> +		    .data_size_in = sizeof(pkt_v4));
> +	struct struct_ops_detach *skel;
> +	struct bpf_link *link = NULL;
> +	int prog_fd;
> +	int err;
> +
> +	skel = struct_ops_detach__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
> +	if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
> +		goto cleanup;
> +
> +	bpf_link__destroy(link);
> +
> +	prog_fd = bpf_program__fd(skel->progs.start_detach);
> +	if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
> +		goto cleanup;
> +
> +	/* Do detachment from the registered subsystem */
> +	err = bpf_prog_test_run_opts(prog_fd, &topts);
> +	if (!ASSERT_OK(err, "start_detach_run"))
> +		goto cleanup;
> +
> +	/* The link may have zero refcount value and may have been
> +	 * unregistered, so the detachment from the subsystem should fail.
> +	 */
> +	ASSERT_EQ(topts.retval, (u32)-ENOENT, "start_detach_run retval");
> +
> +	/* Sync RCU to make sure the link is freed without any crash */
> +	ASSERT_OK(kern_sync_rcu(), "sync rcu");
> +
> +cleanup:
> +	struct_ops_detach__destroy(skel);
> +}
> +
>   void serial_test_struct_ops_module(void)
>   {
>   	if (test__start_subtest("test_struct_ops_load"))
> @@ -304,5 +346,7 @@ void serial_test_struct_ops_module(void)
>   		test_detach_link();
>   	if (test__start_subtest("test_subsystem_detach"))
>   		test_subsystem_detach();
> +	if (test__start_subtest("test_subsystem_detach_free"))
> +		test_subsystem_detach_free();
>   }
>   


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

* Re: [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well.
  2024-05-09  0:04   ` Martin KaFai Lau
@ 2024-05-09 17:02     ` Kui-Feng Lee
  0 siblings, 0 replies; 18+ messages in thread
From: Kui-Feng Lee @ 2024-05-09 17:02 UTC (permalink / raw)
  To: Martin KaFai Lau, Kui-Feng Lee
  Cc: kuifeng, bpf, ast, song, kernel-team, andrii



On 5/8/24 17:04, Martin KaFai Lau wrote:
> On 5/6/24 10:56 PM, Kui-Feng Lee wrote:
>> Subsystems that manage struct_ops objects may attempt to detach a link 
>> when
>> the link has been released or is about to be released. The test in
>> this patch demonstrate to developers the correct way to handle this
>> situation using a locking mechanism and atomic64_inc_not_zero().
>>
>> A subsystem must ensure that a link is valid when detaching the link. In
>> order to achieve that, the subsystem may need to obtain a lock to 
>> safeguard
>> a table that holds the pointer to the link being detached. However, the
>> subsystem cannot invoke link->ops->detach() while holding the lock 
>> because
>> other tasks may be in the process of unregistering, which could lead to a
>> deadlock. This is why atomic64_inc_not_zero() is used to maintain the
> 
> Other tasks un-registering in parallel is not the reason for deadlock. 
> The deadlock is because the link detach will call unreg() which usually 
> will acquire the same lock (the detach_mutex here) and there is lock 
> ordering with the update_mutex also. Hence, the link detach must be done 
> after releasing the detach_mutex. After releasing the detach_mutex, the 
> link is protected by its refcnt.

It is what I mean in the commit log. I will rephrase it to emphasize
holding the same lock.

> 
> I think the above should be put as comments in bpf_dummy_do_link_detach 
> for the subsystem to reference later.

ok!

> 
>> link's validity. (Refer to bpf_dummy_do_link_detach() in the previous 
>> patch
>> for more details.)
>>
>> This test make sure the pattern mentioned above work correctly.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   .../bpf/prog_tests/test_struct_ops_module.c   | 44 +++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git 
>> a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c 
>> b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> index 9f6657b53a93..1e37037cfd8a 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
>> @@ -292,6 +292,48 @@ static void test_subsystem_detach(void)
>>       struct_ops_detach__destroy(skel);
>>   }
>> +/* A subsystem detachs a link while the link is going to be free. */
>> +static void test_subsystem_detach_free(void)
>> +{
>> +    LIBBPF_OPTS(bpf_test_run_opts, topts,
>> +            .data_in = &pkt_v4,
>> +            .data_size_in = sizeof(pkt_v4));
>> +    struct struct_ops_detach *skel;
>> +    struct bpf_link *link = NULL;
>> +    int prog_fd;
>> +    int err;
>> +
>> +    skel = struct_ops_detach__open_and_load();
>> +    if (!ASSERT_OK_PTR(skel, "struct_ops_detach_open_and_load"))
>> +        return;
>> +
>> +    link = bpf_map__attach_struct_ops(skel->maps.testmod_do_detach);
>> +    if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
>> +        goto cleanup;
>> +
>> +    bpf_link__destroy(link);
>> +
>> +    prog_fd = bpf_program__fd(skel->progs.start_detach);
>> +    if (!ASSERT_GE(prog_fd, 0, "start_detach_fd"))
>> +        goto cleanup;
>> +
>> +    /* Do detachment from the registered subsystem */
>> +    err = bpf_prog_test_run_opts(prog_fd, &topts);
>> +    if (!ASSERT_OK(err, "start_detach_run"))
>> +        goto cleanup;
>> +
>> +    /* The link may have zero refcount value and may have been
>> +     * unregistered, so the detachment from the subsystem should fail.
>> +     */
>> +    ASSERT_EQ(topts.retval, (u32)-ENOENT, "start_detach_run retval");
>> +
>> +    /* Sync RCU to make sure the link is freed without any crash */
>> +    ASSERT_OK(kern_sync_rcu(), "sync rcu");
>> +
>> +cleanup:
>> +    struct_ops_detach__destroy(skel);
>> +}
>> +
>>   void serial_test_struct_ops_module(void)
>>   {
>>       if (test__start_subtest("test_struct_ops_load"))
>> @@ -304,5 +346,7 @@ void serial_test_struct_ops_module(void)
>>           test_detach_link();
>>       if (test__start_subtest("test_subsystem_detach"))
>>           test_subsystem_detach();
>> +    if (test__start_subtest("test_subsystem_detach_free"))
>> +        test_subsystem_detach_free();
>>   }
> 

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

end of thread, other threads:[~2024-05-09 17:02 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-07  5:55 [PATCH bpf-next v2 0/6] Notify user space when a struct_ops object is detached/unregistered Kui-Feng Lee
2024-05-07  5:55 ` [PATCH bpf-next v2 1/6] bpf: pass bpf_struct_ops_link to callbacks in bpf_struct_ops Kui-Feng Lee
2024-05-07  5:55 ` [PATCH bpf-next v2 2/6] bpf: enable detaching links of struct_ops objects Kui-Feng Lee
2024-05-08 23:22   ` Martin KaFai Lau
2024-05-09  0:14     ` Kui-Feng Lee
2024-05-09  0:36       ` Martin KaFai Lau
2024-05-09 16:59         ` Kui-Feng Lee
2024-05-09  0:46       ` Kui-Feng Lee
2024-05-07  5:55 ` [PATCH bpf-next v2 3/6] bpf: support epoll from bpf struct_ops links Kui-Feng Lee
2024-05-07  5:55 ` [PATCH bpf-next v2 4/6] selftests/bpf: test struct_ops with epoll Kui-Feng Lee
2024-05-08 23:34   ` Martin KaFai Lau
2024-05-09  0:22     ` Kui-Feng Lee
2024-05-07  5:55 ` [PATCH bpf-next v2 5/6] selftests/bpf: detach a struct_ops link from the subsystem managing it Kui-Feng Lee
2024-05-08 23:50   ` Martin KaFai Lau
2024-05-09  5:50     ` Kui-Feng Lee
2024-05-07  5:56 ` [PATCH bpf-next v2 6/6] selftests/bpf: make sure bpf_testmod handling racing link destroying well Kui-Feng Lee
2024-05-09  0:04   ` Martin KaFai Lau
2024-05-09 17:02     ` Kui-Feng Lee

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox