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

From: Hengqi Chen <hengqi.chen@gmail.com>
To: Yonghong Song <yhs@fb.com>, bpf@vger.kernel.org
Cc: ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	john.fastabend@gmail.com, jolsa@kernel.org, yanivagman@gmail.com
Subject: Re: [PATCH bpf-next 2/2] bpf: expose bpf_d_path helper to vfs_* and security_* functions
Date: Mon, 26 Jul 2021 15:16:23 +0800	[thread overview]
Message-ID: <78894c4d-31e3-f200-b3ea-18371fa98cf6@gmail.com> (raw)
In-Reply-To: <3bfc8755-bd34-2ff1-698f-57ad046726c2@fb.com>

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
>>