linux-kernel - Re: [PATCH v3 4/7] mm: move internal core VMA manipulation functions to own file

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: <caf36eb7-f737-49b6-aa3a-f9196f6a17f6@lucifer.local>
Date: Tue, 23 Jul 2024 18:15:36 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: SeongJae Park <sj@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        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>, Shuah Khan <shuah@...nel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>
Subject: Re: [PATCH v3 4/7] mm: move internal core VMA manipulation functions
 to own file

On Tue, Jul 23, 2024 at 09:58:25AM GMT, SeongJae Park wrote:
> Hi Lorenzo,
>
> On Mon, 22 Jul 2024 12:50:22 +0100 Lorenzo Stoakes <lorenzo.stoakes@...cle.com> wrote:
>
> > This patch introduces vma.c and moves internal core VMA manipulation
> > functions to this file from mmap.c.
> >
> > This allows us to isolate VMA functionality in a single place such that we
> > can create userspace testing code that invokes this functionality in an
> > environment where we can implement simple unit tests of core functionality.
> >
> > This patch ensures that core VMA functionality is explicitly marked as such
> > by its presence in mm/vma.h.
> >
> > It also places the header includes required by vma.c in vma_internal.h,
> > which is simply imported by vma.c. This makes the VMA functionality
> > testable, as userland testing code can simply stub out functionality
> > as required.
> >
> > Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
> > Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> >  include/linux/mm.h |   35 -
> >  mm/Makefile        |    2 +-
> >  mm/internal.h      |  236 +-----
> >  mm/mmap.c          | 1980 +++-----------------------------------------
> >  mm/mmu_notifier.c  |    2 +
> >  mm/vma.c           | 1766 +++++++++++++++++++++++++++++++++++++++
> >  mm/vma.h           |  364 ++++++++
> >  mm/vma_internal.h  |   52 ++
> >  8 files changed, 2294 insertions(+), 2143 deletions(-)
> >  create mode 100644 mm/vma.c
> >  create mode 100644 mm/vma.h
> >  create mode 100644 mm/vma_internal.h
> >
> [...]
> > diff --git a/mm/vma_internal.h b/mm/vma_internal.h
> > new file mode 100644
> > index 000000000000..e13e5950df78
> > --- /dev/null
> > +++ b/mm/vma_internal.h
> > @@ -0,0 +1,52 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * vma_internal.h
> > + *
> > + * Headers required by vma.c, which can be substituted accordingly when testing
> > + * VMA functionality.
> > + */
> > +
> > +#ifndef __MM_VMA_INTERNAL_H
> > +#define __MM_VMA_INTERNAL_H
> > +
> [...]
> > +#include <asm/current.h>
> > +#include <asm/page_types.h>
> > +#include <asm/pgtable_types.h>
>
> I found the latest mm-unstable fails build for arm64 and kunit (tenically
> speaking, UM) with errors including below.  And 'git bisect' points this patch.
>
> From arm64 build:
>       CC      mm/vma.o
>     In file included from /mm/vma.c:7:
>     /mm/vma_internal.h:46:10: fatal error: asm/page_types.h: No such file or directory
>        46 | #include <asm/page_types.h>
>           |          ^~~~~~~~~~~~~~~~~~
>     compilation terminated.
>
> From kunit build:
>
>     $ ./tools/testing/kunit/kunit.py build
>     [...]
>     $ make ARCH=um O=.kunit --jobs=36
>     ERROR:root:../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
>       156 | u64 ioread64_lo_hi(const void __iomem *addr)
>           |     ^~~~~~~~~~~~~~
>     ../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
>       163 | u64 ioread64_hi_lo(const void __iomem *addr)
>           |     ^~~~~~~~~~~~~~
>     ../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
>       170 | u64 ioread64be_lo_hi(const void __iomem *addr)
>           |     ^~~~~~~~~~~~~~~~
>     ../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
>       178 | u64 ioread64be_hi_lo(const void __iomem *addr)
>           |     ^~~~~~~~~~~~~~~~
>     ../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
>       264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
>           |      ^~~~~~~~~~~~~~~
>     ../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
>       272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
>           |      ^~~~~~~~~~~~~~~
>     ../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
>       280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
>           |      ^~~~~~~~~~~~~~~~~
>     ../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
>       288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
>           |      ^~~~~~~~~~~~~~~~~
>     In file included from ../mm/vma_internal.h:46,
>                      from ../mm/vma.c:7:
>
> Maybe the above two #include need to be removed or protected for some configs?
> I confirmed simply removing the two lines as below makes at least kunit, arm64,
> and my x86_64 builds happy, but would like to hear your thoughts.

Thanks, good spot!

Yeah they can just be dropped, this is pedantry from wanting to absolutely
nail down the sources of declarations, something I pared down in the final
release, but obviously these were unfortunately arch-specific.

You're right that they're just not needed.

I will send a -fix patch in a second.

>
> """
> diff --git a/mm/vma_internal.h b/mm/vma_internal.h
> index e13e5950df78..14c24d5cb582 100644
> --- a/mm/vma_internal.h
> +++ b/mm/vma_internal.h
> @@ -43,8 +43,6 @@
>  #include <linux/userfaultfd_k.h>
>
>  #include <asm/current.h>
> -#include <asm/page_types.h>
> -#include <asm/pgtable_types.h>
>  #include <asm/tlb.h>
>
>  #include "internal.h"
> """
>
>
> Thanks,
> SJ
>
> [...]