Message-ID: <f4c704bc-3b9a-48bb-b1dc-8a73d181223c@lucifer.local>
Date: Fri, 6 Dec 2024 17:04:47 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: "Liam R. Howlett" <Liam.Howlett@...cle.com>, akpm@...ux-foundation.org,
        vbabka@...e.cz, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org, linux-mm@...ck.org,
        jorgelo@...omium.org, keescook@...omium.org, pedro.falcato@...il.com,
        rdunlap@...radead.org
Subject: Re: [PATCH v1] mseal: move can_do_mseal to mseal.c

On Fri, Dec 06, 2024 at 08:17:40AM -0800, Jeff Xu wrote:
> On Fri, Dec 6, 2024 at 1:13 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > On Thu, Dec 05, 2024 at 11:25:43PM -0500, Liam R. Howlett wrote:
> > > * jeffxu@...omium.org <jeffxu@...omium.org> [241205 20:39]:
> > > > From: Jeff Xu <jeffxu@...omium.org>
> > > >
> > > > No code logic change.
> > > >
> > > > can_do_mseal is called exclusively by mseal.c,
> > > > and mseal.c is compiled only when CONFIG_64BIT flag is
> > > > set in makefile. Therefore, it is unnecessary to have
> > > > 32 bit stub function in the header file.
> > >
> > > There is no reason to keep this function at all; it is used in one
> > > place, and that place uses three lines of code as well.
> > >
> > > In fact, having it separate from the comment about flags being reserved
> > > makes the function very puzzling.
> >
> > I entirely agree. Jeff - please just make this inline to do_mseal():
> >
> Sure.

Thanks, appreciate it!

>
> >         ...
> >
> >         /* Flags are reserved. */
> >         if (flags)
> >                 retrun -EINVAL;
> >
> >         ...
> >
> > If you do that then cool I'm happy for this patch to be taken.
> >
> > An aside - I actually think we need to move the bulk of this code to
> > mm/vma.c - it makes absolutely no sense to keep the internals in this file,
> > and that way we can userland test mseal functionality.
> >
> Is there a past discussion to read ? That will help me understand your
> strategy of unit testing mm code.
> Moving everything to vma.c, will lose log history, e.g. blame no
> longer helps, did we consider alternatives ?

Re; git blame - I'm not sure what alternative you think exists, and I've
moved brk(), mmap(), etc. with a history spanning >30 years, so I'm not
sure what blame history you're concerned about given how recent mseal is :)

There is always code that gets moved or changed. You can't stay attached to
your name appearing on a git blame line.

Re: discussion, there's dozens of discussions and patch sets totalling ~3k
lines of code... just search lore for vma testing, or search through my
commits in mm/vma.c and you can see.

I can put together links if you really need, but I think say [0] is a good
motivating example of how I was able to actually write unit tests for VMA
merge functionality which previously could not exist.

In any case you can use the git blame -C option to 'see through' things like
code moves.

The whole point of this is to be able to _unit_ test functionality under
circumstances that might be otherwise improbable/incredibly difficult to
obtain if run as part of a kernel and self testing.

Importantly it allows us to conduct fuzzing testing in future, something
key and fundamental to security testing.

I would say for somebody who has clearly stated his huge commitment to
testing and how critically vital it is especially in the security realm,
this is entirely something that is beneficial to the kernel and to mseal
stability and security.

If you want to see it 'in action', you can run the tests in
tools/testing/vma via:

$ make && ./vma

[0]https://lore.kernel.org/linux-mm/1c7a0b43cfad2c511a6b1b52f3507696478ff51a.1725040657.git.lorenzo.stoakes@oracle.com/

Thanks, Lorenzo

>
>
> > I may submit a patch to this effect at some point.
> >
> > Thanks, Lorenzo
> >
> > >
> > > >
> > > > Signed-off-by: Jeff Xu <jeffxu@...omium.org>
> > > > ---
> > > >  mm/internal.h | 16 ----------------
> > > >  mm/mseal.c    |  8 ++++++++
> > > >  2 files changed, 8 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/mm/internal.h b/mm/internal.h
> > > > index 74dc1c48fa31..5e4ef5ce9c0a 100644
> > > > --- a/mm/internal.h
> > > > +++ b/mm/internal.h
> > > > @@ -1457,22 +1457,6 @@ void __meminit __init_single_page(struct page *page, unsigned long pfn,
> > > >  unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg,
> > > >                       int priority);
> > > >
> > > > -#ifdef CONFIG_64BIT
> > > > -static inline int can_do_mseal(unsigned long flags)
> > > > -{
> > > > -   if (flags)
> > > > -           return -EINVAL;
> > > > -
> > > > -   return 0;
> > > > -}
> > > > -
> > > > -#else
> > > > -static inline int can_do_mseal(unsigned long flags)
> > > > -{
> > > > -   return -EPERM;
> > > > -}
> > > > -#endif
> > > > -
> > > >  #ifdef CONFIG_SHRINKER_DEBUG
> > > >  static inline __printf(2, 0) int shrinker_debugfs_name_alloc(
> > > >                     struct shrinker *shrinker, const char *fmt, va_list ap)
> > > > diff --git a/mm/mseal.c b/mm/mseal.c
> > > > index 81d6e980e8a9..e167220a0bf0 100644
> > > > --- a/mm/mseal.c
> > > > +++ b/mm/mseal.c
> > > > @@ -158,6 +158,14 @@ static int apply_mm_seal(unsigned long start, unsigned long end)
> > > >     return 0;
> > > >  }
> > > >
> > > > +static inline int can_do_mseal(unsigned long flags)
> >
> > It makes no sense for this to be inline.
> >
> > > > +{
> > > > +   if (flags)
> > > > +           return -EINVAL;
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > >  /*
> > > >   * mseal(2) seals the VM's meta data from
> > > >   * selected syscalls.
> > > > --
> > > > 2.47.0.338.g60cca15819-goog
> > > >

Powered by blists - more mailing lists