diff mbox series

[v1,1/2] mseal: fix mmap(FIXED) error code.

Message ID 20240828225522.684774-1-jeffxu@chromium.org (mailing list archive)
State New
Headers show
Series [v1,1/2] mseal: fix mmap(FIXED) error code. | expand

Commit Message

Jeff Xu Aug. 28, 2024, 10:55 p.m. UTC
From: Jeff Xu <jeffxu@chromium.org>

mmap(MAP_FIXED) should return EPERM when memory is sealed.

Fixes: 4205a39e06da ("mm/munmap: replace can_modify_mm with can_modify_vma")
Signed-off-by: Jeff Xu <jeffxu@chromium.org>
---
 mm/mmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Pedro Falcato Aug. 28, 2024, 11:38 p.m. UTC | #1
+CC vma reviewers
On Wed, Aug 28, 2024 at 10:55:21PM GMT, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
> 
> mmap(MAP_FIXED) should return EPERM when memory is sealed.
> 
> Fixes: 4205a39e06da ("mm/munmap: replace can_modify_mm with can_modify_vma")

Thank you for the patch!
This Fixes: is wrong, the bug was added during Liam's rebasing of his munmap patch
set on mine.

> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  mm/mmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 80d70ed099cf..0cd0c0ef03c7 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1386,7 +1386,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		mt_on_stack(mt_detach);
>  		mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
>  		/* Prepare to unmap any existing mapping in the area */
> -		if (vms_gather_munmap_vmas(&vms, &mas_detach))
> +		error = vms_gather_munmap_vmas(&vms, &mas_detach);
> +		if (error == -EPERM)
> +			return -EPERM;

Not sure if it makes sense to special case this. We should probably deal with this inside
vms_gather_munmap_vmas and just pass through the error we get.

Otherwise LGTM. Liam?

(we should also squash this into the offending commit)
Lorenzo Stoakes Aug. 29, 2024, 12:09 p.m. UTC | #2
Jeff... come on now.

Please cc- the reviewers of mm/mmap.c on these patches - that's me,
Vlastimil and Liam. Same for mm/vma.c, mm/vma.h, mm/vma_internal.h.

And it seems like it should be pretty obvious you should cc- Liam when it's
quite literally his code you're changing!

Relevant section from MAINTAINERS:

MEMORY MAPPING
M:	Andrew Morton <akpm@linux-foundation.org>
R:	Liam R. Howlett <Liam.Howlett@oracle.com>
R:	Vlastimil Babka <vbabka@suse.cz>
R:	Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
L:	linux-mm@kvack.org
S:	Maintained
W:	http://www.linux-mm.org
T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
F:	mm/mmap.c

On Wed, Aug 28, 2024 at 10:55:21PM GMT, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
>
> mmap(MAP_FIXED) should return EPERM when memory is sealed.
>
> Fixes: 4205a39e06da ("mm/munmap: replace can_modify_mm with can_modify_vma")
> Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> ---
>  mm/mmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 80d70ed099cf..0cd0c0ef03c7 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1386,7 +1386,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		mt_on_stack(mt_detach);
>  		mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
>  		/* Prepare to unmap any existing mapping in the area */
> -		if (vms_gather_munmap_vmas(&vms, &mas_detach))
> +		error = vms_gather_munmap_vmas(&vms, &mas_detach);
> +		if (error == -EPERM)
> +			return -EPERM;
> +		if (error)
>  			return -ENOMEM;

Can't we just return the error here?

This is one for Liam, but I'm ostensibly in favour, this does seem valid!

>
>  		vmg.next = vms.next;
> --
> 2.46.0.295.g3b9ea8a38a-goog
>
Liam R. Howlett Aug. 29, 2024, 2:03 p.m. UTC | #3
* Pedro Falcato <pedro.falcato@gmail.com> [240828 19:38]:
> +CC vma reviewers
> On Wed, Aug 28, 2024 at 10:55:21PM GMT, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> > 
> > mmap(MAP_FIXED) should return EPERM when memory is sealed.

Thanks for the fix and finding the issue.  Please email the maintainers
of the file as well as the patch author next time.

> > 
> > Fixes: 4205a39e06da ("mm/munmap: replace can_modify_mm with can_modify_vma")
> 
> Thank you for the patch!
> This Fixes: is wrong, the bug was added during Liam's rebasing of his munmap patch
> set on mine.

Right now, the akpm/mm-unstable git id of the patch this needs to squash
into is 5887a7ac23836.  Although, this will leave intermittent patches
to return the incorrect error code.

Initially it was introduced in commit c2eb22189bbc9, so I'd like to fix
this in the series so that it doesn't show up in any bisection.

> 
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  mm/mmap.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 80d70ed099cf..0cd0c0ef03c7 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1386,7 +1386,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  		mt_on_stack(mt_detach);
> >  		mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
> >  		/* Prepare to unmap any existing mapping in the area */
> > -		if (vms_gather_munmap_vmas(&vms, &mas_detach))
> > +		error = vms_gather_munmap_vmas(&vms, &mas_detach);
> > +		if (error == -EPERM)
> > +			return -EPERM;
> 
> Not sure if it makes sense to special case this. We should probably deal with this inside
> vms_gather_munmap_vmas and just pass through the error we get.
> 
> Otherwise LGTM. Liam?
> 
> (we should also squash this into the offending commit)

