Message-ID: <CABVgOS=7wrxywmgn8YRW4o_sUN=wOxa4k7NbTObAxA5okmr+CQ@mail.gmail.com>
Date: Fri, 22 Aug 2025 16:57:49 +0800
From: David Gow <davidgow@...gle.com>
To: Marco Elver <elver@...gle.com>
Cc: Ignat Korchagin <ignat@...udflare.com>, Eric Biggers <ebiggers@...nel.org>, 
	Ethan Graham <ethan.w.s.graham@...il.com>, ethangraham@...gle.com, glider@...gle.com, 
	andreyknvl@...il.com, brendan.higgins@...ux.dev, dvyukov@...gle.com, 
	jannh@...gle.com, rmoar@...gle.com, shuah@...nel.org, tarasmadan@...gle.com, 
	kasan-dev@...glegroups.com, kunit-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	David Howells <dhowells@...hat.com>, Lukas Wunner <lukas@...ner.de>, 
	Herbert Xu <herbert@...dor.apana.org.au>, "David S. Miller" <davem@...emloft.net>, 
	"open list:HARDWARE RANDOM NUMBER GENERATOR CORE" <linux-crypto@...r.kernel.org>
Subject: Re: [PATCH v1 RFC 6/6] crypto: implement KFuzzTest targets for PKCS7
 and RSA parsing

On Tue, 19 Aug 2025 at 18:08, Marco Elver <elver@...gle.com> wrote:
>
> On Fri, 15 Aug 2025 at 15:00, Ignat Korchagin <ignat@...udflare.com> wrote:
> >
> > On Fri, Aug 15, 2025 at 2:18 AM Eric Biggers <ebiggers@...nel.org> wrote:
> > >
> > > On Thu, Aug 14, 2025 at 04:28:13PM +0100, Ignat Korchagin wrote:
> > > > Not sure if it has been mentioned elsewhere, but one thing I already
> > > > don't like about it is that these definitions "pollute" the actual
> > > > source files. Might not be such a big deal here, but kernel source
> > > > files for core subsystems tend to become quite large and complex
> > > > already, so not a great idea to make them even larger and harder to
> > > > follow with fuzz definitions.
> > > >
> > > > As far as I'm aware, for the same reason KUnit [1] is not that popular
> > > > (or at least less popular than other approaches, like selftests [2]).
> > > > Is it possible to make it that these definitions live in separate
> > > > files or even closer to selftests?
> > >
> > > That's not the impression I get.  KUnit suites are normally defined in
> > > separate files, and KUnit seems to be increasing in popularity.
> >
> > Great! Either I was wrong from the start or it changed and I haven't
> > looked there recently.
> >
> > > KFuzzTest can use separate files too, it looks like?
> > >
> > > Would it make any sense for fuzz tests to be a special type of KUnit
> > > test, instead of a separate framework?
> >
> > I think so, if possible. There is always some hurdles adopting new
> > framework, but if it would be a new feature of an existing one (either
> > KUnit or selftests - whatever fits better semantically), the existing
> > users of that framework are more likely to pick it up.
>
> The dependency would be in name only (i.e. "branding"). Right now it's
> possible to use KFuzzTest without the KUnit dependency. So there is
> technical merit to decouple.
>

There's definitely some overlap between KFuzzTest and KUnit, from the
relatively superficial API similarities: both having similar
ASSERT/EXPECT macros; to the more specific: KUnit parameterised tests
allow running the same 'test' code against several different pieces of
input data.

Then again, there are definitely some differences, too: KUnit doesn't
have a way of describing complex binary data (though it's definitely a
feature we'd like one day), and the purpose of KUnit tests, while
having some overlap, is different than exposing fuzz targets.

If there's a bit of KUnit functionality you can reasonably re-use or
otherwise take advantage of, I'd not discourage you from doing so.
There's a balance to be struck between taking the extra dependency and
ending up with duplicate implementations of the same thing.

I also think that what Ignat says below around simply ensuring that
the API is familiar (i.e., not deviating from what KUnit or other
frameworks do without a good reason) is a good middle ground here.

So my gut feeling is that you could end up with one of three things:
- Make KFuzzTests a special case of (parameterised) KUnit tests. This
would probably involve adding a way to run the tests with a parameter
from debugfs or a kernel command-line argument using the metadata
format, and teaching the fuzzer to run KUnit tests. KUnit already has
an attributes mechanism that could be used to note which tests are
fuzz targets, and maybe even providing some of the annotation, but
there'd be some work needed to improve it. The big advantage here is
that you'd automatically gain the ability to use KUnit helpers to set
up things like memory regions, fake devices, etc.
- Share some of the implementation details between KUnit and
KFuzzTest, but keep them as separate things. We already have a bunch
of, e.g., work on assertions, logging, etc, which could possibly be
helpful. This could be done by depending on CONFIG_KUNIT or by
splitting those out into a shared test library.
- Keep them separate, but be careful to make the APIs similar enough
to be familiar. KFuzzTest already looks pretty similar to me, so I
think we're already in a good place here.

Personally, I'd quite like for there to be a bit more overlap/code
sharing -- at least eventually -- as I could see some benefits to
"borrowing" some KFuzzTest code to allow, e.g., providing custom
inputs/outputs for tests.

> Would sufficient documentation, and perhaps suggesting separate files
> to be the canonical way of defining KFuzzTests, improve the situation?
>
> For example something like:
> For subsystem foo.c, define a KFuzzTest in foo_kfuzz.c, and then in
> the Makfile add "obj-$(CONFIG_KFUZZTEST) += foo_kfuzz.o".
> Alternatively, to test internal static functions, place the KFuzzTest
> harness in a file foo_kfuzz.h, and include at the bottom of foo.c.
>
> Alex, Ethan, and KUnit folks: What's your preference?

I think that keeping tests in separate files by default is the right
way to go, but obviously either #including them or using a whole bunch
of conditional symbol exports (either with symbol namespaces or
something like EXPORT_SYMBOL_IF_KUNIT) will be necessary in some cases
to get coverage of internal functions.

I'd suggest being a little careful with the naming scheme, as Linus
was not happy with the foo_test.c names we were using as they make tab
completion more annoying; we ended up putting tests in a 'tests/'
subdirectory where appropriate:
https://docs.kernel.org/dev-tools/kunit/style.html

But ultimately, I think this is a style decision, not a critically
important technical one: provide some good practices to follow -- and
encourage people to be consistent -- but understand that occasionally
a maintainer will override it (sometimes even for good reason).

Cheers,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)

Powered by blists - more mailing lists