* [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write
@ 2021-07-12 16:24 Hengqi Chen
2021-07-12 19:12 ` John Fastabend
0 siblings, 1 reply; 6+ messages in thread
From: Hengqi Chen @ 2021-07-12 16:24 UTC (permalink / raw)
To: bpf; +Cc: yhs, andriin, jolsa, hengqi.chen
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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write
2021-07-12 16:24 [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write Hengqi Chen
@ 2021-07-12 19:12 ` John Fastabend
2021-07-15 0:18 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: John Fastabend @ 2021-07-12 19:12 UTC (permalink / raw)
To: Hengqi Chen, bpf; +Cc: yhs, andriin, jolsa, hengqi.chen
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write
2021-07-12 19:12 ` John Fastabend
@ 2021-07-15 0:18 ` Yonghong Song
2021-07-16 1:44 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2021-07-15 0:18 UTC (permalink / raw)
To: John Fastabend, Hengqi Chen, bpf; +Cc: andriin, jolsa
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
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write
2021-07-15 0:18 ` Yonghong Song
@ 2021-07-16 1:44 ` Alexei Starovoitov
2021-07-17 16:43 ` Yonghong Song
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2021-07-16 1:44 UTC (permalink / raw)
To: Yonghong Song
Cc: John Fastabend, Hengqi Chen, bpf, Andrii Nakryiko, Jiri Olsa
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 ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write
2021-07-16 1:44 ` Alexei Starovoitov
@ 2021-07-17 16:43 ` Yonghong Song
2021-07-18 11:17 ` Hengqi Chen
0 siblings, 1 reply; 6+ messages in thread
From: Yonghong Song @ 2021-07-17 16:43 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: John Fastabend, Hengqi Chen, bpf, Andrii Nakryiko, Jiri Olsa
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write
2021-07-17 16:43 ` Yonghong Song
@ 2021-07-18 11:17 ` Hengqi Chen
0 siblings, 0 replies; 6+ messages in thread
From: Hengqi Chen @ 2021-07-18 11:17 UTC (permalink / raw)
To: Yonghong Song, Alexei Starovoitov
Cc: John Fastabend, bpf, Andrii Nakryiko, Jiri Olsa
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-18 11:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 16:24 [PATCH bpf-next] bpf: Expose bpf_d_path helper to vfs_read/vfs_write Hengqi Chen
2021-07-12 19:12 ` John Fastabend
2021-07-15 0:18 ` Yonghong Song
2021-07-16 1:44 ` Alexei Starovoitov
2021-07-17 16:43 ` Yonghong Song
2021-07-18 11:17 ` Hengqi Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.