| Message ID | 20240828225522.684774-1-jeffxu@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] mseal: fix mmap(FIXED) error code. | expand |
+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)
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 >
* 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
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 > >
HI Andrew On Wed, Aug 28, 2024 at 3:55 PM <jeffxu@chromium.org> wrote: > > From: Jeff Xu <jeffxu@chromium.org> > > Add more testcases and increase test coverage, e.g. add > get_vma_size to check VMA size and prot bits. > Could you please pull the self-test part of this patch series to mm-unstable ? It will help to prevent regression. The first part ([PATCH v1 1/2] mseal: fix mmap(FIXED) error code) can be ignored as Liam proposed to fix it differently. Thanks -Jeff -Jeff > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > --- > tools/testing/selftests/mm/mseal_test.c | 826 ++++++++++++++++++++++-- > 1 file changed, 759 insertions(+), 67 deletions(-) > > diff --git a/tools/testing/selftests/mm/mseal_test.c b/tools/testing/selftests/mm/mseal_test.c > index e7991e5fdcf3..4b3f883aae17 100644 > --- a/tools/testing/selftests/mm/mseal_test.c > +++ b/tools/testing/selftests/mm/mseal_test.c
On Thu, Aug 29, 2024 at 05:16:50PM GMT, Mark Brown wrote: > On Wed, Aug 28, 2024 at 10:55:22PM +0000, jeffxu@chromium.org wrote: > > > Add more testcases and increase test coverage, e.g. add > > get_vma_size to check VMA size and prot bits. > > I think this needs to be split into multiple patches, for example the > new test cases done separately to the additions to the existing ones, > possibly as multiple patches (eg, should the size changes be split from > the prot ones?). Agree wholeheartedly.
Hi Mark On Thu, Aug 29, 2024 at 9:16 AM Mark Brown <broonie@kernel.org> wrote: > > On Wed, Aug 28, 2024 at 10:55:22PM +0000, jeffxu@chromium.org wrote: > > > Add more testcases and increase test coverage, e.g. add > > get_vma_size to check VMA size and prot bits. > > I think this needs to be split into multiple patches, for example the > new test cases done separately to the additions to the existing ones, > possibly as multiple patches (eg, should the size changes be split from > the prot ones?). The size check should go with prot., because it is part of verifying the sealed VMAs. I will split the patch into updating existing tests and adding new tests. Thanks -Jeff
Hi Lorenzo On Thu, Aug 29, 2024 at 8:44 AM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > > Also, this is a really unusual way to send a series - why is this a 2/2 in > > > reply to the 1/2 and no cover letter? Why is this change totally unrelated > > > to the other patch? > > > 1/2 has a fix that 2/2 is depending on. That is the reason they are together. > > > Can you send this as a separate patch, preferably as an RFC so we can > > > ensure that we all agree on how mseal() should behave? > > > It is not an RFC because it doesn't change any semanic to mseal. The updated test will pass on linux main as well as 6.10. The increased coverage will help to prevent future regression, i.e. during refactoring. I will add a cover letter, split the tests and add more comments to help the review. Thanks -Jeff
On Thu, Aug 29, 2024 at 12:54:09PM -0700, Jeff Xu wrote: > Hi Lorenzo > > On Thu, Aug 29, 2024 at 8:44 AM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > > > > > > Also, this is a really unusual way to send a series - why is this a 2/2 in > > > > reply to the 1/2 and no cover letter? Why is this change totally unrelated > > > > to the other patch? > > > > > 1/2 has a fix that 2/2 is depending on. That is the reason they are together. The normal way to send out these patches is as three emails; a 0/2 cover letter, 1/2 replying to 0/2 and 2/2 also replying to 0/2. That's what has Lorenzo confused. > > > > Can you send this as a separate patch, preferably as an RFC so we can > > > > ensure that we all agree on how mseal() should behave? > > > > > It is not an RFC because it doesn't change any semanic to mseal. The > updated test will pass on linux main as well as 6.10. The increased > coverage will help to prevent future regression, i.e. during > refactoring. You seem to not understand that there is disagreement on the semantics of mseal(). I mean, ther's been a lot of arguing about that over the last week. There's understanable reluctance to accept a large pile of tests saying "this just ensures that mseal behaves the way I think it should", when there is substantial disagreement that the way you think it should behave is in fact the way it should behave. Be prepared to argue for each semantic that you think it should have. > I will add a cover letter, split the tests and add more comments to > help the review. Thank you.
Hi Matthew On Thu, Aug 29, 2024 at 12:58 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > Can you send this as a separate patch, preferably as an RFC so we can > > > > > ensure that we all agree on how mseal() should behave? > > > > > > > It is not an RFC because it doesn't change any semanic to mseal. The > > updated test will pass on linux main as well as 6.10. The increased > > coverage will help to prevent future regression, i.e. during > > refactoring. > > You seem to not understand that there is disagreement on the semantics > of mseal(). I mean, ther's been a lot of arguing about that over the > last week. There's understanable reluctance to accept a large pile of > tests saying "this just ensures that mseal behaves the way I think it > should", when there is substantial disagreement that the way you think > it should behave is in fact the way it should behave. Be prepared to > argue for each semantic that you think it should have. > If this is about in-loop discussion, this patch also passes the latest mm-unstable branch which has in-loop change (pending Liam's fix on mmap). The increased test coverage also helps to ensure the in-loop change's correctness on sealing. I'm not aware there are other semantic changes on mseal, we can continue this discussion on V2 patch, if necessary. Thanks -Jeff
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;