| Message ID | 20200722211223.1055107-10-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa <jolsa@kernel.org> wrote: > > 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(+) > [...] > > +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. > + 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 > + > +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? > + .btf_id = bpf_d_path_btf_ids, > + .allowed = bpf_d_path_allowed, > +}; > + [...]
On Tue, Jul 28, 2020 at 12:47:03PM -0700, Andrii Nakryiko wrote: > On Wed, Jul 22, 2020 at 2:14 PM Jiri Olsa <jolsa@kernel.org> wrote: > > > > 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(+) > > > > [...] > > > > > +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 > > > + 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 > > > + > > +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
On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote: > +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); *blink* What the hell do you need that strlen() for? d_path() copies into the end of buffer (well, starts there and prepends to it); all you really need is memmove(buf, p, buf + sz - p) > + buf[len] = 0; Wait a minute... Why are you NUL-terminating it separately? You do rely upon having NUL in the damn thing (and d_path() does guarantee it there). Without that strlen() would've gone into the nasal demon country; you can't call it on non-NUL-terminated array. So you are guaranteed that p[len] will be '\0'; why bother copying the first len bytes and then separately deal with that NUL? Just memmove() the fucker and be done with that... If you are worried about stray NUL in the middle of the returned data... can't happen. Note the rename_lock use in fs/d_path.c; the names of everything involved are guaranteed to have been stable throughout the copying them into the buffer - if anything were to be renamed while we are doing that, we'd repeat the whole thing (with rename_lock taken exclusive the second time around). So make it simply if (IS_ERR(p)) return PTR_ERR(p); len = buf + sz - p; memmove(buf, p, len); return len; and be done with that. BTW, the odds of p == buf are pretty much nil - it would happen only if sz - 1 happened to be the exact length of pathname.
On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote: > +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) While we are at it, I hope you realize that the names of kernel function are subject to change at zero notice. If some script breaks since we give e.g. filp_close a something less revolting name, it's Not My Problem(tm)...
On Wed, Jul 29, 2020 at 09:17:40PM +0100, Al Viro wrote: > On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote: > > > +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) > > While we are at it, I hope you realize that the names of kernel function > are subject to change at zero notice. If some script breaks since > we give e.g. filp_close a something less revolting name, it's Not My > Problem(tm)... even now when we change function name some scripts will stop working, so I don't think we are creating new problem in here thanks, jirka
On Wed, Jul 29, 2020 at 09:11:17PM +0100, Al Viro wrote: > On Wed, Jul 22, 2020 at 11:12:19PM +0200, Jiri Olsa wrote: > > > +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); > > *blink* > What the hell do you need that strlen() for? d_path() copies into > the end of buffer (well, starts there and prepends to it); all you > really need is memmove(buf, p, buf + sz - p) I used the code from some of the other users like backing_dev_show fsg_show_file nice, looks like we could omit strlen call in perf mmap event call as well > > > > + buf[len] = 0; > > Wait a minute... Why are you NUL-terminating it separately? > You do rely upon having NUL in the damn thing (and d_path() does > guarantee it there). Without that strlen() would've gone into > the nasal demon country; you can't call it on non-NUL-terminated > array. So you are guaranteed that p[len] will be '\0'; why bother > copying the first len bytes and then separately deal with that > NUL? Just memmove() the fucker and be done with that... > > If you are worried about stray NUL in the middle of the returned > data... can't happen. Note the rename_lock use in fs/d_path.c; > the names of everything involved are guaranteed to have been > stable throughout the copying them into the buffer - if anything > were to be renamed while we are doing that, we'd repeat the whole > thing (with rename_lock taken exclusive the second time around). > > So make it simply > if (IS_ERR(p)) > return PTR_ERR(p); > len = buf + sz - p; > memmove(buf, p, len); > return len; ok, will use this > and be done with that. BTW, the odds of p == buf are pretty much > nil - it would happen only if sz - 1 happened to be the exact length > of pathname. > ok, great thanks, jirka
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 54d0c886e3ba..f8341219b5ea 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -3377,6 +3377,18 @@ union bpf_attr { * A non-negative value equal to or less than *size* on success, * or a negative error in case of failure. * + * 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' and + * is zero terminated. + * + * Return + * On success, the strictly positive length of the string, + * including the trailing NUL character. On error, a negative + * value. + * */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3521,6 +3533,7 @@ union bpf_attr { FN(skc_to_tcp_request_sock), \ FN(skc_to_udp6_sock), \ FN(get_task_stack), \ + FN(d_path), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 3cc0dcb60ca2..0488db62aaaf 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -1098,6 +1098,52 @@ 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; + /* 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) + +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, + .btf_id = bpf_d_path_btf_ids, + .allowed = bpf_d_path_allowed, +}; + const struct bpf_func_proto * bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -1579,6 +1625,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return prog->expected_attach_type == BPF_TRACE_ITER ? &bpf_seq_write_proto : NULL; + case BPF_FUNC_d_path: + return &bpf_d_path_proto; default: return raw_tp_prog_func_proto(func_id, prog); } diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py index 5bfa448b4704..08388173973f 100755 --- a/scripts/bpf_helpers_doc.py +++ b/scripts/bpf_helpers_doc.py @@ -432,6 +432,7 @@ class PrinterHelpers(Printer): 'struct __sk_buff', 'struct sk_msg_md', 'struct xdp_md', + 'struct path', ] known_types = { '...', @@ -472,6 +473,7 @@ class PrinterHelpers(Printer): 'struct tcp_request_sock', 'struct udp6_sock', 'struct task_struct', + 'struct path', } mapped_types = { 'u8': '__u8', diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 54d0c886e3ba..f8341219b5ea 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -3377,6 +3377,18 @@ union bpf_attr { * A non-negative value equal to or less than *size* on success, * or a negative error in case of failure. * + * 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' and + * is zero terminated. + * + * Return + * On success, the strictly positive length of the string, + * including the trailing NUL character. On error, a negative + * value. + * */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -3521,6 +3533,7 @@ union bpf_attr { FN(skc_to_tcp_request_sock), \ FN(skc_to_udp6_sock), \ FN(get_task_stack), \ + FN(d_path), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
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(+)