linux-kernel - Re: [RFC PATCH 7/7] tools: add skeleton code for userland testing of VMA logic

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: <6bd118dd-de4b-4ccd-bdbe-f8c45e8ea783@lucifer.local>
Date: Thu, 27 Jun 2024 20:25:13 +0100
From: Lorenzo Stoakes <lstoakes@...il.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Vlastimil Babka <vbabka@...e.cz>,
	Matthew Wilcox <willy@...radead.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>
Subject: Re: [RFC PATCH 7/7] tools: add skeleton code for userland testing of
 VMA logic

On Thu, Jun 27, 2024 at 01:20:30PM -0400, Liam R. Howlett wrote:
[snip]
> > +
> > +clean:
> > +	$(RM) $(TARGETS) *.o radix-tree.c idr.c generated/map-shift.h generated/bit-length.h
>
> This needs to clean out vma.c to avoid stale testing.
>
> But, none of this is needed.
>
> What we can do instead is add the correct header guards to the
> mm/vma_internal.h file, change the tools/testing/vma/vma_internal.h
> header guards to be the same (ie: remove TESTING_ from the existing
> ones), then we can include vma_internal.h into vma_stub.c prior to
> including "../../../mm/vma.c", and we don't need to copy the file.
>
> Essentially use the #ifdef guards to replace the header by ordering the
> local header for inclusion prior to the c file.

Ack this is a good idea, will do in v2.

>
>
> > diff --git a/tools/testing/vma/errors.txt b/tools/testing/vma/errors.txt
> > new file mode 100644
> > index 000000000000..e69de29bb2d1
> > diff --git a/tools/testing/vma/generated/autoconf.h b/tools/testing/vma/generated/autoconf.h
> > new file mode 100644
> > index 000000000000..92dc474c349b
> > --- /dev/null
> > +++ b/tools/testing/vma/generated/autoconf.h
> > @@ -0,0 +1,2 @@
> > +#include "bit-length.h"
> > +#define CONFIG_XARRAY_MULTI 1
> > diff --git a/tools/testing/vma/linux/atomic.h b/tools/testing/vma/linux/atomic.h
> > new file mode 100644
> > index 000000000000..298b0fb7aab2
> > --- /dev/null
> > +++ b/tools/testing/vma/linux/atomic.h
>
> This should have header guards as well.

Yup, the reason I kept it like this is because existing linux/*.h headers
in shared/linux didn't have header guards and I wanted to keep things in
line with that... will change.

>
> > @@ -0,0 +1,19 @@
> > +#ifndef atomic_t
> > +#define atomic_t int32_t
> > +#endif
> > +
> > +#ifndef atomic_inc
> > +#define atomic_inc(x) uatomic_inc(x)
> > +#endif
> > +
> > +#ifndef atomic_read
> > +#define atomic_read(x) uatomic_read(x)
> > +#endif
> > +
> > +#ifndef atomic_set
> > +#define atomic_set(x, y) do {} while (0)
> > +#endif
> > +
> > +#ifndef U8_MAX
> > +#define U8_MAX UCHAR_MAX
> > +#endif
> > diff --git a/tools/testing/vma/linux/mmzone.h b/tools/testing/vma/linux/mmzone.h
> > new file mode 100644
> > index 000000000000..71546e15bdd3
> > --- /dev/null
> > +++ b/tools/testing/vma/linux/mmzone.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _TOOLS_MMZONE_H
> > +#define _TOOLS_MMZONE_H
>
> It might be best to use the same guards here to avoid mmzone.h from
> getting pulled in.

You mean the actual [root]/include/linux/mmzone.h ? Just deploying the same
header guard trick as mentioned above re: vma_internal.h?

[snip]

> > new file mode 100644
> > index 000000000000..b29eeb0daf31
> > --- /dev/null
> > +++ b/tools/testing/vma/main.c
>
> If you employ the use of header guards, we can rename main.c to vma.c
> and produce the executable "vma" instead of "main".

Sure, will do.

>
> > @@ -0,0 +1,161 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <assert.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +
> > +#include "maple-shared.h"
> > +#include "vma_internal.h"
> > +#include "vma.h"
>
> You can directly include "../../../mm/vma.h" here and remove the vma.h
> file you have in this directory.

This was, I think, to keep to a convention, but you're right I don't think
there's any reason to do this, will change.

[snip]

> > +int main(void)
> > +{
> > +	maple_tree_init();
> > +
> > +	test_simple_merge();
> > +	test_simple_modify();
> > +	test_simple_expand();
> > +	test_simple_shrink();
> > +
> > +	return EXIT_SUCCESS;
> > +}
>
> It would be nice to have some output stating the number of tests
> passed/failed.

Ack, will add.

>
> > diff --git a/tools/testing/vma/vma.h b/tools/testing/vma/vma.h
> > new file mode 100644
> > index 000000000000..87a6cb222b63
> > --- /dev/null
> > +++ b/tools/testing/vma/vma.h
> > @@ -0,0 +1,3 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +
> > +#include "../../../mm/vma.h"
>
> I'd rather just drop this file and have this line in main.c (or vma.c if
> you decide to rename it).

Ack, will do.

[snip]