Re: [PATCH v8 bpf-next 09/13] bpf: Add d_path helper

On Tue, Jul 28, 2020 at 12:47:03PM -0700, Andrii Nakryiko wrote:

On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa [off-list ref] wrote:

quoted

Adding d_path helper function that returns 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' and is zero terminated.

  bpf_d_path(&file->f_path, buf, size);

The helper calls directly d_path function, so there's only
limited set of function it can be called from. Adding just
very modest set for the start.

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       | 13 +++++++++
 kernel/trace/bpf_trace.c       | 48 ++++++++++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  2 ++
 tools/include/uapi/linux/bpf.h | 13 +++++++++
 4 files changed, 76 insertions(+)

[...]

quoted

+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);

not sure if it's worth it, but if len == sz - 1 then memmove is not
necessary. Again, don't know if worth it, as it's probably not going
to be a common case.

I did not see condition like that for d_path/file_path usage,
I'll check if such return values are even possible

quoted

+               buf[len] = 0;
+               /* Include the trailing NUL. */
+               len++;
+       }
+
+       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)

We should probably comply with an updated coding style ([0]) and use
an allowlist name for this?

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=49decddd39e5f6132ccd7d9fdc3d7c470b0061bb

right, will change

quoted

+
+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);
+}
+
+BTF_ID_LIST(bpf_d_path_btf_ids)
+BTF_ID(struct, path)
+
+static const struct bpf_func_proto bpf_d_path_proto = {
+       .func           = bpf_d_path,
+       .gpl_only       = false,
+       .ret_type       = RET_INTEGER,
+       .arg1_type      = ARG_PTR_TO_BTF_ID,
+       .arg2_type      = ARG_PTR_TO_MEM,
+       .arg3_type      = ARG_CONST_SIZE,

I feel like we had a discussion about ARG_CONST_SIZE vs
ARG_CONST_SIZE_OR_ZERO before, maybe on some different thread.
Basically, this >0 restriction was a major nuisance for
bpf_perf_event_output() cases, so much that we changed it to _OR_ZERO.
In practice, while it might never be the case that we have sz == 0
passed into the function, having to prove this to the verifier is a
PITA. Unless there is a very strong reason not to, let's mark this as
ARG_CONST_SIZE_OR_ZERO and handle sz == 0 case as a noop?

sure, will change

thanks,
jirka