| Message ID | 20210712162424.2034006-1-hengqi.chen@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 10 maintainers not CCed: netdev@vger.kernel.org kpsingh@kernel.org mingo@redhat.com andrii@kernel.org daniel@iogearbox.net kafai@fb.com rostedt@goodmis.org ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6 this patch: 6 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6 this patch: 6 |
| netdev/header_inline | success | Link |
Hengqi Chen wrote: > Add vfs_read and vfs_write to bpf_d_path allowlist. > This will help tools like IOVisor's filetop to get > full file path. > > Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > --- As I understand it dpath helper is allowed as long as we are not in NMI/interrupt context, so these should be fine to add. Acked-by: John Fastabend <john.fastabend@gmail.com> > kernel/trace/bpf_trace.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 64bd2d84367f..6d3f951f38c5 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate) > BTF_ID(func, dentry_open) > BTF_ID(func, vfs_getattr) > BTF_ID(func, filp_close) > +BTF_ID(func, vfs_read) > +BTF_ID(func, vfs_write) > BTF_SET_END(btf_allowlist_d_path) > > static bool bpf_d_path_allowed(const struct bpf_prog *prog) > -- > 2.25.1 >
On 7/12/21 12:12 PM, John Fastabend wrote: > Hengqi Chen wrote: >> Add vfs_read and vfs_write to bpf_d_path allowlist. >> This will help tools like IOVisor's filetop to get >> full file path. >> >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >> --- > > As I understand it dpath helper is allowed as long as we > are not in NMI/interrupt context, so these should be fine > to add. > > Acked-by: John Fastabend <john.fastabend@gmail.com> The corresponding bcc discussion thread is here: https://github.com/iovisor/bcc/issues/3527 Acked-by: Yonghong Song <yhs@fb.com> > >> kernel/trace/bpf_trace.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >> index 64bd2d84367f..6d3f951f38c5 100644 >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate) >> BTF_ID(func, dentry_open) >> BTF_ID(func, vfs_getattr) >> BTF_ID(func, filp_close) >> +BTF_ID(func, vfs_read) >> +BTF_ID(func, vfs_write) >> BTF_SET_END(btf_allowlist_d_path) >> >> static bool bpf_d_path_allowed(const struct bpf_prog *prog) >> -- >> 2.25.1 >> > >
On Wed, Jul 14, 2021 at 5:55 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 7/12/21 12:12 PM, John Fastabend wrote: > > Hengqi Chen wrote: > >> Add vfs_read and vfs_write to bpf_d_path allowlist. > >> This will help tools like IOVisor's filetop to get > >> full file path. > >> > >> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> > >> --- > > > > As I understand it dpath helper is allowed as long as we > > are not in NMI/interrupt context, so these should be fine > > to add. > > > > Acked-by: John Fastabend <john.fastabend@gmail.com> > > The corresponding bcc discussion thread is here: > https://github.com/iovisor/bcc/issues/3527 > > Acked-by: Yonghong Song <yhs@fb.com> > > > > >> kernel/trace/bpf_trace.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > >> index 64bd2d84367f..6d3f951f38c5 100644 > >> --- a/kernel/trace/bpf_trace.c > >> +++ b/kernel/trace/bpf_trace.c > >> @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate) > >> BTF_ID(func, dentry_open) > >> BTF_ID(func, vfs_getattr) > >> BTF_ID(func, filp_close) > >> +BTF_ID(func, vfs_read) > >> +BTF_ID(func, vfs_write) > >> BTF_SET_END(btf_allowlist_d_path) That feels incomplete. I know we can add more later, but why these two and not vfs_readv ? security_file_permission should probably be added as well ? Along with all sys_* entry points ?
On 7/15/21 6:44 PM, Alexei Starovoitov wrote: > On Wed, Jul 14, 2021 at 5:55 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 7/12/21 12:12 PM, John Fastabend wrote: >>> Hengqi Chen wrote: >>>> Add vfs_read and vfs_write to bpf_d_path allowlist. >>>> This will help tools like IOVisor's filetop to get >>>> full file path. >>>> >>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>>> --- >>> >>> As I understand it dpath helper is allowed as long as we >>> are not in NMI/interrupt context, so these should be fine >>> to add. >>> >>> Acked-by: John Fastabend <john.fastabend@gmail.com> >> >> The corresponding bcc discussion thread is here: >> https://github.com/iovisor/bcc/issues/3527 >> >> Acked-by: Yonghong Song <yhs@fb.com> >> >>> >>>> kernel/trace/bpf_trace.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>>> index 64bd2d84367f..6d3f951f38c5 100644 >>>> --- a/kernel/trace/bpf_trace.c >>>> +++ b/kernel/trace/bpf_trace.c >>>> @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate) >>>> BTF_ID(func, dentry_open) >>>> BTF_ID(func, vfs_getattr) >>>> BTF_ID(func, filp_close) >>>> +BTF_ID(func, vfs_read) >>>> +BTF_ID(func, vfs_write) >>>> BTF_SET_END(btf_allowlist_d_path) > > That feels incomplete. > I know we can add more later, but why these two and not vfs_readv ? > security_file_permission should probably be added as well ? > Along with all sys_* entry points ? The first argument of bpf_d_path is "struct path *, path" which needs to be a BTF_ID w.r.t. verifier. I think maybe we should target the common kernel functions which has "struct path *" or "struct file *" arguments? vfs_readv and security_file_permission or other possible candidates are not added since there are no use case for those yet. But agree that adding some vfs_* calls and security_file* options are a good call since they could be used in a different situation and adding them may save another kernel patch. The syscall entry points typically only contains fd. Although bpf program might hack to do something to convert fd to a file, I still think this is a unlikely use case.
On 7/18/21 12:43 AM, Yonghong Song wrote: > > > On 7/15/21 6:44 PM, Alexei Starovoitov wrote: >> On Wed, Jul 14, 2021 at 5:55 PM Yonghong Song <yhs@fb.com> wrote: >>> >>> >>> >>> On 7/12/21 12:12 PM, John Fastabend wrote: >>>> Hengqi Chen wrote: >>>>> Add vfs_read and vfs_write to bpf_d_path allowlist. >>>>> This will help tools like IOVisor's filetop to get >>>>> full file path. >>>>> >>>>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> >>>>> --- >>>> >>>> As I understand it dpath helper is allowed as long as we >>>> are not in NMI/interrupt context, so these should be fine >>>> to add. >>>> >>>> Acked-by: John Fastabend <john.fastabend@gmail.com> >>> >>> The corresponding bcc discussion thread is here: >>> https://github.com/iovisor/bcc/issues/3527 >>> >>> Acked-by: Yonghong Song <yhs@fb.com> >>> >>>> >>>>> kernel/trace/bpf_trace.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c >>>>> index 64bd2d84367f..6d3f951f38c5 100644 >>>>> --- a/kernel/trace/bpf_trace.c >>>>> +++ b/kernel/trace/bpf_trace.c >>>>> @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate) >>>>> BTF_ID(func, dentry_open) >>>>> BTF_ID(func, vfs_getattr) >>>>> BTF_ID(func, filp_close) >>>>> +BTF_ID(func, vfs_read) >>>>> +BTF_ID(func, vfs_write) >>>>> BTF_SET_END(btf_allowlist_d_path) >> >> That feels incomplete. >> I know we can add more later, but why these two and not vfs_readv ? >> security_file_permission should probably be added as well ? >> Along with all sys_* entry points ? > > The first argument of bpf_d_path is "struct path *, path" > which needs to be a BTF_ID w.r.t. verifier. > > I think maybe we should target the common kernel functions which > has "struct path *" or "struct file *" arguments? > > vfs_readv and security_file_permission or other possible candidates > are not added since there are no use case for those yet. But agree > that adding some vfs_* calls and security_file* options are a good > call since they could be used in a different situation and adding > them may save another kernel patch. > > The syscall entry points typically only contains fd. Although > bpf program might hack to do something to convert fd to a file, > I still think this is a unlikely use case. Thanks for the review and suggestions. I will send a v2 for review. Thanks, Hengqi
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 64bd2d84367f..6d3f951f38c5 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -861,6 +861,8 @@ BTF_ID(func, vfs_fallocate) BTF_ID(func, dentry_open) BTF_ID(func, vfs_getattr) BTF_ID(func, filp_close) +BTF_ID(func, vfs_read) +BTF_ID(func, vfs_write) BTF_SET_END(btf_allowlist_d_path) static bool bpf_d_path_allowed(const struct bpf_prog *prog)
Add vfs_read and vfs_write to bpf_d_path allowlist. This will help tools like IOVisor's filetop to get full file path. Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com> --- kernel/trace/bpf_trace.c | 2 ++ 1 file changed, 2 insertions(+)