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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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():

	...

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

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
> >