[PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Alan Maguire]

add simple kfuncs to create/destroy a context type to bpf_testmod,
register them and add a kfunc_call test to use them.  This provides
test coverage for registration of dtor kfuncs from modules.

Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 46 +++++++++++++++++++
 .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  9 ++++
 .../selftests/bpf/prog_tests/kfunc_call.c     |  1 +
 .../selftests/bpf/progs/kfunc_call_test.c     | 14 ++++++
 4 files changed, 70 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 49f9a311e49b..894cb31f906b 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -159,6 +159,37 @@ __bpf_kfunc void bpf_kfunc_dynptr_test(struct bpf_dynptr *ptr,
 {
 }
 
+__bpf_kfunc struct bpf_testmod_ctx *
+bpf_testmod_ctx_create(int *err)
+{
+	struct bpf_testmod_ctx *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx) {
+		*err = -ENOMEM;
+		return NULL;
+	}
+	refcount_set(&ctx->usage, 1);
+
+	return ctx;
+}
+
+static void testmod_free_cb(struct rcu_head *head)
+{
+	struct bpf_testmod_ctx *ctx;
+
+	ctx = container_of(head, struct bpf_testmod_ctx, rcu);
+	kfree(ctx);
+}
+
+__bpf_kfunc void bpf_testmod_ctx_release(struct bpf_testmod_ctx *ctx)
+{
+	if (!ctx)
+		return;
+	if (refcount_dec_and_test(&ctx->usage))
+		call_rcu(&ctx->rcu, testmod_free_cb);
+}
+
 struct bpf_testmod_btf_type_tag_1 {
 	int a;
 };
@@ -369,8 +400,14 @@ BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_testmod_seq_destroy, KF_ITER_DESTROY)
 BTF_ID_FLAGS(func, bpf_kfunc_common_test)
 BTF_ID_FLAGS(func, bpf_kfunc_dynptr_test)
+BTF_ID_FLAGS(func, bpf_testmod_ctx_create, KF_ACQUIRE | KF_RET_NULL)
+BTF_ID_FLAGS(func, bpf_testmod_ctx_release, KF_RELEASE)
 BTF_KFUNCS_END(bpf_testmod_common_kfunc_ids)
 
+BTF_ID_LIST(bpf_testmod_dtor_ids)
+BTF_ID(struct, bpf_testmod_ctx)
+BTF_ID(func, bpf_testmod_ctx_release)
+
 static const struct btf_kfunc_id_set bpf_testmod_common_kfunc_set = {
 	.owner = THIS_MODULE,
 	.set   = &bpf_testmod_common_kfunc_ids,
@@ -904,6 +941,12 @@ extern int bpf_fentry_test1(int a);
 
 static int bpf_testmod_init(void)
 {
+	const struct btf_id_dtor_kfunc bpf_testmod_dtors[] = {
+		{
+			.btf_id		= bpf_testmod_dtor_ids[0],
+			.kfunc_btf_id	= bpf_testmod_dtor_ids[1]
+		},
+	};
 	int ret;
 
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_UNSPEC, &bpf_testmod_common_kfunc_set);
@@ -912,6 +955,9 @@ static int bpf_testmod_init(void)
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &bpf_testmod_kfunc_set);
 	ret = ret ?: register_bpf_struct_ops(&bpf_bpf_testmod_ops, bpf_testmod_ops);
 	ret = ret ?: register_bpf_struct_ops(&bpf_testmod_ops2, bpf_testmod_ops2);
+	ret = ret ?: register_btf_id_dtor_kfuncs(bpf_testmod_dtors,
+						 ARRAY_SIZE(bpf_testmod_dtors),
+						 THIS_MODULE);
 	if (ret < 0)
 		return ret;
 	if (bpf_fentry_test1(0) < 0)
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 f9809517e7fa..e587a79f2239 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod_kfunc.h
@@ -80,6 +80,11 @@ struct sendmsg_args {
 	int msglen;
 };
 
+struct bpf_testmod_ctx {
+	struct callback_head	rcu;
+	refcount_t		usage;
+};
+
 struct prog_test_ref_kfunc *
 bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr) __ksym;
 void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p) __ksym;
