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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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
>
