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@xxxxxxxxx>
>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>> ---
>
> 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
