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

[Hengqi Chen <hengqi.chen@gmail.com>]

On 2021/7/20 3:02 AM, Andrii Nakryiko wrote:
> On Mon, Jul 19, 2021 at 8:18 AM Hengqi Chen <hengqi.chen@gmail.com> 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.
>>
>> Changes since v1: [1]
>>  - Alexei and Yonghong suggested that bpf_d_path helper could also
>>    apply to vfs_* and security_file_* kernel functions. Added them.
>>
>> [1] https://lore.kernel.org/bpf/20210712162424.2034006-1-hengqi.chen@gmail.com/
>> [2] https://github.com/iovisor/bcc/issues/3527
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
>> ---
>>  kernel/trace/bpf_trace.c | 50 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 08906007306d..c784f3c7143f 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -850,16 +850,62 @@ 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)
>>  #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)
>>
> 
> Before we lend this expanded list of allowed functions, I think we
> should address an issue that comes up from time to time with .BTF_ids.
> Sometimes the referenced function can be changed from global to static
> and get inlined by the compiler, and thus disappears from BTF
> altogether. This will result in kernel build failure causing a lot of
> confusion, because the change might be done by people unfamiliar with
> the BTF_ID() stuff and not even aware of it.
> 

Thanks for the detailed background.
I was able to reproduce this kernel build failure.

> This came up a few times before and it's frustrating for everyone
> involved. Before we proceed with extending the list further, let's
> teach resolve_btfids to warn on such missing function (so that we are
> at least aware) but otherwise ignore it (probably leaving ID as zero,
> but let's also confirm that all the users of BTF_ID() stuff handle
> those zeros correctly).
> 

Do you mean that resolve_btfids should be updated to emit warning messages
instead of aborting the build process ?

>> --
>> 2.25.1
>>