Re: [PATCH v1] mseal: move can_do_mseal to mseal.c

[Jeff Xu <jeffxu@chromium.org>]

On Fri, Dec 6, 2024 at 1:13 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Dec 05, 2024 at 11:25:43PM -0500, Liam R. Howlett wrote:
> > * jeffxu@chromium.org <jeffxu@chromium.org> [241205 20:39]:
> > > From: Jeff Xu <jeffxu@chromium.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.

>         ...
>
>         /* 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 ?


> I may submit a patch to this effect at some point.
>
> Thanks, Lorenzo
>
> >
> > >
> > > Signed-off-by: Jeff Xu <jeffxu@chromium.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
> > >