@@ -135,4 +140,8 @@ int bpf_kfunc_call_kernel_getsockname(struct addr_args *args) __ksym;
 int bpf_kfunc_call_kernel_getpeername(struct addr_args *args) __ksym;
 
 void bpf_kfunc_dynptr_test(struct bpf_dynptr *ptr, struct bpf_dynptr *ptr__nullable) __ksym;
+
+struct bpf_testmod_ctx *bpf_testmod_ctx_create(int *err) __ksym;
+void bpf_testmod_ctx_release(struct bpf_testmod_ctx *ctx) __ksym;
+
 #endif /* _BPF_TESTMOD_KFUNC_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
index 2eb71559713c..5b743212292f 100644
--- a/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
+++ b/tools/testing/selftests/bpf/prog_tests/kfunc_call.c
@@ -78,6 +78,7 @@ static struct kfunc_test_params kfunc_tests[] = {
 	SYSCALL_TEST(kfunc_syscall_test, 0),
 	SYSCALL_NULL_CTX_TEST(kfunc_syscall_test_null, 0),
 	TC_TEST(kfunc_call_test_static_unused_arg, 0),
+	TC_TEST(kfunc_call_ctx, 0),
 };
 
 struct syscall_test_args {
diff --git a/tools/testing/selftests/bpf/progs/kfunc_call_test.c b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
index cf68d1e48a0f..c4174d9e1501 100644
--- a/tools/testing/selftests/bpf/progs/kfunc_call_test.c
+++ b/tools/testing/selftests/bpf/progs/kfunc_call_test.c
@@ -177,4 +177,18 @@ int kfunc_call_test_static_unused_arg(struct __sk_buff *skb)
 	return actual != expected ? -1 : 0;
 }
 
+SEC("tc")
+int kfunc_call_ctx(struct __sk_buff *skb)
+{
+	struct bpf_testmod_ctx *ctx;
+	int err = 0;
+
+	ctx = bpf_testmod_ctx_create(&err);
+	if (!ctx && !err)
+		err = -1;
+	if (ctx)
+		bpf_testmod_ctx_release(ctx);
+	return err;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.31.1

Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Alan Maguire]

