Re: [PATCH v3 bpf-next 5/5] selftests/bpf: Tweak cgroup kfunc test.

[David Vernet <void@manifault.com>]

On Tue, Feb 28, 2023 at 04:29:18PM -0800, Alexei Starovoitov wrote:
> On Tue, Feb 28, 2023 at 9:07 AM David Vernet <void@manifault.com> wrote:
> > libbpf: prog 'on_lookup': failed to load: -13
> > libbpf: failed to load object 'cgrp_ls_recursion'
> > libbpf: failed to load BPF skeleton 'cgrp_ls_recursion': -13
> > test_recursion:FAIL:skel_open_and_load unexpected error: -13
> > #43/3    cgrp_local_storage/recursion:FAIL
> > #43      cgrp_local_storage:FAIL
> >
> > All error logs:
> > test_cgrp_local_storage:PASS:join_cgroup /cgrp_local_storage 0 nsec
> > libbpf: prog 'on_lookup': BPF program load failed: Permission denied
> > libbpf: prog 'on_lookup': -- BEGIN PROG LOAD LOG --
> > reg type unsupported for arg#0 function on_lookup#16
> > 0: R1=ctx(off=0,imm=0) R10=fp0
> > ; struct task_struct *task = bpf_get_current_task_btf();
> > 0: (85) call bpf_get_current_task_btf#158     ; R0_w=trusted_ptr_task_struct(off=0,imm=0)
> > 1: (bf) r6 = r0                       ; R0_w=trusted_ptr_task_struct(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
> > ; bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
> > 2: (79) r1 = *(u64 *)(r6 +2296)       ; R1_w=rcu_ptr_or_null_css_set(off=0,imm=0) R6_w=trusted_ptr_task_struct(off=0,imm=0)
> > ; bpf_cgrp_storage_delete(&map_a, task->cgroups->dfl_cgrp);
> > 3: (79) r2 = *(u64 *)(r1 +120)
> > R1 invalid mem access 'rcu_ptr_or_null_'
> 
> This one was tricky :)
> Turned out btf_nested_type_is_trusted() was able to find
> 'cgroups' field in gcc compiled kernel and was failing on clang
> compiled kernel because patch 2 did:
> BTF_TYPE_SAFE_NESTED(struct task_struct) {
>         const cpumask_t *cpus_ptr;
>         struct css_set *cgroups;
> };
> instead of
> BTF_TYPE_SAFE_NESTED(struct task_struct) {
>         const cpumask_t *cpus_ptr;
>         struct css_set __rcu *cgroups;
> };
> The missing tag was causing a miscompare.

Ahh, sorry I missed that in review. Once your patch set lands I'll add a
very loud comment here so that it's not missed in the future.

> Something to keep in mind.
> This ugliness will go away once GCC supports btf tag.

Looking forward to that day.

Given that you'll apply that fix to [0] here's my stamp for this patch:

Acked-by: David Vernet <void@manifault.com>

[0]: https://lore.kernel.org/all/20230228040121.94253-3-alexei.starovoitov@gmail.com/