Re: [PATCH 09/11] bpf: Add d_path helper

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jiri Olsa <jolsa@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
	Martin KaFai Lau <kafai@fb.com>, David Miller <davem@redhat.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Wenbo Zhang <ethercflow@gmail.com>,
	KP Singh <kpsingh@chromium.org>, Andrii Nakryiko <andriin@fb.com>,
	Brendan Gregg <bgregg@netflix.com>,
	Florent Revest <revest@chromium.org>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH 09/11] bpf: Add d_path helper
Date: Thu, 18 Jun 2020 21:35:10 -0700	[thread overview]
Message-ID: <CAEf4BzY=d5y_-fXvomG7SjkbK7DZn5=-f+sdCYRdZh9qeynQrQ@mail.gmail.com> (raw)
In-Reply-To: <20200616100512.2168860-10-jolsa@kernel.org>

On Tue, Jun 16, 2020 at 3:07 AM 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/linux/bpf.h            |  4 ++++
>  include/uapi/linux/bpf.h       | 14 ++++++++++++-
>  kernel/bpf/btf_ids.c           | 11 ++++++++++
>  kernel/trace/bpf_trace.c       | 38 ++++++++++++++++++++++++++++++++++
>  scripts/bpf_helpers_doc.py     |  2 ++
>  tools/include/uapi/linux/bpf.h | 14 ++++++++++++-
>  6 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a94e85c2ec50..d35265b6c574 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1752,5 +1752,9 @@ extern int bpf_skb_output_btf_ids[];
>  extern int bpf_seq_printf_btf_ids[];
>  extern int bpf_seq_write_btf_ids[];
>  extern int bpf_xdp_output_btf_ids[];
> +extern int bpf_d_path_btf_ids[];
> +
> +extern int btf_whitelist_d_path[];
> +extern int btf_whitelist_d_path_cnt;

So with suggestion from previous patch, this would be declared as:

extern const struct btf_id_set btf_whitelist_d_path;

>
>  #endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index c65b374a5090..e308746b9344 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3252,6 +3252,17 @@ union bpf_attr {
>   *             case of **BPF_CSUM_LEVEL_QUERY**, the current skb->csum_level
>   *             is returned or the error code -EACCES in case the skb is not
>   *             subject to CHECKSUM_UNNECESSARY.
> + *
> + * 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
> + *
>   */
>  #define __BPF_FUNC_MAPPER(FN)          \
>         FN(unspec),                     \
> @@ -3389,7 +3400,8 @@ union bpf_attr {
>         FN(ringbuf_submit),             \
>         FN(ringbuf_discard),            \
>         FN(ringbuf_query),              \
> -       FN(csum_level),
> +       FN(csum_level),                 \
> +       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/bpf/btf_ids.c b/kernel/bpf/btf_ids.c
> index d8d0df162f04..853c8fd59b06 100644
> --- a/kernel/bpf/btf_ids.c
> +++ b/kernel/bpf/btf_ids.c
> @@ -13,3 +13,14 @@ BTF_ID(struct, seq_file)
>
>  BTF_ID_LIST(bpf_xdp_output_btf_ids)
>  BTF_ID(struct, xdp_buff)
> +
> +BTF_ID_LIST(bpf_d_path_btf_ids)
> +BTF_ID(struct, path)
> +
> +BTF_WHITELIST_ENTRY(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_WHITELIST_END(btf_whitelist_d_path)

Oh, so that's why you added btf_ids.c. Do you think centralizing all
those BTF ID lists in one file is going to be more convenient? I lean
towards keeping them closer to where they are used, as it was with all
those helper BTF IDS. But I wonder what others think...

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index c1866d76041f..0ff5d8434d40 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1016,6 +1016,42 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
>         .arg1_type      = ARG_ANYTHING,
>  };
>

[...]