On 18/06/2024 23:27, Eduard Zingerman wrote:
> On Tue, 2024-06-18 at 17:04 +0100, Alan Maguire wrote:
>> add simple kfuncs to create/destroy a context type to bpf_testmod,
>> register them and add a kfunc_call test to use them.  This provides
>> test coverage for registration of dtor kfuncs from modules.
>>
>> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
>> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>> ---
>
> Hi Alan,
>
> Thank you for adding this test, I think it is fine except one defect
below.
>
>>  .../selftests/bpf/bpf_testmod/bpf_testmod.c   | 46 +++++++++++++++++++
>>  .../bpf/bpf_testmod/bpf_testmod_kfunc.h       |  9 ++++
>>  .../selftests/bpf/prog_tests/kfunc_call.c     |  1 +
>>  .../selftests/bpf/progs/kfunc_call_test.c     | 14 ++++++
>>  4 files changed, 70 insertions(+)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> index 49f9a311e49b..894cb31f906b 100644
>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>> @@ -159,6 +159,37 @@ __bpf_kfunc void bpf_kfunc_dynptr_test(struct
bpf_dynptr *ptr,
>>  {
>>  }
>>
>> +__bpf_kfunc struct bpf_testmod_ctx *
>> +bpf_testmod_ctx_create(int *err)
>> +{
>> +	struct bpf_testmod_ctx *ctx;
>> +
>> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>
> Note: I get the following message in the kernel log when I run this test:
>
> [   34.168244] BUG: sleeping function called from invalid context at
include/linux/sched/mm.h:337
> [   34.168633] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid:
185, name: test_progs
> [   34.168838] preempt_count: 200, expected: 0
> [   34.168926] RCU nest depth: 1, expected: 0
> [   34.168989] 1 lock held by test_progs/185:
> [   34.169056]  #0: ffffffff83198a60 (rcu_read_lock){....}-{1:2}, at:
bpf_test_timer_enter+0x1d/0xb0
> [   34.169056] Preemption disabled at:
> [   34.169056] [<ffffffff81a0eeea>] bpf_test_run+0x16a/0x300
> [   34.169397] CPU: 0 PID: 185 Comm: test_progs Tainted: G
OE      6.10.0-rc2-00763-g6dba637e3bf3-dirty #31
> [   34.169557] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS 1.15.0-1 04/01/2014
> [   34.169679] Call Trace:
> [   34.169731]  <TASK>
> [   34.169767]  dump_stack_lvl+0x83/0xa0
> [   34.169828]  __might_resched+0x199/0x2b0
> [   34.169884]  kmalloc_trace_noprof+0x273/0x320
> [   34.169954]  ? srso_alias_return_thunk+0x5/0xfbef5
> [   34.170034]  ? bpf_test_run+0xc0/0x300
> [   34.170096]  ? bpf_testmod_ctx_create+0x23/0x50 [bpf_testmod]
> [   34.170169]  ? srso_alias_return_thunk+0x5/0xfbef5
> [   34.170241]  bpf_testmod_ctx_create+0x23/0x50 [bpf_testmod]
> [   34.170328]  bpf_prog_9591c1d0a1bb3a0f_kfunc_call_ctx+0x2b/0x58
> [   34.170394]  bpf_test_run+0x198/0x300
> [   34.170394]  ? srso_alias_return_thunk+0x5/0xfbef5
> [   34.170394]  ? lockdep_init_map_type+0x4b/0x250
> [   34.170394]  bpf_prog_test_run_skb+0x381/0x7f0
> [   34.170394]  __sys_bpf+0xc4f/0x2e00
> [   34.170394]  ? srso_alias_return_thunk+0x5/0xfbef5
> [   34.170394]  ? reacquire_held_locks+0xcf/0x1f0
> [   34.170394]  __x64_sys_bpf+0x1e/0x30
> [   34.170394]  do_syscall_64+0x68/0x140
> [   34.170394]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [   34.170394] RIP: 0033:0x7ff25a1161bd
>


oops, missed a GFP_ATOMIC here to avoid possible sleeping. To use
existing kfunc call test structure it's simpler to do this than add a
sleepable test context I think, especially since the focus here is on
adding a basic test. More below..

On 19/06/2024 00:28, Eduard Zingerman wrote:
> On Tue, 2024-06-18 at 15:27 -0700, Eduard Zingerman wrote:
>> On Tue, 2024-06-18 at 17:04 +0100, Alan Maguire wrote:
> 
> [...]
> >>> diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>> index 49f9a311e49b..894cb31f906b 100644
>>> --- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>> +++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
>>> @@ -159,6 +159,37 @@ __bpf_kfunc void bpf_kfunc_dynptr_test(struct bpf_dynptr *ptr,
>>>  {
>>>  }
>>>   
>>> +__bpf_kfunc struct bpf_testmod_ctx *
>>> +bpf_testmod_ctx_create(int *err)
>>> +{
>>> +	struct bpf_testmod_ctx *ctx;
>>> +
>>> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>>> +	if (!ctx) {
>>> +		*err = -ENOMEM;
>>> +		return NULL;
>>> +	}
>>> +	refcount_set(&ctx->usage, 1);
>>> +
>>> +	return ctx;
>>> +}
> 
> One more note:
> As far as I understand, we only test the logic inside
> register_btf_id_dtor_kfuncs() in this test case.
> The dtor logic seem to be triggered only for fields of structures that
> reside in certain types of objects, e.g. arraymap or other places
> where bpf_obj_free_fields() is called.
> So, the full dtor test might look as follows:
> - allocate such map and put an object there;
> - deallocate the map and verify that dtor kfunc was really called.
> If we consider this too much of a hassle (which it probably is),
> the body of both kfunc and accompanying bpf program could be empty.
> Please correct me if I'm wrong.

Yeah, my focus here was testing the registration to be honest and
thankfully as you noted it caught a case where I had forgotten to do id
relocation, so thanks for suggesting this!

To trigger the dtor cleanup via a map, I came up with the following:

- call bpf_testmod_ctx_create()
- do bpf_kptr_xchg(&ctx_val->ctx, ctx) to transfer the ctx kptr into the
map value;
- only release the reference if the kptr exchange fails
- and then it gets cleaned up on exit.

I haven't used kptrs much so hopefully that's right.

Tracing I confirmed cleanup happens via:

$ sudo dtrace -n 'fbt::bpf_testmod_ctx_release:entry { stack(); }'
dtrace: description 'fbt::bpf_testmod_ctx_release:entry ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  3 113779    bpf_testmod_ctx_release:entry
              vmlinux`array_map_free+0x69
              vmlinux`bpf_map_free_deferred+0x62
              vmlinux`process_one_work+0x192
              vmlinux`worker_thread+0x27a
              vmlinux`kthread+0xf7
              vmlinux`ret_from_fork+0x41
              vmlinux`ret_from_fork_asm+0x1a

Does the above sound right? Thanks!

Alan

Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Eduard Zingerman]

On Wed, 2024-06-19 at 17:45 +0100, Alan Maguire wrote:

[...]

> oops, missed a GFP_ATOMIC here to avoid possible sleeping. To use
> existing kfunc call test structure it's simpler to do this than add a
> sleepable test context I think, especially since the focus here is on
> adding a basic test. More below..

Hi Alan,

And I agree, GFP_ATOMIC should probably help and it is simpler than
adding a sleepable context.

[...]

> Yeah, my focus here was testing the registration to be honest and
> thankfully as you noted it caught a case where I had forgotten to do id
> relocation, so thanks for suggesting this!
> 
> To trigger the dtor cleanup via a map, I came up with the following:
> 
> - call bpf_testmod_ctx_create()
> - do bpf_kptr_xchg(&ctx_val->ctx, ctx) to transfer the ctx kptr into the
> map value;
> - only release the reference if the kptr exchange fails
> - and then it gets cleaned up on exit.
> 
> I haven't used kptrs much so hopefully that's right.
> 
> Tracing I confirmed cleanup happens via:
> 
> $ sudo dtrace -n 'fbt::bpf_testmod_ctx_release:entry { stack(); }'
> dtrace: description 'fbt::bpf_testmod_ctx_release:entry ' matched 1 probe
> CPU     ID                    FUNCTION:NAME
>   3 113779    bpf_testmod_ctx_release:entry
>               vmlinux`array_map_free+0x69
>               vmlinux`bpf_map_free_deferred+0x62
>               vmlinux`process_one_work+0x192
>               vmlinux`worker_thread+0x27a
>               vmlinux`kthread+0xf7
>               vmlinux`ret_from_fork+0x41
>               vmlinux`ret_from_fork_asm+0x1a
> 
> Does the above sound right? Thanks!

It does, might as well set some flag in the dtor kfunc and check it in
the program (using another map?). Tbh, I thought we could get away w/o
complicating the test, but since you already have it working, we should
probably add it.

Thanks,
Eduard

Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Alan Maguire]

On 19/06/2024 17:54, Eduard Zingerman wrote:
> On Wed, 2024-06-19 at 17:45 +0100, Alan Maguire wrote:
> 
> [...]
> 
>> oops, missed a GFP_ATOMIC here to avoid possible sleeping. To use
>> existing kfunc call test structure it's simpler to do this than add a
>> sleepable test context I think, especially since the focus here is on
>> adding a basic test. More below..
> 
> Hi Alan,
> 
> And I agree, GFP_ATOMIC should probably help and it is simpler than
> adding a sleepable context.
> 
> [...]
> 
>> Yeah, my focus here was testing the registration to be honest and
>> thankfully as you noted it caught a case where I had forgotten to do id
>> relocation, so thanks for suggesting this!
>>
>> To trigger the dtor cleanup via a map, I came up with the following:
>>
>> - call bpf_testmod_ctx_create()
>> - do bpf_kptr_xchg(&ctx_val->ctx, ctx) to transfer the ctx kptr into the
>> map value;
>> - only release the reference if the kptr exchange fails
>> - and then it gets cleaned up on exit.
>>
>> I haven't used kptrs much so hopefully that's right.
>>
>> Tracing I confirmed cleanup happens via:
>>
>> $ sudo dtrace -n 'fbt::bpf_testmod_ctx_release:entry { stack(); }'
>> dtrace: description 'fbt::bpf_testmod_ctx_release:entry ' matched 1 probe
>> CPU     ID                    FUNCTION:NAME
>>   3 113779    bpf_testmod_ctx_release:entry
>>               vmlinux`array_map_free+0x69
>>               vmlinux`bpf_map_free_deferred+0x62
>>               vmlinux`process_one_work+0x192
>>               vmlinux`worker_thread+0x27a
>>               vmlinux`kthread+0xf7
>>               vmlinux`ret_from_fork+0x41
>>               vmlinux`ret_from_fork_asm+0x1a
>>
>> Does the above sound right? Thanks!
> 
> It does, might as well set some flag in the dtor kfunc and check it in
> the program (using another map?). Tbh, I thought we could get away w/o
> 

Sorry, I'm not following here. So I think what you'd like is a way to
verify that the dtor actually runs, is that right? The problem there is
that the map cleanup gets run when the skeleton gets destroyed, but then
it's too late then to collect a count value via that BPF object.

The only thing I can think of is to create an additional tracing object
that we separately load/attach to bpf_testmod_ctx_release() prior to
running kfunc call tests to verify that the destructor fires on cleanup
of the kfunc test skeletons. Is that what you have in mind? Thanks!

Alan

Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Eduard Zingerman]

On Wed, 2024-06-19 at 18:42 +0100, Alan Maguire wrote:

[...]

> Sorry, I'm not following here. So I think what you'd like is a way to
> verify that the dtor actually runs, is that right? The problem there is
> that the map cleanup gets run when the skeleton gets destroyed, but then
> it's too late then to collect a count value via that BPF object.
> 
> The only thing I can think of is to create an additional tracing object
> that we separately load/attach to bpf_testmod_ctx_release() prior to
> running kfunc call tests to verify that the destructor fires on cleanup
> of the kfunc test skeletons. Is that what you have in mind? Thanks!

Tracing program could be an option, yes.
I was thinking about some map created by the driver program (the one
from prog_tests) that could be updated by destructor.
There is a question of how to pass the map FD to the kfunc,
probably it could be passed in a constructor for the kfunc and stored
in the context. But tracing program sounds good as well.

Again, it might be the case that checking that registration logic
works is sufficient. In such a case bodies of both kfunc and BPF
program could be empty.

Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Alan Maguire]

On 19/06/2024 21:12, Eduard Zingerman wrote:
> On Wed, 2024-06-19 at 18:42 +0100, Alan Maguire wrote:
> 
> [...]
> 
>> Sorry, I'm not following here. So I think what you'd like is a way to
>> verify that the dtor actually runs, is that right? The problem there is
>> that the map cleanup gets run when the skeleton gets destroyed, but then
>> it's too late then to collect a count value via that BPF object.
>>
>> The only thing I can think of is to create an additional tracing object
>> that we separately load/attach to bpf_testmod_ctx_release() prior to
>> running kfunc call tests to verify that the destructor fires on cleanup
>> of the kfunc test skeletons. Is that what you have in mind? Thanks!
> 
> Tracing program could be an option, yes.
> I was thinking about some map created by the driver program (the one
> from prog_tests) that could be updated by destructor.
> There is a question of how to pass the map FD to the kfunc,
> probably it could be passed in a constructor for the kfunc and stored
> in the context. But tracing program sounds good as well.
> 
> Again, it might be the case that checking that registration logic
> works is sufficient. In such a case bodies of both kfunc and BPF
> program could be empty.
> 

I explored this further. Even adding a separate skeleton with tracing
prog to catch release is insufficient because the map free is deferred
and the test can have already run by the time the deferred map free is
called. Whatever mechanism we use to detect release would be subject to
the timing of that it seems. This seems like a recipe for a flaky test
and I'd rather not add sleeps etc so I've stuck with the current
approach which exercises the dtor codepaths but doesn't verify release.
Thanks!

Alan

Re: [PATCH bpf-next 5/5] selftests/bpf: add kfunc_call test for simple dtor in bpf_testmod

[Eduard Zingerman]

On Thu, 2024-06-20 at 10:17 +0100, Alan Maguire wrote:

[...]

> I explored this further. Even adding a separate skeleton with tracing
> prog to catch release is insufficient because the map free is deferred
> and the test can have already run by the time the deferred map free is
> called. Whatever mechanism we use to detect release would be subject to
> the timing of that it seems. This seems like a recipe for a flaky test
> and I'd rather not add sleeps etc so I've stuck with the current
> approach which exercises the dtor codepaths but doesn't verify release.
> Thanks!

Makes sense, thank you for taking a look.