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

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

[jeffxu]

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.

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)
+{
+	if (flags)
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * mseal(2) seals the VM's meta data from
  * selected syscalls.
-- 
2.47.0.338.g60cca15819-goog

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

[Liam R. Howlett]

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

> 
> 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)
> +{
> +	if (flags)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  /*
>   * mseal(2) seals the VM's meta data from
>   * selected syscalls.
> -- 
> 2.47.0.338.g60cca15819-goog
> 

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

[Lorenzo Stoakes]

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

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

[Jeff Xu]

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

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

[Lorenzo Stoakes]

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

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

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

[Jeff Xu]

Hi Lorenzo,

Regarding your proposal of moving mseal.c to vma.c for unit testing.

On Fri, Dec 6, 2024 at 9:04 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
> > >
> > > 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
>
I want to express my support for unit testing and agree that more
testing would benefit mm. However, I'm unsure about the reasoning
behind moving code to vma.c in bulk. Could you please clarify this for
me?

In my understanding, unit tests can be conducted regardless of the
code's location once dependencies are addressed with stubs. Have you
considered adding mseal.c to the unittest makefile at the same level
as vma.c? Since mseal.c doesn't introduce new dependencies, i.e. it
operates directly on the vm_area_struct, so I would start with that.

I guess, for UT, you might need to change some functions' signatures,
e.g. remove static, if you want to test an internal function (e.g
mseal_fixup) , from your unit-test, but this is the same even after
moving them to vma.c.

There will be additional work of clean up including header (".h"),
still I believe this is the same work even after moving the code into
vma.c. You might still need to move the prototype of some functions
into vma.h or vma_internal.h (e.g. definition of MADV_FREE). But I
think this work is also orthogonal to where the mseal business logic
is located.

I understand the logic behind the current vma.c (on the linux_main
branch) and the unit test for the VMA merge functionality. However, if
your plan is to move all VMA-related code into vma.c, that means more
stubs are needed (depending on the boundary of the proposed unit
testing), and I don't understand how moving the code can help reduce
the amount of work or stubs (if that is the motivation).

To avoid spending too much of your time, if there are previous
discussions on this topic, please share links or a brief summary, so I
can study them first.

Thanks!
Best Regards,
-Jeff


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

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

[Lorenzo Stoakes]

On Tue, Dec 10, 2024 at 06:38:49PM -0800, Jeff Xu wrote:
> Hi Lorenzo,
>
> Regarding your proposal of moving mseal.c to vma.c for unit testing.
>
> On Fri, Dec 6, 2024 at 9:04 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> > > >
> > > > 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
> >
> I want to express my support for unit testing and agree that more
> testing would benefit mm. However, I'm unsure about the reasoning
> behind moving code to vma.c in bulk. Could you please clarify this for
> me?
>
> In my understanding, unit tests can be conducted regardless of the
> code's location once dependencies are addressed with stubs. Have you
> considered adding mseal.c to the unittest makefile at the same level
> as vma.c? Since mseal.c doesn't introduce new dependencies, i.e. it
> operates directly on the vm_area_struct, so I would start with that.

These aren't ordinary unit tests, this is a whole new structure to allow
for _userland_ unit tests, that is the ability to compile kernel code in
userland.

The mm/vma.c file has been specially set up to allow for this, it
outsources its imports to vma_internal.h, one which exists in mm/ for the
kernel and one which exists in tools/testing/vma for the userland unit
tests.

It also strictly means that vma.c is _internal_ only and _cannot_ be used
from any other part of the kernel except mm - it's a sealed environment
unto itself.

None of these things are true of mseal.c.

In any case it was something I was considering, if it makes sense to. You
can see VMA manipulation code in mprotect.c, etc. which may not actually
make a huge amount of sense to move over.

So this isn't for certain, and you'll be involved in any discussion if this
were to be done... :)

>
> I guess, for UT, you might need to change some functions' signatures,
> e.g. remove static, if you want to test an internal function (e.g
> mseal_fixup) , from your unit-test, but this is the same even after
> moving them to vma.c.
>
> There will be additional work of clean up including header (".h"),
> still I believe this is the same work even after moving the code into
> vma.c. You might still need to move the prototype of some functions
> into vma.h or vma_internal.h (e.g. definition of MADV_FREE). But I
> think this work is also orthogonal to where the mseal business logic
> is located.
>
> I understand the logic behind the current vma.c (on the linux_main
> branch) and the unit test for the VMA merge functionality. However, if
> your plan is to move all VMA-related code into vma.c, that means more
> stubs are needed (depending on the boundary of the proposed unit
> testing), and I don't understand how moving the code can help reduce
> the amount of work or stubs (if that is the motivation).

Yeah it isn't to move _all_ VMA-related code, because some don't make sense
there, but rather core VMA operations which make sense to be there and also
tested.

The possibilities are pretty exciting as to what we can do with this (ok
maybe only to me but still :P).

So again, it's far from certain I'll try to do this with mseal, it was just
a heads up ahead of time just in case I do.

I mean speciflcally speaking it'd be the very straightforward stuff about
applying mseal flags, checking compatibility etc.

>
> To avoid spending too much of your time, if there are previous
> discussions on this topic, please share links or a brief summary, so I
> can study them first.

Sure, I mean again the best thing is the original series [1]

[1]:https://lore.kernel.org/all/cover.1722251717.git.lorenzo.stoakes@oracle.com/

>
> Thanks!
> Best Regards,
> -Jeff
>
>
> > [0]https://lore.kernel.org/linux-mm/1c7a0b43cfad2c511a6b1b52f3507696478ff51a.1725040657.git.lorenzo.stoakes@oracle.com/
> >
> > Thanks, Lorenzo

This won't happen until next year at the earliest anyway, as I'm off for
Christmas/NY at the end of this week and this is nowhere near my TODO list
even at the moment :)

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

[Jeff Xu]

On Thu, Dec 5, 2024 at 8:25 PM Liam R. Howlett <Liam.Howlett@oracle.com> 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.
>
Sure

> In fact, having it separate from the comment about flags being reserved
> makes the function very puzzling.
>
> >
> > 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)
> > +{
> > +     if (flags)
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +
> >  /*
> >   * mseal(2) seals the VM's meta data from
> >   * selected syscalls.
> > --
> > 2.47.0.338.g60cca15819-goog
> >

[PATCH] mseal: remove can_do_mseal

[jeffxu]

From: Jeff Xu <jeffxu@google.com>

Hi Andrew,

Is that possible to squash this change on top of 
"mseal: move can_do_mseal to mseal.c" on the mm-unstable ?

This is the first time I m doing this, because I saw dev did this before,
and saying this is the prefered approach.

I could also send v2 which is what I usually do.

Thanks
-Jeff

Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 mm/mseal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/mseal.c b/mm/mseal.c
index e167220a0bf0..08e6f328d9c7 100644
--- a/mm/mseal.c
+++ b/mm/mseal.c
@@ -225,9 +225,9 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
 	unsigned long end;
 	struct mm_struct *mm = current->mm;
 
-	ret = can_do_mseal(flags);
-	if (ret)
-		return ret;
+	/* Verify flags not set. */
+	if (flags)
+		return -EINVAL;
 
 	start = untagged_addr(start);
 	if (!PAGE_ALIGNED(start))
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH] mseal: remove can_do_mseal

