Re: [PATCH bpf-next] bpftool: Fix missing pids during link show

[Quentin Monnet <quentin@isovalent.com>]

2024-03-11 23:30 UTC+0000 ~ Yonghong Song <yonghong.song@linux.dev>
> 
> On 3/11/24 4:13 PM, Andrii Nakryiko wrote:
>> On Mon, Mar 11, 2024 at 2:41 PM Yonghong Song
>> <yonghong.song@linux.dev> wrote:
>>> Current 'bpftool link' command does not show pids, e.g.,
>>>    $ tools/build/bpftool/bpftool link
>>>    ...
>>>    4: tracing  prog 23
>>>          prog_type lsm  attach_type lsm_mac
>>>          target_obj_id 1  target_btf_id 31320
>>>
>>> Hack the following change to enable normal libbpf debug output,
>>>    --- a/tools/bpf/bpftool/pids.c
>>>    +++ b/tools/bpf/bpftool/pids.c
>>>    @@ -121,9 +121,9 @@ int build_obj_refs_table(struct hashmap **map,
>>> enum bpf_obj_type type)
>>>            /* we don't want output polluted with libbpf errors if
>>> bpf_iter is not
>>>             * supported
>>>             */
>>>    -       default_print = libbpf_set_print(libbpf_print_none);
>>>    +       /* default_print = libbpf_set_print(libbpf_print_none); */
>>>            err = pid_iter_bpf__load(skel);
>>>    -       libbpf_set_print(default_print);
>>>    +       /* libbpf_set_print(default_print); */

I'm taking note to make these logs available when users pass the --debug
flag (https://github.com/libbpf/bpftool/issues/137), there's no reason
to hide them in that case.

>>>
>>> Rerun the above bpftool command:
>>>    $ tools/build/bpftool/bpftool link
>>>    libbpf: prog 'iter': BPF program load failed: Permission denied
>>>    libbpf: prog 'iter': -- BEGIN PROG LOAD LOG --
>>>    0: R1=ctx() R10=fp0
>>>    ; struct task_struct *task = ctx->task; @ pid_iter.bpf.c:69
>>>    0: (79) r6 = *(u64 *)(r1 +8)          ; R1=ctx()
>>> R6_w=ptr_or_null_task_struct(id=1)
>>>    ; struct file *file = ctx->file; @ pid_iter.bpf.c:68
>>>    ...
>>>    ; struct bpf_link *link = (struct bpf_link *) file->private_data;
>>> @ pid_iter.bpf.c:103
>>>    80: (79) r3 = *(u64 *)(r8 +432)       ; R3_w=scalar() R8=ptr_file()
>>>    ; if (link->type == bpf_core_enum_value(enum
>>> bpf_link_type___local, @ pid_iter.bpf.c:105
>>>    81: (61) r1 = *(u32 *)(r3 +12)
>>>    R3 invalid mem access 'scalar'
>>>    processed 39 insns (limit 1000000) max_states_per_insn 0
>>> total_states 3 peak_states 3 mark_read 2
>>>    -- END PROG LOAD LOG --
>>>    libbpf: prog 'iter': failed to load: -13
>>>    ...
>>>
>>> The 'file->private_data' returns a 'void' type and this caused
>>> subsequent 'link->type'
>>> (insn #81) failed in verification.
>>>
>>> To fix the issue, do a type cast from 'void *' to 'struct bpf_link *'
>>> for file->private_data
>>> as in this patch, and the 'bpftool link' runs successfully with 'pids'.
>>>    $ tools/build/bpftool/bpftool link
>>>    ...
>>>    4: tracing  prog 23
>>>          prog_type lsm  attach_type lsm_mac
>>>          target_obj_id 1  target_btf_id 31320
>>>          pids systemd(1)
>>>
>>> Fixes: 44ba7b30e84f ("bpftool: Use a local copy of
>>> BPF_LINK_TYPE_PERF_EVENT in pid_iter.bpf.c")
>>> Cc: Quentin Monnet <quentin@isovalent.com>
>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>> ---
>>>   tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
>>> b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
>>> index 26004f0c5a6a..96ffcc4f0e67 100644
>>> --- a/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
>>> +++ b/tools/bpf/bpftool/skeleton/pid_iter.bpf.c
>>> @@ -100,7 +100,7 @@ int iter(struct bpf_iter__task_file *ctx)
>>>          if (obj_type == BPF_OBJ_LINK &&
>>>              bpf_core_enum_value_exists(enum bpf_link_type___local,
>>>                                        
>>> BPF_LINK_TYPE_PERF_EVENT___local)) {
>>> -               struct bpf_link *link = (struct bpf_link *)
>>> file->private_data;
>>> +               struct bpf_link *link =
>>> bpf_core_cast(file->private_data, struct bpf_link);
>> bpf_core_cast() (i.e., bpf_rdonly_cast()) is newer feature, so relying
>> on it in bpftool will make PID printing not work on older kernels that
>> generally do support iter/task_file iterators.
>>
>>>                  if (link->type == bpf_core_enum_value(enum
>>> bpf_link_type___local,
>> the problem is that we dropped BPF_CORE_READ() here and went with
>> `link->type`. Should we restore BPF_CORE_READ(link, type) here and
>> solve the problem, while also supporting this feature on wider set of
>> kernels? Quentin?
> 
> Good point. Using the latest fancy feature might not be the best way as old
> kernels won't work and this is a strong argument. I will change
> to use BPF_CORE_READ in v2.

Agreed, the whole point of commit 44ba7b30e84f that introduced the issue
was to make bpftool work with older kernels. BPF_CORE_READ sounds like
the right approach, thank you!