Re: [PATCH v2 0/2] perf trace: Fix support for the new BPF feature in clang 12

[Namhyung Kim]

Hi Arnaldo,

On Tue, Oct 15, 2024 at 05:37:38PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Oct 15, 2024 at 04:58:56PM -0300, Arnaldo Carvalho de Melo wrote:
> > So I'm trying adding extra bounds checking, marking the index as
> > volatile, adding compiler barriers, etc, all the fun with the verifier,
> > but got distracted with other stuff, coming back to this now.
>  
> > Ok, the following seems to do the trick:
>  
> > [acme@dell-per740-01 perf-tools]$ git diff
> > diff --git a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > index 3b30aa74a3ae..ef87a04ff8d0 100644
> > --- a/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > +++ b/tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c
> > @@ -486,6 +486,7 @@ static int augment_sys_enter(void *ctx, struct syscall_enter_args *args)
> >                                 augmented = true;
> >                 } else if (size < 0 && size >= -6) { /* buffer */
> >                         index = -(size + 1);
> > +                       index &= 7; // To satisfy the bounds checking with the verifier in some kernels
> >                         aug_size = args->args[index];
> >  
> >                         if (aug_size > TRACE_AUG_MAX_BUF)
> > 
> > I'll now test it without Howard's patch to see if it fixes the RHEL8 +
> > clang 17 case.
> 
> It works with this one-liner + the simplified patch from Howard and also
> on this other system (RHEL9), as well as with Fedora 40, it would be
> nice if someone could test with clang 16 and report back the version of
> the kernel tested as well as the distro name/release, that way I can try
> to get my hands on such as system and test there as well.
> 
> Its all at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git tmp.perf-tools
> 
> This is the current set of patches that when further tested will go to
> Linus for v6.12:
> 
> ⬢[acme@toolbox perf-tools]$ git log --oneline torvalds/master..
> ff14baa7a290bf42 (HEAD -> perf-tools, x1/perf-tools, perf-tools/tmp.perf-tools) perf trace augmented_raw_syscalls: Add more checks to pass the verifier
> 46180bec048aad85 perf trace augmented_raw_syscalls: Add extra array index bounds checking to satisfy some BPF verifiers
> 45d1aadac64869a2 perf build: Change the clang check back to 12.0.1

Wouldn't it be better to have this change after fixing the verifier
issues in the later commits?

> 4e21679eb81b5f0d perf trace: The return from 'write' isn't a pid
> 2d2314d4b09b5ed9 tools headers UAPI: Sync linux/const.h with the kernel headers
> ⬢[acme@toolbox perf-tools]$

I guess you also need the syscalltbl fix from Jiri Slaby.

https://lore.kernel.org/linux-perf-users/3a592835-a14f-40be-8961-c0cee7720a94@xxxxxxxxxx/

Thanks,
Namhyung

> 
> [root@nine ~]# uname -a
> Linux nine 5.14.0-427.31.1.el9_4.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Aug 9 14:06:03 EDT 2024 x86_64 x86_64 x86_64 GNU/Linux
> [root@nine ~]# perf trace -e *sleep* sleep 1.234567890
>      0.000 (1234.742 ms): sleep/80014 clock_nanosleep(rqtp: 0x7ffc55b11240, rmtp: 0x7ffc55b11230)           = 0
> [root@nine ~]# clang --version
> clang version 17.0.6 (Red Hat, Inc. 17.0.6-5.el9)
> Target: x86_64-redhat-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin
> [root@nine ~]#
> 
> - Arnaldo