[Jeff Xu]

On Fri, Dec 6, 2024 at 11:33 AM <jeffxu@chromium.org> wrote:
>
> From: Jeff Xu <jeffxu@google.com>
>
> Hi Andrew,
>
> Is that possible to squash this change on top of
> "mseal: move can_do_mseal to mseal.c" on the mm-unstable ?
>
> This is the first time I m doing this, because I saw dev did this before,
> and saying this is the prefered approach.
>
> I could also send v2 which is what I usually do.
>
Actually, please ignore this, I will send a V2 instead.
(this didn't remove the code, sorry about that)

> Thanks
> -Jeff
>
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  mm/mseal.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index e167220a0bf0..08e6f328d9c7 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -225,9 +225,9 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
>         unsigned long end;
>         struct mm_struct *mm = current->mm;
>
> -       ret = can_do_mseal(flags);
> -       if (ret)
> -               return ret;
> +       /* Verify flags not set. */
> +       if (flags)
> +               return -EINVAL;
>
>         start = untagged_addr(start);
>         if (!PAGE_ALIGNED(start))
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH] mseal: remove can_do_mseal

[Lorenzo Stoakes]

On Fri, Dec 06, 2024 at 07:33:09PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
>
> Hi Andrew,
>
> Is that possible to squash this change on top of
> "mseal: move can_do_mseal to mseal.c" on the mm-unstable ?
>
> This is the first time I m doing this, because I saw dev did this before,
> and saying this is the prefered approach.
>
> I could also send v2 which is what I usually do.
>
> Thanks
> -Jeff
>
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>

