diff mbox series

[bpf-next,2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions

Message ID 20210725141814.2000828-3-hengqi.chen@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series expand bpf_d_path helper allowlist | expand

Checks

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 6 maintainers not CCed: netdev@vger.kernel.org kpsingh@kernel.org mingo@redhat.com kafai@fb.com rostedt@goodmis.org 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, 66 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 6 this patch: 6
netdev/header_inline success Link

Commit Message

Hengqi Chen July 25, 2021, 2:18 p.m. UTC 
Add vfs_* and security_* to bpf_d_path allowlist, so that we can use
bpf_d_path helper to extract full file path from these functions'
`struct path *` and `struct file *` arguments. This will help tools
like IOVisor's filetop[2]/filelife to get full file path.

Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
---
 kernel/trace/bpf_trace.c | 52 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

--
2.25.1

Comments

Yonghong Song July 26, 2021, 6:20 a.m. UTC | #1 
On 7/25/21 7:18 AM, Hengqi Chen wrote:
> Add vfs_* and security_* to bpf_d_path allowlist, so that we can use
> bpf_d_path helper to extract full file path from these functions'
> `struct path *` and `struct file *` arguments. This will help tools
> like IOVisor's filetop[2]/filelife to get full file path.

Please use bcc intead of IOVisor.
What is "[2]" in "filetop[2]"?

> 
> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>

LGTM with minor comments below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   kernel/trace/bpf_trace.c | 52 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index c5e0b6a64091..355777b5bf63 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -850,16 +850,64 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
>   BTF_SET_START(btf_allowlist_d_path)
>   #ifdef CONFIG_SECURITY
>   BTF_ID(func, security_file_permission)
> -BTF_ID(func, security_inode_getattr)
>   BTF_ID(func, security_file_open)
> +BTF_ID(func, security_file_ioctl)
> +BTF_ID(func, security_file_free)
> +BTF_ID(func, security_file_alloc)
> +BTF_ID(func, security_file_lock)
> +BTF_ID(func, security_file_fcntl)
> +BTF_ID(func, security_file_set_fowner)
> +BTF_ID(func, security_file_receive)
> +BTF_ID(func, security_inode_getattr)
> +BTF_ID(func, security_sb_mount)
> +BTF_ID(func, security_bprm_check)

Here and also below "segments" (security_path_* functions, and
later vfs_*/dentry_open/filp_close functions),
maybe you can list functions with increasing alphabet order.
This will make it easy to check whether a particular function
exists or not and whether we miss anything.

There are more security_bprm_* functions, e.g.,
security_bprm_creds_from_file, security_bprm_committing_creds
and security_bprm_committed_creds.
These functions all have "struct linux_binprm *bprm"
parameters. Maybe we can add these few functions as well
in this round.

>   #endif
>   #ifdef CONFIG_SECURITY_PATH
>   BTF_ID(func, security_path_truncate)
> +BTF_ID(func, security_path_notify)
> +BTF_ID(func, security_path_unlink)
> +BTF_ID(func, security_path_mkdir)
> +BTF_ID(func, security_path_rmdir)
> +BTF_ID(func, security_path_mknod)
> +BTF_ID(func, security_path_symlink)
> +BTF_ID(func, security_path_link)
> +BTF_ID(func, security_path_rename)
> +BTF_ID(func, security_path_chmod)
> +BTF_ID(func, security_path_chown)
> +BTF_ID(func, security_path_chroot)
>   #endif
>   BTF_ID(func, vfs_truncate)
>   BTF_ID(func, vfs_fallocate)
> -BTF_ID(func, dentry_open)
>   BTF_ID(func, vfs_getattr)
> +BTF_ID(func, vfs_fadvise)
> +BTF_ID(func, vfs_fchmod)
> +BTF_ID(func, vfs_fchown)
> +BTF_ID(func, vfs_open)
> +BTF_ID(func, vfs_setpos)
> +BTF_ID(func, vfs_llseek)
> +BTF_ID(func, vfs_read)
> +BTF_ID(func, vfs_write)
> +BTF_ID(func, vfs_iocb_iter_read)
> +BTF_ID(func, vfs_iter_read)
> +BTF_ID(func, vfs_readv)
> +BTF_ID(func, vfs_iocb_iter_write)
> +BTF_ID(func, vfs_iter_write)
> +BTF_ID(func, vfs_writev)
> +BTF_ID(func, vfs_copy_file_range)
> +BTF_ID(func, vfs_getattr_nosec)
> +BTF_ID(func, vfs_ioctl)
> +BTF_ID(func, vfs_fsync_range)
> +BTF_ID(func, vfs_fsync)
> +BTF_ID(func, vfs_utimes)
> +BTF_ID(func, vfs_statfs)
> +BTF_ID(func, vfs_dedupe_file_range_one)
> +BTF_ID(func, vfs_dedupe_file_range)
> +BTF_ID(func, vfs_clone_file_range)
> +BTF_ID(func, vfs_cancel_lock)
> +BTF_ID(func, vfs_test_lock)
> +BTF_ID(func, vfs_setlease)
> +BTF_ID(func, vfs_lock_file)

I double checked that for the above three lock
related functions (vfs_cancel_lock, vfs_test_lock,
vfs_lock_file), I double checked d_path
does not use these locks, so we should be fine.

> +BTF_ID(func, dentry_open)
>   BTF_ID(func, filp_close)
>   BTF_SET_END(btf_allowlist_d_path)
> 
> --
> 2.25.1
>
Hengqi Chen July 26, 2021, 7:16 a.m. UTC | #2 
On 2021/7/26 2:20 PM, Yonghong Song wrote:
> 
> 
> On 7/25/21 7:18 AM, Hengqi Chen wrote:
>> Add vfs_* and security_* to bpf_d_path allowlist, so that we can use
>> bpf_d_path helper to extract full file path from these functions'
>> `struct path *` and `struct file *` arguments. This will help tools
>> like IOVisor's filetop[2]/filelife to get full file path.
> 
> Please use bcc intead of IOVisor.
> What is "[2]" in "filetop[2]"?

OK. Some links are missed in the commit messages 
and version number is not added to subject.

> 
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
> 
> LGTM with minor comments below.
> 
> Acked-by: Yonghong Song <yhs@fb.com>
> 
>> ---
>>   kernel/trace/bpf_trace.c | 52 ++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index c5e0b6a64091..355777b5bf63 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -850,16 +850,64 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
>>   BTF_SET_START(btf_allowlist_d_path)
>>   #ifdef CONFIG_SECURITY
>>   BTF_ID(func, security_file_permission)
>> -BTF_ID(func, security_inode_getattr)
>>   BTF_ID(func, security_file_open)
>> +BTF_ID(func, security_file_ioctl)
>> +BTF_ID(func, security_file_free)
>> +BTF_ID(func, security_file_alloc)
>> +BTF_ID(func, security_file_lock)
>> +BTF_ID(func, security_file_fcntl)
>> +BTF_ID(func, security_file_set_fowner)
>> +BTF_ID(func, security_file_receive)
>> +BTF_ID(func, security_inode_getattr)
>> +BTF_ID(func, security_sb_mount)
>> +BTF_ID(func, security_bprm_check)
> 
> Here and also below "segments" (security_path_* functions, and
> later vfs_*/dentry_open/filp_close functions),
> maybe you can list functions with increasing alphabet order.
> This will make it easy to check whether a particular function
> exists or not and whether we miss anything.
> 
> There are more security_bprm_* functions, e.g.,
> security_bprm_creds_from_file, security_bprm_committing_creds
> and security_bprm_committed_creds.
> These functions all have "struct linux_binprm *bprm"
> parameters. Maybe we can add these few functions as well
> in this round.
> 

Thanks. Will send a v4 for review.

>>   #endif
>>   #ifdef CONFIG_SECURITY_PATH
>>   BTF_ID(func, security_path_truncate)
>> +BTF_ID(func, security_path_notify)
>> +BTF_ID(func, security_path_unlink)
>> +BTF_ID(func, security_path_mkdir)
>> +BTF_ID(func, security_path_rmdir)
>> +BTF_ID(func, security_path_mknod)
>> +BTF_ID(func, security_path_symlink)
>> +BTF_ID(func, security_path_link)
>> +BTF_ID(func, security_path_rename)
>> +BTF_ID(func, security_path_chmod)
>> +BTF_ID(func, security_path_chown)
>> +BTF_ID(func, security_path_chroot)
>>   #endif
>>   BTF_ID(func, vfs_truncate)
>>   BTF_ID(func, vfs_fallocate)
>> -BTF_ID(func, dentry_open)
>>   BTF_ID(func, vfs_getattr)
>> +BTF_ID(func, vfs_fadvise)
>> +BTF_ID(func, vfs_fchmod)
>> +BTF_ID(func, vfs_fchown)
>> +BTF_ID(func, vfs_open)
>> +BTF_ID(func, vfs_setpos)
>> +BTF_ID(func, vfs_llseek)
>> +BTF_ID(func, vfs_read)
>> +BTF_ID(func, vfs_write)
>> +BTF_ID(func, vfs_iocb_iter_read)
>> +BTF_ID(func, vfs_iter_read)
>> +BTF_ID(func, vfs_readv)
>> +BTF_ID(func, vfs_iocb_iter_write)
>> +BTF_ID(func, vfs_iter_write)
>> +BTF_ID(func, vfs_writev)
>> +BTF_ID(func, vfs_copy_file_range)
>> +BTF_ID(func, vfs_getattr_nosec)
>> +BTF_ID(func, vfs_ioctl)
>> +BTF_ID(func, vfs_fsync_range)
>> +BTF_ID(func, vfs_fsync)
>> +BTF_ID(func, vfs_utimes)
>> +BTF_ID(func, vfs_statfs)
>> +BTF_ID(func, vfs_dedupe_file_range_one)
>> +BTF_ID(func, vfs_dedupe_file_range)
>> +BTF_ID(func, vfs_clone_file_range)
>> +BTF_ID(func, vfs_cancel_lock)
>> +BTF_ID(func, vfs_test_lock)
>> +BTF_ID(func, vfs_setlease)
>> +BTF_ID(func, vfs_lock_file)
> 
> I double checked that for the above three lock
> related functions (vfs_cancel_lock, vfs_test_lock,
> vfs_lock_file), I double checked d_path
> does not use these locks, so we should be fine.
> 
>> +BTF_ID(func, dentry_open)
>>   BTF_ID(func, filp_close)
>>   BTF_SET_END(btf_allowlist_d_path)
>>
>> -- 
>> 2.25.1
>>
diff mbox series

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index c5e0b6a64091..355777b5bf63 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -850,16 +850,64 @@  BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
 BTF_SET_START(btf_allowlist_d_path)
 #ifdef CONFIG_SECURITY
 BTF_ID(func, security_file_permission)
-BTF_ID(func, security_inode_getattr)
 BTF_ID(func, security_file_open)
+BTF_ID(func, security_file_ioctl)
+BTF_ID(func, security_file_free)
+BTF_ID(func, security_file_alloc)
+BTF_ID(func, security_file_lock)
+BTF_ID(func, security_file_fcntl)
+BTF_ID(func, security_file_set_fowner)
+BTF_ID(func, security_file_receive)
+BTF_ID(func, security_inode_getattr)
+BTF_ID(func, security_sb_mount)
+BTF_ID(func, security_bprm_check)
 #endif
 #ifdef CONFIG_SECURITY_PATH
 BTF_ID(func, security_path_truncate)
+BTF_ID(func, security_path_notify)
+BTF_ID(func, security_path_unlink)
+BTF_ID(func, security_path_mkdir)
+BTF_ID(func, security_path_rmdir)
+BTF_ID(func, security_path_mknod)
+BTF_ID(func, security_path_symlink)
+BTF_ID(func, security_path_link)
+BTF_ID(func, security_path_rename)
+BTF_ID(func, security_path_chmod)
+BTF_ID(func, security_path_chown)
+BTF_ID(func, security_path_chroot)
 #endif
 BTF_ID(func, vfs_truncate)
 BTF_ID(func, vfs_fallocate)
-BTF_ID(func, dentry_open)
 BTF_ID(func, vfs_getattr)
+BTF_ID(func, vfs_fadvise)
+BTF_ID(func, vfs_fchmod)
+BTF_ID(func, vfs_fchown)
+BTF_ID(func, vfs_open)
+BTF_ID(func, vfs_setpos)
+BTF_ID(func, vfs_llseek)
+BTF_ID(func, vfs_read)
+BTF_ID(func, vfs_write)
+BTF_ID(func, vfs_iocb_iter_read)
+BTF_ID(func, vfs_iter_read)
+BTF_ID(func, vfs_readv)
+BTF_ID(func, vfs_iocb_iter_write)
+BTF_ID(func, vfs_iter_write)
+BTF_ID(func, vfs_writev)
+BTF_ID(func, vfs_copy_file_range)
+BTF_ID(func, vfs_getattr_nosec)
+BTF_ID(func, vfs_ioctl)
+BTF_ID(func, vfs_fsync_range)
+BTF_ID(func, vfs_fsync)
+BTF_ID(func, vfs_utimes)
+BTF_ID(func, vfs_statfs)
+BTF_ID(func, vfs_dedupe_file_range_one)
+BTF_ID(func, vfs_dedupe_file_range)
+BTF_ID(func, vfs_clone_file_range)
+BTF_ID(func, vfs_cancel_lock)
+BTF_ID(func, vfs_test_lock)
+BTF_ID(func, vfs_setlease)
+BTF_ID(func, vfs_lock_file)
+BTF_ID(func, dentry_open)
 BTF_ID(func, filp_close)
 BTF_SET_END(btf_allowlist_d_path)