linux-kernel - Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

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: <20221022112228.GB30596@1wt.eu>
Date:   Sat, 22 Oct 2022 13:22:28 +0200
From:   Willy Tarreau <w@....eu>
To:     "Paul E. McKenney" <paulmck@...nel.org>
Cc:     Rasmus Villemoes <linux@...musvillemoes.dk>,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selftests/nolibc: add 7 tests for memcmp()

On Fri, Oct 21, 2022 at 11:00:40AM -0700, Paul E. McKenney wrote:
> > It's even easier, you don't even need the clean phase in include/nolibc.
> > I'm doing this and it's sufficient:
> > 
> >   make -C tools/testing/selftests/nolibc clean
> >   make -C tools/testing/selftests/nolibc nolibc-test
> >   tools/testing/selftests/nolibc/nolibc-test
> > 
> > Or for the test under QEMU, which involves a kernel build:
> > 
> >   make -C tools/testing/selftests/nolibc clean
> >   make -C tools/testing/selftests/nolibc -j $(nproc) run
> > 
> > Where would you first look for such a hint ? Maybe the help output of
> > the default "make" command could send as a hint that a clean is needed
> > after patching nolibc and that could be sufficient ? I just want to make
> > sure users don't waste their time trying to find what they could be doing
> > wrong.
> 
> Maybe it suffices for the near term for me to put this information in
> the signed tag for the pull request?

It can be sufficient for short term indeed, but it can be easy as well
for me to mention it in the make output.

> Another approach would be to remind about "make clean" in the case of
> a test failure.  Or make test failure combined with a detected change
> trigger an automatic "make clean" and a retry.

In fact failures are not the only case. For me it was the opposite. I
applied Rasmus' fix, then I developed the test, verified that it worked,
then reverted Rasmus' fix... to find that the test didn't catch the
failure. I had a second look at the original patch and figured that
the -192..+192 values were really not possible with a char so I
concluded that a clean was needed. But leaving something in a claimed
working state while it's not can be sufficiently misleading and make
one waste significant time, because in such cases we rarely search why
it works.

> Or other schemes of increasing complexity and fragility.  ;-)

That's exactly what I'd like to avoid with such a lightweight component.
If it takes more time to figure why something is going wrong than to
write a test, we'll all give up. I think that a clean for QEMU is worth
it because the kernel is rebuilt and its dependencies are quite robust,
so that one would be a surprise. For other tests, probably leaving it
explicit with a hint that it's needed should suffice. I'll recheck what
conditions the installation of uapi headers because that's really what
I don't want to see happening all the time. The rest is discrete, it's
just a few files being copied, maybe it can be done every time.

Will keep thinking about it and hopefully propose a patch to make the
tests easier to use before we're too far in the 6.1 release.

Thanks for keeping the conversation flowing, that helps me!
Willy