Hi Jeff,

Fix-patches and the like are usually for stuff that's been taken and this, I
don't believe has (I might be wrong actually).

But anyway, as a reviewer it's easier for me to deal with a v2 in this case, I
am not going to give tags to the original version of this and want to make it
explicit I am only giving them to the corrected version.

So from my perspective could you do a favour and send as a v2 as it makes my
life easier anyway? :)

Thanks, Lorenzo

> ---
>  mm/mseal.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mseal.c b/mm/mseal.c
> index e167220a0bf0..08e6f328d9c7 100644
> --- a/mm/mseal.c
> +++ b/mm/mseal.c
> @@ -225,9 +225,9 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
>  	unsigned long end;
>  	struct mm_struct *mm = current->mm;
>
> -	ret = can_do_mseal(flags);
> -	if (ret)
> -		return ret;
> +	/* Verify flags not set. */
> +	if (flags)
> +		return -EINVAL;
>
>  	start = untagged_addr(start);
>  	if (!PAGE_ALIGNED(start))
> --
> 2.47.0.338.g60cca15819-goog
>

Re: [PATCH] mseal: remove can_do_mseal

[Jeff Xu]

On Fri, Dec 6, 2024 at 11:39 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Fri, Dec 06, 2024 at 07:33:09PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@google.com>
> >
> > Hi Andrew,
> >
> > Is that possible to squash this change on top of
> > "mseal: move can_do_mseal to mseal.c" on the mm-unstable ?
> >
> > This is the first time I m doing this, because I saw dev did this before,
> > and saying this is the prefered approach.
> >
> > I could also send v2 which is what I usually do.
> >
> > Thanks
> > -Jeff
> >
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
>
> Hi Jeff,
>
> Fix-patches and the like are usually for stuff that's been taken and this, I
> don't believe has (I might be wrong actually).
>
sure.

> But anyway, as a reviewer it's easier for me to deal with a v2 in this case, I
> am not going to give tags to the original version of this and want to make it
> explicit I am only giving them to the corrected version.
>
> So from my perspective could you do a favour and send as a v2 as it makes my
> life easier anyway? :)
>
sure.

> Thanks, Lorenzo
>
> > ---
> >  mm/mseal.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/mm/mseal.c b/mm/mseal.c
> > index e167220a0bf0..08e6f328d9c7 100644
> > --- a/mm/mseal.c
> > +++ b/mm/mseal.c
> > @@ -225,9 +225,9 @@ int do_mseal(unsigned long start, size_t len_in, unsigned long flags)
> >       unsigned long end;
> >       struct mm_struct *mm = current->mm;
> >
> > -     ret = can_do_mseal(flags);
> > -     if (ret)
> > -             return ret;
> > +     /* Verify flags not set. */
> > +     if (flags)
> > +             return -EINVAL;
> >
> >       start = untagged_addr(start);
> >       if (!PAGE_ALIGNED(start))
> > --
> > 2.47.0.338.g60cca15819-goog
> >