[v4,bpf-next,5/5] selftests/bpf: Test bpf_kptr_xchg stashing into local kptr

Commit Message

Amery Hung Aug. 13, 2024, 9:24 p.m. UTC

From: Dave Marchevsky <[e-mail]>

Test stashing both referenced kptr and local kptr into local kptrs. Then,
test unstashing them.

Acked-by: Martin KaFai Lau <[e-mail]>
Acked-by: Hou Tao <[e-mail]>
Signed-off-by: Dave Marchevsky <[e-mail]>
Signed-off-by: Amery Hung <[e-mail]>
---
 .../selftests/bpf/progs/local_kptr_stash.c    | 30 +++++++++++++++++--
 .../selftests/bpf/progs/task_kfunc_success.c  | 26 +++++++++++++++-
 2 files changed, 53 insertions(+), 3 deletions(-)

Comments

Alexei Starovoitov Aug. 23, 2024, 6:48 p.m. UTC | #1

On Tue, Aug 13, 2024 at 2:24 PM Amery Hung <[e-mail]> wrote:
>
> From: Dave Marchevsky <[e-mail]>
>
> Test stashing both referenced kptr and local kptr into local kptrs. Then,
> test unstashing them.
>
> Acked-by: Martin KaFai Lau <[e-mail]>
> Acked-by: Hou Tao <[e-mail]>
> Signed-off-by: Dave Marchevsky <[e-mail]>
> Signed-off-by: Amery Hung <[e-mail]>
> ---
>  .../selftests/bpf/progs/local_kptr_stash.c    | 30 +++++++++++++++++--
>  .../selftests/bpf/progs/task_kfunc_success.c  | 26 +++++++++++++++-
>  2 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> index 75043ffc5dad..b092a72b2c9d 100644
> --- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> +++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> @@ -8,9 +8,12 @@
>  #include "../bpf_experimental.h"
>  #include "../bpf_testmod/bpf_testmod_kfunc.h"
>
> +struct plain_local;
> +
>  struct node_data {
>         long key;
>         long data;
> +       struct plain_local __kptr * stashed_in_local_kptr;
>         struct bpf_rb_node node;
>  };

Everything looks correct and I applied the set.
The selftest sort-of covers the case where stashed_in_local_kptr
is being freed by rb_root recursive freeing,
but it doesn't really check for memory leaks.
It only checks that nothing will crash.

Please follow up with an improvement to selftest that
actually makes sure that recursive freeing of stashed kptr
correctly calls bpf_obj_free_fields->__bpf_obj_drop_impl.

The patches seem to do the right thing in terms of storing
correct btf/records in the right places,
but this is tricky, so extra tests are warranted.

Amery Hung Aug. 23, 2024, 11:59 p.m. UTC | #2

On Fri, Aug 23, 2024 at 11:49 AM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Tue, Aug 13, 2024 at 2:24 PM Amery Hung <[e-mail]> wrote:
> >
> > From: Dave Marchevsky <[e-mail]>
> >
> > Test stashing both referenced kptr and local kptr into local kptrs. Then,
> > test unstashing them.
> >
> > Acked-by: Martin KaFai Lau <[e-mail]>
> > Acked-by: Hou Tao <[e-mail]>
> > Signed-off-by: Dave Marchevsky <[e-mail]>
> > Signed-off-by: Amery Hung <[e-mail]>
> > ---
> >  .../selftests/bpf/progs/local_kptr_stash.c    | 30 +++++++++++++++++--
> >  .../selftests/bpf/progs/task_kfunc_success.c  | 26 +++++++++++++++-
> >  2 files changed, 53 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> > index 75043ffc5dad..b092a72b2c9d 100644
> > --- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> > +++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
> > @@ -8,9 +8,12 @@
> >  #include "../bpf_experimental.h"
> >  #include "../bpf_testmod/bpf_testmod_kfunc.h"
> >
> > +struct plain_local;
> > +
> >  struct node_data {
> >         long key;
> >         long data;
> > +       struct plain_local __kptr * stashed_in_local_kptr;
> >         struct bpf_rb_node node;
> >  };
>
> Everything looks correct and I applied the set.
> The selftest sort-of covers the case where stashed_in_local_kptr
> is being freed by rb_root recursive freeing,
> but it doesn't really check for memory leaks.
> It only checks that nothing will crash.
>
> Please follow up with an improvement to selftest that
> actually makes sure that recursive freeing of stashed kptr
> correctly calls bpf_obj_free_fields->__bpf_obj_drop_impl.
>

