Re: [PATCH v4 bpf-next 10/14] bpf: Add d_path helper

[KP Singh <kpsingh@chromium.org>]

On 6/28/20 9:42 PM, Jiri Olsa wrote:
> On Fri, Jun 26, 2020 at 01:38:27PM -0700, Andrii Nakryiko wrote:
>> On Thu, Jun 25, 2020 at 4:49 PM Jiri Olsa <jolsa@kernel.org> wrote:
>>>
>>> Adding d_path helper function that returns full path
>>> for give 'struct path' object, which needs to be the
>>> kernel BTF 'path' object.
>>>
>>> The helper calls directly d_path function.
>>>
>>> Updating also bpf.h tools uapi header and adding
>>> 'path' to bpf_helpers_doc.py script.
>>>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>> ---
>>>  include/uapi/linux/bpf.h       | 14 +++++++++-
>>>  kernel/trace/bpf_trace.c       | 47 ++++++++++++++++++++++++++++++++++
>>>  scripts/bpf_helpers_doc.py     |  2 ++
>>>  tools/include/uapi/linux/bpf.h | 14 +++++++++-
>>>  4 files changed, 75 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 0cb8ec948816..23274c81f244 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -3285,6 +3285,17 @@ union bpf_attr {
>>>   *             Dynamically cast a *sk* pointer to a *udp6_sock* pointer.
>>>   *     Return
>>>   *             *sk* if casting is valid, or NULL otherwise.
>>> + *
>>> + * int bpf_d_path(struct path *path, char *buf, u32 sz)
>>> + *     Description
>>> + *             Return full path for given 'struct path' object, which
>>> + *             needs to be the kernel BTF 'path' object. The path is
>>> + *             returned in buffer provided 'buf' of size 'sz'.
>>> + *
>>> + *     Return
>>> + *             length of returned string on success, or a negative
>>> + *             error in case of failure
>>
>> It's important to note whether string is always zero-terminated (I'm
>> guessing it is, right?).
> 
> right, will add

Also note that bpf_probe_read_{kernel, user}_str return the length including
the NUL byte:

 * 	Return
 * 		On success, the strictly positive length of the string,
 * 		including the trailing NUL character. On error, a negative
 * 		value.

It would be good to keep this uniform. So you will need a len += 1 here as well.

- KP

> 
>>
>>> + *
>>>   */
>>>  #define __BPF_FUNC_MAPPER(FN)          \
>>>         FN(unspec),                     \
>>> @@ -3427,7 +3438,8 @@ union bpf_attr {
>>>         FN(skc_to_tcp_sock),            \
>>>         FN(skc_to_tcp_timewait_sock),   \
>>>         FN(skc_to_tcp_request_sock),    \
>>> -       FN(skc_to_udp6_sock),
>>> +       FN(skc_to_udp6_sock),           \
>>> +       FN(d_path),
>>>
>>>  /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>>   * function eBPF program intends to call
>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>> index b124d468688c..6f31e21565b6 100644
>>> --- a/kernel/trace/bpf_trace.c
>>> +++ b/kernel/trace/bpf_trace.c
>>> @@ -1060,6 +1060,51 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
>>>         .arg1_type      = ARG_ANYTHING,
>>>  };
>>>
>>> +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
>>> +{
>>> +       char *p = d_path(path, buf, sz - 1);
>>> +       int len;
>>> +
>>> +       if (IS_ERR(p)) {
>>> +               len = PTR_ERR(p);
>>> +       } else {
>>> +               len = strlen(p);
>>> +               if (len && p != buf) {
>>> +                       memmove(buf, p, len);
>>> +                       buf[len] = 0;
>>
>> if len above is zero, you won't zero-terminate it, so probably better
>> to move buf[len] = 0 out of if to do unconditionally
> 
> good catch, will change
> 
>>
>>> +               }
>>> +       }
>>> +
>>> +       return len;
>>> +}
>>> +
>>> +BTF_SET_START(btf_whitelist_d_path)
>>> +BTF_ID(func, vfs_truncate)
>>> +BTF_ID(func, vfs_fallocate)
>>> +BTF_ID(func, dentry_open)
>>> +BTF_ID(func, vfs_getattr)
>>> +BTF_ID(func, filp_close)
>>> +BTF_SET_END(btf_whitelist_d_path)
>>> +
>>> +static bool bpf_d_path_allowed(const struct bpf_prog *prog)
>>> +{
>>> +       return btf_id_set_contains(&btf_whitelist_d_path, prog->aux->attach_btf_id);
>>> +}
>>> +
>>
>> This looks pretty great and clean, considering what's happening under
>> the covers. Nice work, thanks a lot!
>>
>>> +BTF_ID_LIST(bpf_d_path_btf_ids)
>>> +BTF_ID(struct, path)
>>
>> this is a bit more confusing to read and error-prone, but I couldn't
>> come up with any better way to do this... Still better than
>> alternatives.
>>
>>> +
>>> +static const struct bpf_func_proto bpf_d_path_proto = {
>>> +       .func           = bpf_d_path,
>>> +       .gpl_only       = true,
>>
>> Does it have to be GPL-only? What's the criteria? Sorry if this was
>> brought up previously.
> 
> I don't think it's needed to be gpl_only, I'll set it to false
> 
> thanks,
> jirka
>