Re: [PATCHv2 bpf-next] selftests/bpf: Fix stat probe in d_path test

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Sep 21, 2020 at 04:32:13PM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 18, 2020 at 4:23 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Some kernels builds might inline vfs_getattr call within fstat
> > syscall code path, so fentry/vfs_getattr trampoline is not called.
> >
> > Alexei suggested [1] we should use security_inode_getattr instead,
> > because it's less likely to get inlined. Using this idea also for
> > vfs_truncate (replaced with security_path_truncate) and vfs_fallocate
> > (replaced with security_file_permission).
> >
> > Keeping dentry_open and filp_close, because they are in their own
> > files, so unlikely to be inlined, but in case they are, adding
> > security_file_open.
> >
> > Switching the d_path test stat trampoline to security_inode_getattr.
> >
> > Adding flags that indicate trampolines were called and failing
> > the test if any of them got missed, so it's easier to identify
> > the issue next time.
> >
> > Suggested-by: Alexei Starovoitov <ast@kernel.org>
> > [1] https://lore.kernel.org/bpf/CAADnVQJ0FchoPqNWm+dEppyij-MOvvEG_trEfyrHdabtcEuZGg@mail.gmail.com/
> > Fixes: e4d1af4b16f8 ("selftests/bpf: Add test for d_path helper")
> > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> > ---
> > v2 changes:
> >   - replaced vfs_* function with security_* in d_path allow list
> >     vfs_truncate  -> security_path_truncate
> >     vfs_fallocate -> security_file_permission
> >     vfs_getattr   -> security_inode_getattr
> >   - added security_file_open to d_path allow list
> >   - split verbose output for trampoline flags
> >
> >  kernel/trace/bpf_trace.c                        |  7 ++++---
> >  tools/testing/selftests/bpf/prog_tests/d_path.c | 10 ++++++++++
> >  tools/testing/selftests/bpf/progs/test_d_path.c |  9 ++++++++-
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index b2a5380eb187..e24323d72cac 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -1118,10 +1118,11 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> >  }
> >
> >  BTF_SET_START(btf_allowlist_d_path)
> > -BTF_ID(func, vfs_truncate)
> > -BTF_ID(func, vfs_fallocate)
> > +BTF_ID(func, security_path_truncate)
> > +BTF_ID(func, security_file_permission)
> > +BTF_ID(func, security_inode_getattr)
> > +BTF_ID(func, security_file_open)
> >  BTF_ID(func, dentry_open)
> > -BTF_ID(func, vfs_getattr)
> >  BTF_ID(func, filp_close)
> >  BTF_SET_END(btf_allowlist_d_path)
> 
> bpf CI system flagged the build error:
> FAILED unresolved symbol security_path_truncate
> because CONFIG_SECURITY_PATH wasn't set.
> Which points to the issue with this patch that the above
> security_* funcs have to be guarded with appropriate #ifdef.

ugh, sry

> I don't have a use case for tracing vfs_truncate, but
> security_path_unlink I would want to do in the future.
> Unfortunately it's under the same SECURITY_PATH ifdef.
> So my earlier desire to make it fool proof is not feasible at this point.
> Adding 'was_probed_func_inlined' check to libbpftrace.a would
> solve it eventually.
> For now I think we have to live with this function probing fragility.
> So I've modified the patch to add these few security_* funcs
> and kept vfs_* equivalents.
> Also reworded commit log and applied to bpf-next. Thanks
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=a8a717963fe5ecfd274eb93dd1285ee9428ffca7
> 

ok, looks good, thanks,
jirka