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

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

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

>

On Fri, Jul 17, 2020 at 01:13:23AM +0200, KP Singh wrote:
> 

> 

> 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.


good point, will keep it that way

thanks,
jirka