All code paths that exist today in vms_gather_munmap_vmas() can only
return -EPERM and -ENOMEM. So filtering isn't really necessary right
now.  But then again, vms_gather_munmap_vmas() is only used in two
places and this filters one return, but not the other.

I think it best to address this in vms_gather_munmap_vmas() to only
return -ENOMEM or -EPERM.

I will fix this in my series, thanks Jeff.

Regards,
Liam
Jeff Xu Aug. 29, 2024, 2:32 p.m. UTC | #4
On Wed, Aug 28, 2024 at 4:38 PM Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> +CC vma reviewers
> On Wed, Aug 28, 2024 at 10:55:21PM GMT, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > mmap(MAP_FIXED) should return EPERM when memory is sealed.
> >
> > Fixes: 4205a39e06da ("mm/munmap: replace can_modify_mm with can_modify_vma")
>
> Thank you for the patch!
> This Fixes: is wrong, the bug was added during Liam's rebasing of his munmap patch
> set on mine.
>
ok.

> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  mm/mmap.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 80d70ed099cf..0cd0c0ef03c7 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1386,7 +1386,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >               mt_on_stack(mt_detach);
> >               mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
> >               /* Prepare to unmap any existing mapping in the area */
> > -             if (vms_gather_munmap_vmas(&vms, &mas_detach))
> > +             error = vms_gather_munmap_vmas(&vms, &mas_detach);
> > +             if (error == -EPERM)
> > +                     return -EPERM;
>
> Not sure if it makes sense to special case this. We should probably deal with this inside
> vms_gather_munmap_vmas and just pass through the error we get.
>
> Otherwise LGTM. Liam?
>
> (we should also squash this into the offending commit)
>
> --
> Pedro
Jeff Xu Aug. 29, 2024, 2:34 p.m. UTC | #5
On Thu, Aug 29, 2024 at 5:09 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> Jeff... come on now.
>
> Please cc- the reviewers of mm/mmap.c on these patches - that's me,
> Vlastimil and Liam. Same for mm/vma.c, mm/vma.h, mm/vma_internal.h.
>
sure, that was a small fix and I thought introduced by Pedro's commit
(which was wrong)

> And it seems like it should be pretty obvious you should cc- Liam when it's
> quite literally his code you're changing!
>
> Relevant section from MAINTAINERS:
>
> MEMORY MAPPING
> M:      Andrew Morton <akpm@linux-foundation.org>
> R:      Liam R. Howlett <Liam.Howlett@oracle.com>
> R:      Vlastimil Babka <vbabka@suse.cz>
> R:      Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> L:      linux-mm@kvack.org
> S:      Maintained
> W:      http://www.linux-mm.org
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> F:      mm/mmap.c
>
> On Wed, Aug 28, 2024 at 10:55:21PM GMT, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > mmap(MAP_FIXED) should return EPERM when memory is sealed.
> >
> > Fixes: 4205a39e06da ("mm/munmap: replace can_modify_mm with can_modify_vma")
> > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > ---
> >  mm/mmap.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 80d70ed099cf..0cd0c0ef03c7 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1386,7 +1386,10 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >               mt_on_stack(mt_detach);
> >               mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
> >               /* Prepare to unmap any existing mapping in the area */
> > -             if (vms_gather_munmap_vmas(&vms, &mas_detach))
> > +             error = vms_gather_munmap_vmas(&vms, &mas_detach);
> > +             if (error == -EPERM)
> > +                     return -EPERM;
> > +             if (error)
> >                       return -ENOMEM;
>
> Can't we just return the error here?
>
> This is one for Liam, but I'm ostensibly in favour, this does seem valid!
>
> >
> >               vmg.next = vms.next;
> > --
> > 2.46.0.295.g3b9ea8a38a-goog
> >
Jeff Xu Aug. 29, 2024, 2:36 p.m. UTC | #6
On Thu, Aug 29, 2024 at 7:03 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> I will fix this in my series, thanks Jeff.
>
Sure.

-Jeff

> Regards,
> Liam
Andrew Morton Aug. 31, 2024, 1:15 a.m. UTC | #7
On Thu, 29 Aug 2024 13:09:41 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> Relevant section from MAINTAINERS:
> 
> MEMORY MAPPING

I always thought it meant "memory management" ;)
Lorenzo Stoakes Aug. 31, 2024, 10:14 a.m. UTC | #8
On Fri, Aug 30, 2024 at 06:15:46PM GMT, Andrew Morton wrote:
> On Thu, 29 Aug 2024 13:09:41 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> > Relevant section from MAINTAINERS:
> >
> > MEMORY MAPPING
>
> I always thought it meant "memory management" ;)

Ha ha no, I leave the managing to you, we just map and pray! ;)
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 80d70ed099cf..0cd0c0ef03c7 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1386,7 +1386,10 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 		mt_on_stack(mt_detach);
 		mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
 		/* Prepare to unmap any existing mapping in the area */
-		if (vms_gather_munmap_vmas(&vms, &mas_detach))
+		error = vms_gather_munmap_vmas(&vms, &mas_detach);
+		if (error == -EPERM)
+			return -EPERM;
+		if (error)
 			return -ENOMEM;
 
 		vmg.next = vms.next;