Will do. Thanks for reviewing the patchset!

> The patches seem to do the right thing in terms of storing
> correct btf/records in the right places,
> but this is tricky, so extra tests are warranted.

Patch

diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
index 75043ffc5dad..b092a72b2c9d 100644
--- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c
+++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c
@@ -8,9 +8,12 @@ 
 #include "../bpf_experimental.h"
 #include "../bpf_testmod/bpf_testmod_kfunc.h"
 
+struct plain_local;
+
 struct node_data {
 	long key;
 	long data;
+	struct plain_local __kptr * stashed_in_local_kptr;
 	struct bpf_rb_node node;
 };
 
@@ -85,6 +88,7 @@  static bool less(struct bpf_rb_node *a, const struct bpf_rb_node *b)
 
 static int create_and_stash(int idx, int val)
 {
+	struct plain_local *inner_local_kptr;
 	struct map_value *mapval;
 	struct node_data *res;
 
@@ -92,11 +96,25 @@  static int create_and_stash(int idx, int val)
 	if (!mapval)
 		return 1;
 
+	inner_local_kptr = bpf_obj_new(typeof(*inner_local_kptr));
+	if (!inner_local_kptr)
+		return 2;
+
 	res = bpf_obj_new(typeof(*res));
-	if (!res)
-		return 1;
+	if (!res) {
+		bpf_obj_drop(inner_local_kptr);
+		return 3;
+	}
 	res->key = val;
 
+	inner_local_kptr = bpf_kptr_xchg(&res->stashed_in_local_kptr, inner_local_kptr);
+	if (inner_local_kptr) {
+		/* Should never happen, we just obj_new'd res */
+		bpf_obj_drop(inner_local_kptr);
+		bpf_obj_drop(res);
+		return 4;
+	}
+
 	res = bpf_kptr_xchg(&mapval->node, res);
 	if (res)
 		bpf_obj_drop(res);
@@ -169,6 +187,7 @@  long stash_local_with_root(void *ctx)
 SEC("tc")
 long unstash_rb_node(void *ctx)
 {
+	struct plain_local *inner_local_kptr = NULL;
 	struct map_value *mapval;
 	struct node_data *res;
 	long retval;
@@ -180,6 +199,13 @@  long unstash_rb_node(void *ctx)
 
 	res = bpf_kptr_xchg(&mapval->node, NULL);
 	if (res) {
+		inner_local_kptr = bpf_kptr_xchg(&res->stashed_in_local_kptr, inner_local_kptr);
+		if (!inner_local_kptr) {
+			bpf_obj_drop(res);
+			return 1;
+		}
+		bpf_obj_drop(inner_local_kptr);
+
 		retval = res->key;
 		bpf_obj_drop(res);
 		return retval;
diff --git a/tools/testing/selftests/bpf/progs/task_kfunc_success.c b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
index 70df695312dc..3138bb689b0b 100644
--- a/tools/testing/selftests/bpf/progs/task_kfunc_success.c
+++ b/tools/testing/selftests/bpf/progs/task_kfunc_success.c
@@ -5,6 +5,7 @@ 
 #include <bpf/bpf_tracing.h>
 #include <bpf/bpf_helpers.h>
 
+#include "../bpf_experimental.h"
 #include "task_kfunc_common.h"
 
 char _license[] SEC("license") = "GPL";
@@ -143,7 +144,7 @@  SEC("tp_btf/task_newtask")
 int BPF_PROG(test_task_xchg_release, struct task_struct *task, u64 clone_flags)
 {
 	struct task_struct *kptr;
-	struct __tasks_kfunc_map_value *v;
+	struct __tasks_kfunc_map_value *v, *local;
 	long status;
 
 	if (!is_test_kfunc_task())
@@ -167,6 +168,29 @@  int BPF_PROG(test_task_xchg_release, struct task_struct *task, u64 clone_flags)
 		return 0;
 	}
 
+	local = bpf_obj_new(typeof(*local));
+	if (!local) {
+		err = 4;
+		bpf_task_release(kptr);
+		return 0;
+	}
+
+	kptr = bpf_kptr_xchg(&local->task, kptr);
+	if (kptr) {
+		err = 5;
+		bpf_obj_drop(local);
+		bpf_task_release(kptr);
+		return 0;
+	}
+
+	kptr = bpf_kptr_xchg(&local->task, NULL);
+	if (!kptr) {
+		err = 6;
+		bpf_obj_drop(local);
+		return 0;
+	}
+
+	bpf_obj_drop(local);
 	bpf_task_release(kptr);
 
 	return 0;