Re: [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write

[Yonghong Song <yhs@fb.com>]

On 7/15/21 6:44 PM, Alexei Starovoitov wrote:
> On Wed, Jul 14, 2021 at 5:55 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 7/12/21 12:12 PM, John Fastabend wrote:
>>> Hengqi Chen wrote:
>>>> Add vfs_read and vfs_write to bpf_d_path allowlist.
>>>> This will help tools like IOVisor's filetop to get
>>>> full file path.
>>>>
>>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>>>> ---
>>>
>>> As I understand it dpath helper is allowed as long as we
>>> are not in NMI/interrupt context, so these should be fine
>>> to add.
>>>
>>> Acked-by: John Fastabend <john.fastabend@gmail.com>
>>
>> The corresponding bcc discussion thread is here:
>>     https://github.com/iovisor/bcc/issues/3527
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>>
>>>
>>>>    kernel/trace/bpf_trace.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>>> index 64bd2d84367f..6d3f951f38c5 100644
>>>> --- a/kernel/trace/bpf_trace.c
>>>> +++ b/kernel/trace/bpf_trace.c
>>>> @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate)
>>>>    BTF_ID(func, dentry_open)
>>>>    BTF_ID(func, vfs_getattr)
>>>>    BTF_ID(func, filp_close)
>>>> +BTF_ID(func, vfs_read)
>>>> +BTF_ID(func, vfs_write)
>>>>    BTF_SET_END(btf_allowlist_d_path)
> 
> That feels incomplete.
> I know we can add more later, but why these two and not vfs_readv ?
> security_file_permission should probably be added as well ?
> Along with all sys_* entry points ?

The first argument of bpf_d_path is "struct path *, path"
which needs to be a BTF_ID w.r.t. verifier.

I think maybe we should target the common kernel functions which
has "struct path *" or "struct file *" arguments?

vfs_readv and security_file_permission or other possible candidates
are not added since there are no use case for those yet. But agree
that adding some vfs_* calls and security_file* options are a good
call since they could be used in a different situation and adding
them may save another kernel patch.

The syscall entry points typically only contains fd. Although
bpf program might hack to do something to convert fd to a file,
I still think this is a unlikely use case.