* [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr @ 2023-08-23 22:55 Yonghong Song 2023-08-23 22:56 ` [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields Yonghong Song 2023-08-24 5:16 ` [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr David Marchevsky 0 siblings, 2 replies; 5+ messages in thread From: Yonghong Song @ 2023-08-23 22:55 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team, Martin KaFai Lau Currently, in function bpf_obj_free_fields(), for local kptr, a warning will be issued if the struct does not contain any special fields. But actually the kernel seems totally okay with a local kptr without any special fields. Permitting no special fields also aligns with future percpu kptr which also allows no special fields. Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- kernel/bpf/syscall.c | 1 - 1 file changed, 1 deletion(-) NOTE: I didn't put a fix tag since except the warning there is no correctness issue here. diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 10666d17b9e3..ebeb0695305a 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -657,7 +657,6 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj) if (!btf_is_kernel(field->kptr.btf)) { pointee_struct_meta = btf_find_struct_meta(field->kptr.btf, field->kptr.btf_id); - WARN_ON_ONCE(!pointee_struct_meta); migrate_disable(); __bpf_obj_drop_impl(xchgd_field, pointee_struct_meta ? pointee_struct_meta->record : -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields 2023-08-23 22:55 [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr Yonghong Song @ 2023-08-23 22:56 ` Yonghong Song 2023-08-24 5:31 ` David Marchevsky 2023-08-24 5:16 ` [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr David Marchevsky 1 sibling, 1 reply; 5+ messages in thread From: Yonghong Song @ 2023-08-23 22:56 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team, Martin KaFai Lau Add a local kptr test with no special fields in the struct. Without the previous patch, the following warning will hit: [ 44.683877] WARNING: CPU: 3 PID: 485 at kernel/bpf/syscall.c:660 bpf_obj_free_fields+0x220/0x240 [ 44.684640] Modules linked in: bpf_testmod(OE) [ 44.685044] CPU: 3 PID: 485 Comm: kworker/u8:5 Tainted: G OE 6.5.0-rc5-01703-g260d855e9b90 #248 [ 44.685827] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 [ 44.686693] Workqueue: events_unbound bpf_map_free_deferred [ 44.687297] RIP: 0010:bpf_obj_free_fields+0x220/0x240 [ 44.687775] Code: e8 55 17 1f 00 49 8b 74 24 08 4c 89 ef e8 e8 14 05 00 e8 a3 da e2 ff e9 55 fe ff ff 0f 0b e9 4e fe ff ff 0f 0b e9 47 fe ff ff <0f> 0b e8 d9 d9 e2 ff 31 f6 eb d5 48 83 c4 10 5b 41 5c e [ 44.689353] RSP: 0018:ffff888106467cb8 EFLAGS: 00010246 [ 44.689806] RAX: 0000000000000000 RBX: ffff888112b3a200 RCX: 0000000000000001 [ 44.690433] RDX: 0000000000000000 RSI: dffffc0000000000 RDI: ffff8881128ad988 [ 44.691094] RBP: 0000000000000002 R08: ffffffff81370bd0 R09: 1ffff110216231a5 [ 44.691643] R10: dffffc0000000000 R11: ffffed10216231a6 R12: ffff88810d68a488 [ 44.692245] R13: ffff88810767c288 R14: ffff88810d68a400 R15: ffff88810d68a418 [ 44.692829] FS: 0000000000000000(0000) GS:ffff8881f7580000(0000) knlGS:0000000000000000 [ 44.693484] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 44.693964] CR2: 000055c7f2afce28 CR3: 000000010fee4002 CR4: 0000000000370ee0 [ 44.694513] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 44.695102] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 44.695747] Call Trace: [ 44.696001] <TASK> [ 44.696183] ? __warn+0xfe/0x270 [ 44.696447] ? bpf_obj_free_fields+0x220/0x240 [ 44.696817] ? report_bug+0x220/0x2d0 [ 44.697180] ? handle_bug+0x3d/0x70 [ 44.697507] ? exc_invalid_op+0x1a/0x50 [ 44.697887] ? asm_exc_invalid_op+0x1a/0x20 [ 44.698282] ? btf_find_struct_meta+0xd0/0xd0 [ 44.698634] ? bpf_obj_free_fields+0x220/0x240 [ 44.699027] ? bpf_obj_free_fields+0x1e2/0x240 [ 44.699414] array_map_free+0x1a3/0x260 [ 44.699763] bpf_map_free_deferred+0x7b/0xe0 [ 44.700154] process_one_work+0x46d/0x750 [ 44.700523] worker_thread+0x49e/0x900 [ 44.700892] ? pr_cont_work+0x270/0x270 [ 44.701224] kthread+0x1ae/0x1d0 [ 44.701516] ? kthread_blkcg+0x50/0x50 [ 44.701860] ret_from_fork+0x34/0x50 [ 44.702178] ? kthread_blkcg+0x50/0x50 [ 44.702508] ret_from_fork_asm+0x11/0x20 [ 44.702880] </TASK> With the previous patch, there is no warnings. Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- .../bpf/prog_tests/local_kptr_stash.c | 25 ++++++++++++++++- .../selftests/bpf/progs/local_kptr_stash.c | 28 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c index 158616c94658..4225108b8e4d 100644 --- a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c +++ b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c @@ -27,6 +27,27 @@ static void test_local_kptr_stash_simple(void) local_kptr_stash__destroy(skel); } +static void test_local_kptr_stash_simple_2(void) +{ + LIBBPF_OPTS(bpf_test_run_opts, opts, + .data_in = &pkt_v4, + .data_size_in = sizeof(pkt_v4), + .repeat = 1, + ); + struct local_kptr_stash *skel; + int ret; + + skel = local_kptr_stash__open_and_load(); + if (!ASSERT_OK_PTR(skel, "local_kptr_stash__open_and_load")) + return; + + ret = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.stash_rb_nodes_2), &opts); + ASSERT_OK(ret, "local_kptr_stash_add_nodes run"); + ASSERT_OK(opts.retval, "local_kptr_stash_add_nodes retval"); + + local_kptr_stash__destroy(skel); +} + static void test_local_kptr_stash_unstash(void) { LIBBPF_OPTS(bpf_test_run_opts, opts, @@ -59,8 +80,10 @@ static void test_local_kptr_stash_fail(void) void test_local_kptr_stash(void) { - if (test__start_subtest("local_kptr_stash_simple")) + if (test__start_subtest("local_kptr_stash_simple_yes_special_field")) test_local_kptr_stash_simple(); + if (test__start_subtest("local_kptr_stash_simple_no_special_field")) + test_local_kptr_stash_simple_2(); if (test__start_subtest("local_kptr_stash_unstash")) test_local_kptr_stash_unstash(); if (test__start_subtest("local_kptr_stash_fail")) diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c index 06838083079c..4de548c31aab 100644 --- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c +++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c @@ -14,10 +14,16 @@ struct node_data { struct bpf_rb_node node; }; +struct node_data2 { + long key; + long data; +}; + struct map_value { struct prog_test_ref_kfunc *not_kptr; struct prog_test_ref_kfunc __kptr *val; struct node_data __kptr *node; + struct node_data2 __kptr *node2; }; /* This is necessary so that LLVM generates BTF for node_data struct @@ -66,6 +72,28 @@ long stash_rb_nodes(void *ctx) return create_and_stash(0, 41) ?: create_and_stash(1, 42); } +SEC("tc") +long stash_rb_nodes_2(void *ctx) +{ + struct map_value *mapval; + struct node_data2 *res; + int idx = 0; + + mapval = bpf_map_lookup_elem(&some_nodes, &idx); + if (!mapval) + return 1; + + res = bpf_obj_new(typeof(*res)); + if (!res) + return 1; + res->key = 41; + + res = bpf_kptr_xchg(&mapval->node2, res); + if (res) + bpf_obj_drop(res); + return 0; +} + SEC("tc") long unstash_rb_node(void *ctx) { -- 2.34.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields 2023-08-23 22:56 ` [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields Yonghong Song @ 2023-08-24 5:31 ` David Marchevsky 2023-08-24 5:56 ` Yonghong Song 0 siblings, 1 reply; 5+ messages in thread From: David Marchevsky @ 2023-08-24 5:31 UTC (permalink / raw) To: Yonghong Song, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team, Martin KaFai Lau On 8/23/23 6:56 PM, Yonghong Song wrote: > Add a local kptr test with no special fields in the struct. Without the > previous patch, the following warning will hit: > > [ 44.683877] WARNING: CPU: 3 PID: 485 at kernel/bpf/syscall.c:660 bpf_obj_free_fields+0x220/0x240 > [ 44.684640] Modules linked in: bpf_testmod(OE) > [ 44.685044] CPU: 3 PID: 485 Comm: kworker/u8:5 Tainted: G OE 6.5.0-rc5-01703-g260d855e9b90 #248 > [ 44.685827] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 > [ 44.686693] Workqueue: events_unbound bpf_map_free_deferred > [ 44.687297] RIP: 0010:bpf_obj_free_fields+0x220/0x240 > [ 44.687775] Code: e8 55 17 1f 00 49 8b 74 24 08 4c 89 ef e8 e8 14 05 00 e8 a3 da e2 ff e9 55 fe ff ff 0f 0b e9 4e fe ff > ff 0f 0b e9 47 fe ff ff <0f> 0b e8 d9 d9 e2 ff 31 f6 eb d5 48 83 c4 10 5b 41 5c e > [ 44.689353] RSP: 0018:ffff888106467cb8 EFLAGS: 00010246 > [ 44.689806] RAX: 0000000000000000 RBX: ffff888112b3a200 RCX: 0000000000000001 > [ 44.690433] RDX: 0000000000000000 RSI: dffffc0000000000 RDI: ffff8881128ad988 > [ 44.691094] RBP: 0000000000000002 R08: ffffffff81370bd0 R09: 1ffff110216231a5 > [ 44.691643] R10: dffffc0000000000 R11: ffffed10216231a6 R12: ffff88810d68a488 > [ 44.692245] R13: ffff88810767c288 R14: ffff88810d68a400 R15: ffff88810d68a418 > [ 44.692829] FS: 0000000000000000(0000) GS:ffff8881f7580000(0000) knlGS:0000000000000000 > [ 44.693484] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 44.693964] CR2: 000055c7f2afce28 CR3: 000000010fee4002 CR4: 0000000000370ee0 > [ 44.694513] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 44.695102] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > [ 44.695747] Call Trace: > [ 44.696001] <TASK> > [ 44.696183] ? __warn+0xfe/0x270 > [ 44.696447] ? bpf_obj_free_fields+0x220/0x240 > [ 44.696817] ? report_bug+0x220/0x2d0 > [ 44.697180] ? handle_bug+0x3d/0x70 > [ 44.697507] ? exc_invalid_op+0x1a/0x50 > [ 44.697887] ? asm_exc_invalid_op+0x1a/0x20 > [ 44.698282] ? btf_find_struct_meta+0xd0/0xd0 > [ 44.698634] ? bpf_obj_free_fields+0x220/0x240 > [ 44.699027] ? bpf_obj_free_fields+0x1e2/0x240 > [ 44.699414] array_map_free+0x1a3/0x260 > [ 44.699763] bpf_map_free_deferred+0x7b/0xe0 > [ 44.700154] process_one_work+0x46d/0x750 > [ 44.700523] worker_thread+0x49e/0x900 > [ 44.700892] ? pr_cont_work+0x270/0x270 > [ 44.701224] kthread+0x1ae/0x1d0 > [ 44.701516] ? kthread_blkcg+0x50/0x50 > [ 44.701860] ret_from_fork+0x34/0x50 > [ 44.702178] ? kthread_blkcg+0x50/0x50 > [ 44.702508] ret_from_fork_asm+0x11/0x20 > [ 44.702880] </TASK> > > With the previous patch, there is no warnings. > > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- > .../bpf/prog_tests/local_kptr_stash.c | 25 ++++++++++++++++- > .../selftests/bpf/progs/local_kptr_stash.c | 28 +++++++++++++++++++ > 2 files changed, 52 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c > index 158616c94658..4225108b8e4d 100644 > --- a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c > +++ b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c > @@ -27,6 +27,27 @@ static void test_local_kptr_stash_simple(void) > local_kptr_stash__destroy(skel); > } > > +static void test_local_kptr_stash_simple_2(void) > +{ > + LIBBPF_OPTS(bpf_test_run_opts, opts, > + .data_in = &pkt_v4, > + .data_size_in = sizeof(pkt_v4), > + .repeat = 1, > + ); > + struct local_kptr_stash *skel; > + int ret; > + > + skel = local_kptr_stash__open_and_load(); > + if (!ASSERT_OK_PTR(skel, "local_kptr_stash__open_and_load")) > + return; > + > + ret = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.stash_rb_nodes_2), &opts); > + ASSERT_OK(ret, "local_kptr_stash_add_nodes run"); > + ASSERT_OK(opts.retval, "local_kptr_stash_add_nodes retval"); > + > + local_kptr_stash__destroy(skel); > +} > + > static void test_local_kptr_stash_unstash(void) > { > LIBBPF_OPTS(bpf_test_run_opts, opts, > @@ -59,8 +80,10 @@ static void test_local_kptr_stash_fail(void) > > void test_local_kptr_stash(void) > { > - if (test__start_subtest("local_kptr_stash_simple")) > + if (test__start_subtest("local_kptr_stash_simple_yes_special_field")) > test_local_kptr_stash_simple(); > + if (test__start_subtest("local_kptr_stash_simple_no_special_field")) > + test_local_kptr_stash_simple_2(); nit: Can you use same name in if (test__start_subtest("$NAME")) $NAME(); so test_local_kptr_stash_simple would be renamed to local_kptr_stash_simple_yes_special_field and similar for test_local_kptr_stash_simple_2. This way 'git grep' for failing subtest name will quickly find the right prog_tests subtest runner func. > if (test__start_subtest("local_kptr_stash_unstash")) > test_local_kptr_stash_unstash(); > if (test__start_subtest("local_kptr_stash_fail")) > diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c > index 06838083079c..4de548c31aab 100644 > --- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c > +++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c > @@ -14,10 +14,16 @@ struct node_data { > struct bpf_rb_node node; > }; > > +struct node_data2 { > + long key; > + long data; > +}; > + Since this has no special fields, it's not a collection node. I've been using 'node_data' and similar naming pattern in selftests for collection nodes specifically, this muddles the meaning a bit. Can the name be changed to something else? 'struct plain_local' maybe? I don't feel strongly about 'plain_local', though, anything distinct enough from 'node_data' is fine by me. > struct map_value { > struct prog_test_ref_kfunc *not_kptr; > struct prog_test_ref_kfunc __kptr *val; > struct node_data __kptr *node; > + struct node_data2 __kptr *node2; Similar naming nit here. Maybe 'node2' -> 'plain'? Aside from the naming nits, LGTM. > }; > > /* This is necessary so that LLVM generates BTF for node_data struct > @@ -66,6 +72,28 @@ long stash_rb_nodes(void *ctx) > return create_and_stash(0, 41) ?: create_and_stash(1, 42); > } > > +SEC("tc") > +long stash_rb_nodes_2(void *ctx) > +{ > + struct map_value *mapval; > + struct node_data2 *res; > + int idx = 0; > + > + mapval = bpf_map_lookup_elem(&some_nodes, &idx); > + if (!mapval) > + return 1; > + > + res = bpf_obj_new(typeof(*res)); > + if (!res) > + return 1; > + res->key = 41; > + > + res = bpf_kptr_xchg(&mapval->node2, res); > + if (res) > + bpf_obj_drop(res); > + return 0; > +} > + > SEC("tc") > long unstash_rb_node(void *ctx) > { ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields 2023-08-24 5:31 ` David Marchevsky @ 2023-08-24 5:56 ` Yonghong Song 0 siblings, 0 replies; 5+ messages in thread From: Yonghong Song @ 2023-08-24 5:56 UTC (permalink / raw) To: David Marchevsky, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team, Martin KaFai Lau On 8/23/23 10:31 PM, David Marchevsky wrote: > On 8/23/23 6:56 PM, Yonghong Song wrote: >> Add a local kptr test with no special fields in the struct. Without the >> previous patch, the following warning will hit: >> >> [ 44.683877] WARNING: CPU: 3 PID: 485 at kernel/bpf/syscall.c:660 bpf_obj_free_fields+0x220/0x240 >> [ 44.684640] Modules linked in: bpf_testmod(OE) >> [ 44.685044] CPU: 3 PID: 485 Comm: kworker/u8:5 Tainted: G OE 6.5.0-rc5-01703-g260d855e9b90 #248 >> [ 44.685827] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014 >> [ 44.686693] Workqueue: events_unbound bpf_map_free_deferred >> [ 44.687297] RIP: 0010:bpf_obj_free_fields+0x220/0x240 >> [ 44.687775] Code: e8 55 17 1f 00 49 8b 74 24 08 4c 89 ef e8 e8 14 05 00 e8 a3 da e2 ff e9 55 fe ff ff 0f 0b e9 4e fe ff >> ff 0f 0b e9 47 fe ff ff <0f> 0b e8 d9 d9 e2 ff 31 f6 eb d5 48 83 c4 10 5b 41 5c e >> [ 44.689353] RSP: 0018:ffff888106467cb8 EFLAGS: 00010246 >> [ 44.689806] RAX: 0000000000000000 RBX: ffff888112b3a200 RCX: 0000000000000001 >> [ 44.690433] RDX: 0000000000000000 RSI: dffffc0000000000 RDI: ffff8881128ad988 >> [ 44.691094] RBP: 0000000000000002 R08: ffffffff81370bd0 R09: 1ffff110216231a5 >> [ 44.691643] R10: dffffc0000000000 R11: ffffed10216231a6 R12: ffff88810d68a488 >> [ 44.692245] R13: ffff88810767c288 R14: ffff88810d68a400 R15: ffff88810d68a418 >> [ 44.692829] FS: 0000000000000000(0000) GS:ffff8881f7580000(0000) knlGS:0000000000000000 >> [ 44.693484] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 44.693964] CR2: 000055c7f2afce28 CR3: 000000010fee4002 CR4: 0000000000370ee0 >> [ 44.694513] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >> [ 44.695102] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >> [ 44.695747] Call Trace: >> [ 44.696001] <TASK> >> [ 44.696183] ? __warn+0xfe/0x270 >> [ 44.696447] ? bpf_obj_free_fields+0x220/0x240 >> [ 44.696817] ? report_bug+0x220/0x2d0 >> [ 44.697180] ? handle_bug+0x3d/0x70 >> [ 44.697507] ? exc_invalid_op+0x1a/0x50 >> [ 44.697887] ? asm_exc_invalid_op+0x1a/0x20 >> [ 44.698282] ? btf_find_struct_meta+0xd0/0xd0 >> [ 44.698634] ? bpf_obj_free_fields+0x220/0x240 >> [ 44.699027] ? bpf_obj_free_fields+0x1e2/0x240 >> [ 44.699414] array_map_free+0x1a3/0x260 >> [ 44.699763] bpf_map_free_deferred+0x7b/0xe0 >> [ 44.700154] process_one_work+0x46d/0x750 >> [ 44.700523] worker_thread+0x49e/0x900 >> [ 44.700892] ? pr_cont_work+0x270/0x270 >> [ 44.701224] kthread+0x1ae/0x1d0 >> [ 44.701516] ? kthread_blkcg+0x50/0x50 >> [ 44.701860] ret_from_fork+0x34/0x50 >> [ 44.702178] ? kthread_blkcg+0x50/0x50 >> [ 44.702508] ret_from_fork_asm+0x11/0x20 >> [ 44.702880] </TASK> >> >> With the previous patch, there is no warnings. >> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >> --- >> .../bpf/prog_tests/local_kptr_stash.c | 25 ++++++++++++++++- >> .../selftests/bpf/progs/local_kptr_stash.c | 28 +++++++++++++++++++ >> 2 files changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c >> index 158616c94658..4225108b8e4d 100644 >> --- a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c >> +++ b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c >> @@ -27,6 +27,27 @@ static void test_local_kptr_stash_simple(void) >> local_kptr_stash__destroy(skel); >> } >> >> +static void test_local_kptr_stash_simple_2(void) >> +{ >> + LIBBPF_OPTS(bpf_test_run_opts, opts, >> + .data_in = &pkt_v4, >> + .data_size_in = sizeof(pkt_v4), >> + .repeat = 1, >> + ); >> + struct local_kptr_stash *skel; >> + int ret; >> + >> + skel = local_kptr_stash__open_and_load(); >> + if (!ASSERT_OK_PTR(skel, "local_kptr_stash__open_and_load")) >> + return; >> + >> + ret = bpf_prog_test_run_opts(bpf_program__fd(skel->progs.stash_rb_nodes_2), &opts); >> + ASSERT_OK(ret, "local_kptr_stash_add_nodes run"); >> + ASSERT_OK(opts.retval, "local_kptr_stash_add_nodes retval"); >> + >> + local_kptr_stash__destroy(skel); >> +} >> + >> static void test_local_kptr_stash_unstash(void) >> { >> LIBBPF_OPTS(bpf_test_run_opts, opts, >> @@ -59,8 +80,10 @@ static void test_local_kptr_stash_fail(void) >> >> void test_local_kptr_stash(void) >> { >> - if (test__start_subtest("local_kptr_stash_simple")) >> + if (test__start_subtest("local_kptr_stash_simple_yes_special_field")) >> test_local_kptr_stash_simple(); >> + if (test__start_subtest("local_kptr_stash_simple_no_special_field")) >> + test_local_kptr_stash_simple_2(); > > nit: Can you use same name in > > if (test__start_subtest("$NAME")) > $NAME(); > > so test_local_kptr_stash_simple would > be renamed to local_kptr_stash_simple_yes_special_field > and similar for test_local_kptr_stash_simple_2. > > This way 'git grep' for failing subtest name > will quickly find the right prog_tests subtest > runner func. Good point! Ack. Will make them consistent. > >> if (test__start_subtest("local_kptr_stash_unstash")) >> test_local_kptr_stash_unstash(); >> if (test__start_subtest("local_kptr_stash_fail")) >> diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash.c b/tools/testing/selftests/bpf/progs/local_kptr_stash.c >> index 06838083079c..4de548c31aab 100644 >> --- a/tools/testing/selftests/bpf/progs/local_kptr_stash.c >> +++ b/tools/testing/selftests/bpf/progs/local_kptr_stash.c >> @@ -14,10 +14,16 @@ struct node_data { >> struct bpf_rb_node node; >> }; >> >> +struct node_data2 { >> + long key; >> + long data; >> +}; >> + > > Since this has no special fields, it's not a collection node. I've been using > 'node_data' and similar naming pattern in selftests for collection nodes > specifically, this muddles the meaning a bit. Can the name be changed to > something else? 'struct plain_local' maybe? I don't feel strongly about > 'plain_local', though, anything distinct enough from 'node_data' is fine by me. Okay, will use 'plain_local' which, I think, is good enough. > >> struct map_value { >> struct prog_test_ref_kfunc *not_kptr; >> struct prog_test_ref_kfunc __kptr *val; >> struct node_data __kptr *node; >> + struct node_data2 __kptr *node2; > > Similar naming nit here. Maybe 'node2' -> 'plain'? Sounds okay. Will send out v2 soon. > > Aside from the naming nits, LGTM. > >> }; >> >> /* This is necessary so that LLVM generates BTF for node_data struct >> @@ -66,6 +72,28 @@ long stash_rb_nodes(void *ctx) >> return create_and_stash(0, 41) ?: create_and_stash(1, 42); >> } >> >> +SEC("tc") >> +long stash_rb_nodes_2(void *ctx) >> +{ >> + struct map_value *mapval; >> + struct node_data2 *res; >> + int idx = 0; >> + >> + mapval = bpf_map_lookup_elem(&some_nodes, &idx); >> + if (!mapval) >> + return 1; >> + >> + res = bpf_obj_new(typeof(*res)); >> + if (!res) >> + return 1; >> + res->key = 41; >> + >> + res = bpf_kptr_xchg(&mapval->node2, res); >> + if (res) >> + bpf_obj_drop(res); >> + return 0; >> +} >> + >> SEC("tc") >> long unstash_rb_node(void *ctx) >> { ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr 2023-08-23 22:55 [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr Yonghong Song 2023-08-23 22:56 ` [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields Yonghong Song @ 2023-08-24 5:16 ` David Marchevsky 1 sibling, 0 replies; 5+ messages in thread From: David Marchevsky @ 2023-08-24 5:16 UTC (permalink / raw) To: Yonghong Song, bpf Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team, Martin KaFai Lau On 8/23/23 6:55 PM, Yonghong Song wrote: > Currently, in function bpf_obj_free_fields(), for local kptr, > a warning will be issued if the struct does not contain any > special fields. But actually the kernel seems totally okay > with a local kptr without any special fields. Permitting > no special fields also aligns with future percpu kptr which > also allows no special fields. > > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- Weird. Looking at the WARN_ON_ONCE now, I can't understand why I added it, and history of the series adding it doesn't have any clues. The same series added pointee_struct_meta ? pointee_struct_meta->record : NULL two lines below, so it's not clear what I was trying to protect against. Anyways, I agree that: * We can have a struct with a special __kptr field that points to some local kptr type * That local kptr 'pointee' type doesn't need to have any special fields, in which case pointee_struct_meta will rightly be NULL, a NULL record will be passed to __bpf_obj_drop_impl, which will handle it correctly. * In fact this is the same logic that bpf_obj_drop_impl does before calling its double-underscore cousin LGTM Acked-by: Dave Marchevsky <davemarchevsky@fb.com> > kernel/bpf/syscall.c | 1 - > 1 file changed, 1 deletion(-) > > NOTE: I didn't put a fix tag since except the warning > there is no correctness issue here. > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 10666d17b9e3..ebeb0695305a 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -657,7 +657,6 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj) > if (!btf_is_kernel(field->kptr.btf)) { > pointee_struct_meta = btf_find_struct_meta(field->kptr.btf, > field->kptr.btf_id); > - WARN_ON_ONCE(!pointee_struct_meta); > migrate_disable(); > __bpf_obj_drop_impl(xchgd_field, pointee_struct_meta ? > pointee_struct_meta->record : ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-24 5:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-23 22:55 [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr Yonghong Song 2023-08-23 22:56 ` [PATCH bpf-next 2/2] selftests/bpf: Add a local kptr test with no special fields Yonghong Song 2023-08-24 5:31 ` David Marchevsky 2023-08-24 5:56 ` Yonghong Song 2023-08-24 5:16 ` [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr David Marchevsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox