| Message ID | 20200616100512.2168860-10-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
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, > }; > [...]
On Thu, Jun 18, 2020 at 09:35:10PM -0700, Andrii Nakryiko wrote: > 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; yes SNIP > > /* 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... either way works for me, but then BTF_ID_* macros needs to go to include/linux/btf_ids.h header right? jirka > > > 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, > > }; > > > > [...] >
On Fri, Jun 19, 2020 at 6:31 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Thu, Jun 18, 2020 at 09:35:10PM -0700, Andrii Nakryiko wrote: > > 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; > > yes > > SNIP > > > > /* 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... > > either way works for me, but then BTF_ID_* macros needs to go > to include/linux/btf_ids.h header right? > given it's internal API, I'd probably just put it in include/linux/btf.h or include/linux/bpf.h, don't think we need extra header just for these > jirka > > > > > > 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, > > > }; > > > > > > > [...] > > >
On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote: SNIP > > > > /* 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... > > > > either way works for me, but then BTF_ID_* macros needs to go > > to include/linux/btf_ids.h header right? > > > > given it's internal API, I'd probably just put it in > include/linux/btf.h or include/linux/bpf.h, don't think we need extra > header just for these actually, I might end up with extra header, so it's possible to add selftest for this jirka
On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote: > > SNIP > > > > > > /* 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... > > > > > > either way works for me, but then BTF_ID_* macros needs to go > > > to include/linux/btf_ids.h header right? > > > > > > > given it's internal API, I'd probably just put it in > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra > > header just for these > > actually, I might end up with extra header, so it's possible > to add selftest for this > How does extra header help with selftest? > jirka >
On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote: > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote: > > > > SNIP > > > > > > > > /* 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... > > > > > > > > either way works for me, but then BTF_ID_* macros needs to go > > > > to include/linux/btf_ids.h header right? > > > > > > > > > > given it's internal API, I'd probably just put it in > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra > > > header just for these > > > > actually, I might end up with extra header, so it's possible > > to add selftest for this > > > > How does extra header help with selftest? to create binary with various lists defined like we do in kernel using the same macros.. and check they are properly made/sorted jirka
On Tue, Jun 23, 2020 at 3:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote: > > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote: > > > > > > SNIP > > > > > > > > > > /* 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... > > > > > > > > > > either way works for me, but then BTF_ID_* macros needs to go > > > > > to include/linux/btf_ids.h header right? > > > > > > > > > > > > > given it's internal API, I'd probably just put it in > > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra > > > > header just for these > > > > > > actually, I might end up with extra header, so it's possible > > > to add selftest for this > > > > > > > How does extra header help with selftest? > > to create binary with various lists defined like we do in kernel > using the same macros.. and check they are properly made/sorted > So the problem here is that selftests don't have access to internal (non-UAPI) linux/bpf.h header, right? Ok, that's a fair point. > jirka >
On Tue, Jun 23, 2020 at 11:58:33AM -0700, Andrii Nakryiko wrote: > On Tue, Jun 23, 2020 at 3:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote: > > > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote: > > > > > > > > SNIP > > > > > > > > > > > > /* 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... > > > > > > > > > > > > either way works for me, but then BTF_ID_* macros needs to go > > > > > > to include/linux/btf_ids.h header right? > > > > > > > > > > > > > > > > given it's internal API, I'd probably just put it in > > > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra > > > > > header just for these > > > > > > > > actually, I might end up with extra header, so it's possible > > > > to add selftest for this > > > > > > > > > > How does extra header help with selftest? > > > > to create binary with various lists defined like we do in kernel > > using the same macros.. and check they are properly made/sorted > > > > So the problem here is that selftests don't have access to internal > (non-UAPI) linux/bpf.h header, right? Ok, that's a fair point. hm, how about we keep tools/include/linux/btf_ids.h copy like we do for other kernel headers jirka
On Tue, Jun 23, 2020 at 1:14 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Tue, Jun 23, 2020 at 11:58:33AM -0700, Andrii Nakryiko wrote: > > On Tue, Jun 23, 2020 at 3:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > On Mon, Jun 22, 2020 at 12:18:19PM -0700, Andrii Nakryiko wrote: > > > > On Mon, Jun 22, 2020 at 2:02 AM Jiri Olsa <jolsa@redhat.com> wrote: > > > > > > > > > > On Fri, Jun 19, 2020 at 11:25:27AM -0700, Andrii Nakryiko wrote: > > > > > > > > > > SNIP > > > > > > > > > > > > > > /* 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... > > > > > > > > > > > > > > either way works for me, but then BTF_ID_* macros needs to go > > > > > > > to include/linux/btf_ids.h header right? > > > > > > > > > > > > > > > > > > > given it's internal API, I'd probably just put it in > > > > > > include/linux/btf.h or include/linux/bpf.h, don't think we need extra > > > > > > header just for these > > > > > > > > > > actually, I might end up with extra header, so it's possible > > > > > to add selftest for this > > > > > > > > > > > > > How does extra header help with selftest? > > > > > > to create binary with various lists defined like we do in kernel > > > using the same macros.. and check they are properly made/sorted > > > > > > > So the problem here is that selftests don't have access to internal > > (non-UAPI) linux/bpf.h header, right? Ok, that's a fair point. > > hm, how about we keep tools/include/linux/btf_ids.h copy > like we do for other kernel headers yes, I assumed you are going to do that > > jirka >
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; #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) 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, }; +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; + } + } + + return len; +} + +static bool bpf_d_path_allowed(const struct bpf_prog *prog) +{ + return btf_whitelist_search(prog->aux->attach_btf_id, + btf_whitelist_d_path, + btf_whitelist_d_path_cnt); +} + +static const struct bpf_func_proto bpf_d_path_proto = { + .func = bpf_d_path, + .gpl_only = true, + .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) { @@ -1483,6 +1519,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 91fa668fa860..3161bf4ccee4 100755 --- a/scripts/bpf_helpers_doc.py +++ b/scripts/bpf_helpers_doc.py @@ -425,6 +425,7 @@ class PrinterHelpers(Printer): 'struct __sk_buff', 'struct sk_msg_md', 'struct xdp_md', + 'struct path', ] known_types = { '...', @@ -458,6 +459,7 @@ class PrinterHelpers(Printer): 'struct sockaddr', 'struct tcphdr', 'struct seq_file', + 'struct path', } mapped_types = { 'u8': '__u8', diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index c65b374a5090..e308746b9344 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/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
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(-)