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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Jul 21, 2021 at 10:01 PM Hengqi Chen <hengqi.chen@gmail.com> wrote:
>
>
>
> 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 ?

Yes. Just double-check that unresolved BTF IDs are filled out as zeros
and don't break anything.

>
> >> --
> >> 2.25.1
> >>