[v4,27/66] mm/mmap: Change do_brk_munmap() to use do_mas_align_munmap()

Commit Message

Liam R. Howlett Dec. 1, 2021, 2:30 p.m. UTC

From: "Liam R. Howlett" <[e-mail]>

do_brk_munmap() has already aligned the address and has a maple tree
state to be used.  Use the new do_mas_align_munmap() to avoid
unnecessary alignment and error checks.

Signed-off-by: Liam R. Howlett <[e-mail]>
---
 mm/mmap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Vlastimil Babka Jan. 18, 2022, 11:57 a.m. UTC | #1

On 12/1/21 15:30, Liam Howlett wrote:
> From: "Liam R. Howlett" <[e-mail]>
> 
> do_brk_munmap() has already aligned the address and has a maple tree
> state to be used.  Use the new do_mas_align_munmap() to avoid
> unnecessary alignment and error checks.
> 
> Signed-off-by: Liam R. Howlett <[e-mail]>
> ---
>  mm/mmap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 14190306a483..79b8494d83c6 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2926,12 +2926,15 @@ static int do_brk_munmap(struct ma_state *mas, struct vm_area_struct *vma,
>  	struct mm_struct *mm = vma->vm_mm;
>  	struct vm_area_struct unmap;
>  	unsigned long unmap_pages;
> -	int ret = 1;
> +	int ret;
>  
>  	arch_unmap(mm, newbrk, oldbrk);
>  
> -	if (likely(vma->vm_start >= newbrk)) { // remove entire mapping(s)
> -		ret = do_mas_munmap(mas, mm, newbrk, oldbrk-newbrk, uf, true);
> +	if (likely((vma->vm_end < oldbrk) ||
> +		   ((vma->vm_start == newbrk) && (vma->vm_end == oldbrk)))) {

Can you describe this change of conditions in the commit log as well?

> +		// remove entire mapping(s)
> +		ret = do_mas_align_munmap(mas, vma, mm, newbrk, oldbrk, uf,
> +					  true);
>  		goto munmap_full_vma;
>  	}
>

Liam R. Howlett Jan. 22, 2022, 1:53 a.m. UTC | #2

* Vlastimil Babka <[e-mail]> [220118 06:57]:
> On 12/1/21 15:30, Liam Howlett wrote:
> > From: "Liam R. Howlett" <[e-mail]>
> > 
> > do_brk_munmap() has already aligned the address and has a maple tree
> > state to be used.  Use the new do_mas_align_munmap() to avoid
> > unnecessary alignment and error checks.
> > 
> > Signed-off-by: Liam R. Howlett <[e-mail]>
> > ---
> >  mm/mmap.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 14190306a483..79b8494d83c6 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2926,12 +2926,15 @@ static int do_brk_munmap(struct ma_state *mas, struct vm_area_struct *vma,
> >  	struct mm_struct *mm = vma->vm_mm;
> >  	struct vm_area_struct unmap;
> >  	unsigned long unmap_pages;
> > -	int ret = 1;
> > +	int ret;
> >  
> >  	arch_unmap(mm, newbrk, oldbrk);
> >  
> > -	if (likely(vma->vm_start >= newbrk)) { // remove entire mapping(s)
> > -		ret = do_mas_munmap(mas, mm, newbrk, oldbrk-newbrk, uf, true);
> > +	if (likely((vma->vm_end < oldbrk) ||
> > +		   ((vma->vm_start == newbrk) && (vma->vm_end == oldbrk)))) {
> 
> Can you describe this change of conditions in the commit log as well?

Yes, that's fair as it's not easy to see.  The second part is to catch
an exact match of the vma - simple enough.  The first part however, was
changed to catch a tricky mprotect scenario that can land us into a VMA
which is located at a lower address than the end of the brk.  So we know
we have to at least munmap one entire VMA.  Checking the start of the
VMA against the newbrk may not catch this scenario.

And now that I've explained it, I think this change should be backported
to the earlier patch in the series:
mm/mmap: Change do_brk_flags() to expand existing VMA and add do_brk_munmap()

> 
> > +		// remove entire mapping(s)
> > +		ret = do_mas_align_munmap(mas, vma, mm, newbrk, oldbrk, uf,
> > +					  true);
> >  		goto munmap_full_vma;
> >  	}
> >  
>

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 14190306a483..79b8494d83c6 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2926,12 +2926,15 @@  static int do_brk_munmap(struct ma_state *mas, struct vm_area_struct *vma,
 	struct mm_struct *mm = vma->vm_mm;
 	struct vm_area_struct unmap;
 	unsigned long unmap_pages;
-	int ret = 1;
+	int ret;
 
 	arch_unmap(mm, newbrk, oldbrk);
 
-	if (likely(vma->vm_start >= newbrk)) { // remove entire mapping(s)
-		ret = do_mas_munmap(mas, mm, newbrk, oldbrk-newbrk, uf, true);
+	if (likely((vma->vm_end < oldbrk) ||
+		   ((vma->vm_start == newbrk) && (vma->vm_end == oldbrk)))) {
+		// remove entire mapping(s)
+		ret = do_mas_align_munmap(mas, vma, mm, newbrk, oldbrk, uf,
+					  true);
 		goto munmap_full_vma;
 	}