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

[Alan Maguire <alan.maguire@oracle.com>]

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