diff mbox series

[bpf-next,2/2] selftests/bpf: Add a local kptr test with no special fields

Message ID 20230823225601.1293468-1-yonghong.song@linux.dev (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next,1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers warning 13 maintainers not CCed: void@manifault.com davemarchevsky@fb.com kpsingh@kernel.org martin.lau@linux.dev memxor@gmail.com john.fastabend@gmail.com sdf@google.com shuah@kernel.org song@kernel.org mykolal@fb.com linux-kselftest@vger.kernel.org jolsa@kernel.org haoluo@google.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch warning CHECK: spaces preferred around that '*' (ctx:WxV) WARNING: line length of 91 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-25 fail Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-1 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with llvm-16

Commit Message

Yonghong Song Aug. 23, 2023, 10:56 p.m. UTC
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(-)

Comments

David Marchevsky Aug. 24, 2023, 5:31 a.m. UTC | #1
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)
>  {
Yonghong Song Aug. 24, 2023, 5:56 a.m. UTC | #2
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)
>>   {
diff mbox series

Patch

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)
 {