linux-kernel - Re: [PATCH] perf tests: Don't retest sections in "Object code reading"

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aOZplPKCRwvlOgUA@stanley.mountain>
Date: Wed, 8 Oct 2025 16:39:32 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: James Clark <james.clark@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
	Ian Rogers <irogers@...gle.com>,
	Charlie Jenkins <charlie@...osinc.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>, Leo Yan <leo.yan@....com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf tests: Don't retest sections in "Object code
 reading"

On Wed, Oct 08, 2025 at 11:21:34AM +0100, James Clark wrote:
> 
> 
> On 08/10/2025 9:32 am, James Clark wrote:
> > 
> > 
> > On 07/10/2025 9:01 pm, Arnaldo Carvalho de Melo wrote:
> > > On Tue, Oct 07, 2025 at 10:10:12AM +0100, James Clark wrote:
> > > > On 06/10/2025 4:21 pm, Ian Rogers wrote:
> > > > > On Mon, Oct 6, 2025 at 6:11 AM James Clark
> > > > > <james.clark@...aro.org> wrote:
> > > > > > +       data = zalloc(sizeof(*data));
> > > > > > +       if (!data)
> > > > > > +               return true;
> > > 
> > > > > > +       data->addr = addr;
> > > > > > +       strlcpy(data->path, path, sizeof(data->path));
> > > > > nit: perhaps strdup rather than having 4kb per tested_section.
> > > 
> > > > Oh yeah that would have been better, not sure why I didn't do it
> > > > that way.
> > > > Although the max sections I saw was around 50, and it's usually
> > > > a lot less
> > > > so it's probably not worth the churn to change it now that
> > > > Arnaldo's applied
> > > > it?
> > > 
> > > I see you submitted a patch for using strdup() and then there is a need
> > > for checking the strdup(), etc.
> > > 
> > > Since at this point this is an improvement on a test and all is sitting
> > > in linux-next and the window is closing for v6.18, lets leave this for
> > > the next window, ok?
> > > 
> > 
> > Makes sense.
> > 
> > > These would be good things for some tool to catch, before it gets sent,
> > > but that is another rabbit hole :-)
> > > 
> > > Thanks,
> > > 
> > > - Arnaldo
> > 
> > Does Smatch work on Perf? I imagine it would catch this if it does. Or
> > just plain old cppcheck. I'll take a look.
> > 
> > James
> > 
> 
> Smatch doesn't know about strdup and seems to be more focused on kernel so
> probably isn't a good fit.
> 

No one is going to write a check which tells people when to use a fixed
array vs when to allocate memory dynamically.  Those sorts of decisions
are too complicated.

> Cppcheck struggles with a lot of the kernely style that's used in Perf,
> especially the headers. We'd either have to silence a lot of the warnings,
> or start with almost no warnings enabled.
> 
> It doesn't have a warning for usage of a malloc return value without NULL
> checking, so in this case it wouldn't be useful.

In the kernel we care a lot about missing NULL checks but in userspace
it doesn't really matter in the same way...  It's easy enough to write
a check for this in Smatch.

> 
> I'm not 100% convinced that the effort of integrating one of these into the
> build system would be worth it. I know that once they're in, there would be
> constant maintenance of silencing false positives etc. And a lot of the
> warnings are more style or opinions about undefined behavior according to
> the standard, but aren't real based on what the compiler actually does.
> 

The thing about false positives is that people should just ignore all the
warnings except the new ones.  In the kernel, this works well because we
fix all the real bugs right away.  And if we haven't eventually someone
will look at the code again and after 10 years or so it all either gets
fixed or it doesn't matter.

> As an example, cppcheck on code-reading.c with --enable=all gives 17
> portability, 11 style, 3 warning and 1 error outputs. Out of all of these
> only two of the warnings are significant, from commit 0f9ad973b095 ("perf
> tests code-reading: Handle change in objdump output from binutils >= 2.41 on
> riscv"):
> 
>   token = strsep(&version, ".");
>   version_tmp = atoi(token);
>   if (token)
>       version_num += version_tmp * 100;
> 
>   token = strsep(&version, ".");
>   version_tmp = atoi(token);
>   if (token)
>       version_num += version_tmp;
> 			
> "token" has already been passed to atoi() so can't be NULL in the if
> statement. I think the atoi() needs to come after the null check.
>

Yep.  Smatch does cross function analysis so it would have caught that.
(I haven't tested).

regards,
dan carpenter