linux-kernel - Re: [PATCH 1/1] perf build: Require at least clang 16.0.6 to build BPF skeletons

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Zwl3IsgE3drp7mLo@x1>
Date: Fri, 11 Oct 2024 16:06:10 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: James Clark <james.clark@...aro.org>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Howard Chu <howardchu95@...il.com>,
	Alan Maguire <alan.maguire@...cle.com>,
	Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Jiri Olsa <jolsa@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	linux-perf-users <linux-perf-users@...r.kernel.org>
Subject: Re: [PATCH 1/1] perf build: Require at least clang 16.0.6 to build
 BPF skeletons

On Thu, Oct 10, 2024 at 10:00:41AM +0100, James Clark wrote:
> 
> 
> On 10/10/2024 2:20 am, Arnaldo Carvalho de Melo wrote:
> > On Wed, Oct 9, 2024, 10:01 PM Namhyung Kim <namhyung@...nel.org> wrote:
> > 
> > > On Tue, Oct 08, 2024 at 12:27:24AM -0700, Howard Chu wrote:
> > > > Hi Alan, Arnaldo and James,
> > > > 
> > > > This problem was solved in [PATCH 0/2] perf trace: Fix support for the
> > > > new BPF feature in clang 12 (Link:
> > > > 
> > > https://lore.kernel.org/linux-perf-users/20241007051414.2995674-1-howardchu95@gmail.com/T/#t
> > > ),
> > > > sorry I forgot to cc you two.
> > > > 
> > > > Alan's thought was correct. Thank you so much! :)
> > > 
> > > It'd be great if any of you can test this change.  Now I only have
> > > machines with clang 16.
> > > 
> > 
> > I'll test this tomorrow.
> > 
> > - Arnaldo
> > 
> > > 
> > > Thanks,
> > > Namhyung
> > > 
> > > 
> > 
> 
> Tested with clang 15:
> 
> $ sudo perf trace -e write --max-events=100 -- echo hello
> 
>   0.000 ( 0.014 ms): echo/834165 write(fd: 1, buf: hello\10, count: 6)
> 
>                                            =
> 
> Tested-by: James Clark <james.clark@...aro.org>
> 
> 
> Unrelated to this change, I noticed that with older clangs or with
> BUILD_BPF_SKEL=0 that commit b257fac12f38 ("perf trace: Pretty print buffer
> data") changes the buffer address to print nothing, and the '6' return value
> is missing. Not sure if this was intended or not:
> 
>  $ sudo perf trace -e write --max-events=100 -- echo hello
> 
> Before:
>      0.000 ( 0.009 ms): echo/772951 write(fd: 1, buf: 0x58c415257440,
>         count: 6)                           = 6
> 
> After:
>      0.000 ( 0.009 ms): echo/760370 write(fd: 1, buf: , count: 6)
> 

I noticed this with fedora:40, and bisected it to:

⬢[acme@...lbox perf-tools]$ git bisect good
b257fac12f38d7f503b932313d704cee21092350 is the first bad commit
commit b257fac12f38d7f503b932313d704cee21092350
Author: Howard Chu <howardchu95@...il.com>
Date:   Sun Aug 25 00:33:19 2024 +0800

    perf trace: Pretty print buffer data
    
    Define TRACE_AUG_MAX_BUF in trace_augment.h data, which is the maximum
    buffer size we can augment. BPF will include this header too.
    
    Print buffer in a way that's different than just printing a string, we
    print all the control characters in \digits (such as \0 for null, and
    \10 for newline, LF).
    
    For character that has a bigger value than 127, we print the digits
    instead of the character itself as well.
    
    Committer notes:
    
    Simplified the buffer scnprintf to avoid using multiple buffers as
    discussed in the patch review thread.
    
    We can't really all 'buf' args to SCA_BUF as we're collecting so far
    just on the sys_enter path, so we would be printing the previous 'read'
    arg buffer contents, not what the kernel puts there.
    
    So instead of:
       static int syscall_fmt__cmp(const void *name, const void *fmtp)
      @@ -1987,8 +1989,6 @@ syscall_arg_fmt__init_array(struct syscall_arg_fmt *arg, struct tep_format_field
      -               else if (strstr(field->type, "char *") && strstr(field->name, "buf"))
      -                       arg->scnprintf = SCA_BUF;
    
    Do:
    
    static const struct syscall_fmt syscall_fmts[] = {
      +       { .name     = "write",      .errpid = true,
      +         .arg = { [1] = { .scnprintf = SCA_BUF /* buf */, from_user = true, }, }, },
    
    Signed-off-by: Howard Chu <howardchu95@...il.com>
    Cc: Adrian Hunter <adrian.hunter@...el.com>
    Cc: Ian Rogers <irogers@...gle.com>
    Cc: Jiri Olsa <jolsa@...nel.org>
    Cc: Kan Liang <kan.liang@...ux.intel.com>
    Cc: Namhyung Kim <namhyung@...nel.org>
    Link: https://lore.kernel.org/r/20240815013626.935097-8-howardchu95@gmail.com
    Link: https://lore.kernel.org/r/20240824163322.60796-6-howardchu95@gmail.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>


Just did a:

⬢[acme@...lbox perf-tools]$ git checkout b257fac12f38d7f503b932313d704cee21092350
Updating files: 100% (12326/12326), done.
Note: switching to 'b257fac12f38d7f503b932313d704cee21092350'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.
<SNIP>
HEAD is now at b257fac12f38d7f5 perf trace: Pretty print buffer


make clean + rebuild and the returns are gone, if I do:

⬢[acme@...lbox perf-tools]$ git checkout b257fac12f38d7f503b932313d704cee21092350^
Previous HEAD position was b257fac12f38d7f5 perf trace: Pretty print buffer data
HEAD is now at cb32035214b9a09d perf trace: Pretty print struct data
⬢[acme@...lbox perf-tools]$ 

clean + rebuild instead I get those returns back:

Now staring at it...


- Arnaldo