diff mbox series

[v5,18/21] mm/mmap: Move can_modify_mm() check down the stack

Message ID 20240717200709.1552558-19-Liam.Howlett@oracle.com (mailing list archive)
State New
Headers show
Series Avoid MAP_FIXED gap exposure | expand

Commit Message

Liam R. Howlett July 17, 2024, 8:07 p.m. UTC 
From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>

Without an arch_unmap() call so high in the call stack, the check for
mseal'ed vmas can be moved lower as well.  This has the benefit of only
actually checking if things are msealed when there is anything to check.
That is, we know there is at least one vma that is in the way and needs
to be checked.

Only call the can_modify_mm() in do_vmi_align_munmap() and the MAP_FIXED
case of mmap_region().

Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
Cc: Jeff Xu <jeffxu@chromium.org>
---
 mm/mmap.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

Comments

Lorenzo Stoakes July 22, 2024, 2:41 p.m. UTC | #1 
On Wed, Jul 17, 2024 at 04:07:06PM GMT, Liam R. Howlett wrote:
> From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
>
> Without an arch_unmap() call so high in the call stack, the check for
> mseal'ed vmas can be moved lower as well.  This has the benefit of only
> actually checking if things are msealed when there is anything to check.
> That is, we know there is at least one vma that is in the way and needs
> to be checked.
>
> Only call the can_modify_mm() in do_vmi_align_munmap() and the MAP_FIXED
> case of mmap_region().
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> Cc: Jeff Xu <jeffxu@chromium.org>
> ---
>  mm/mmap.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 117e8240f697..a32f545d3987 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2877,6 +2877,10 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	struct vma_munmap_struct vms;
>  	int error;
>
> +	/* Prevent unmapping a sealed VMA. */
> +	if (unlikely(!can_modify_mm(mm, start, end)))
> +		return -EPERM;
> +
>  	init_vma_munmap(&vms, vmi, vma, start, end, uf, unlock);
>  	error = vms_gather_munmap_vmas(&vms, &mas_detach);
>  	if (error)
> @@ -2927,13 +2931,6 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
>  	if (end == start)
>  		return -EINVAL;
>
> -	/*
> -	 * Prevent unmapping a sealed VMA.
> -	 * can_modify_mm assumes we have acquired the lock on MM.
> -	 */
> -	if (unlikely(!can_modify_mm(mm, start, end)))
> -		return -EPERM;
> -
>  	/* Find the first overlapping VMA */
>  	vma = vma_find(vmi, end);
>  	if (!vma) {
> @@ -2991,13 +2988,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
>  		return -ENOMEM;
>
> -	if (unlikely(!can_modify_mm(mm, addr, end)))
> -		return -EPERM;
>
>  	/* Find the first overlapping VMA */
>  	vma = vma_find(&vmi, end);
>  	init_vma_munmap(&vms, &vmi, vma, addr, end, uf, /* unlock = */ false);
>  	if (vma) {
> +		/* Prevent unmapping a sealed VMA. */
> +		if (unlikely(!can_modify_mm(mm, addr, end)))
> +			return -EPERM;
> +
>  		mt_init_flags(&mt_detach, vmi.mas.tree->ma_flags & MT_FLAGS_LOCK_MASK);
>  		mt_on_stack(mt_detach);
>  		mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
> @@ -3370,13 +3369,6 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  {
>  	struct mm_struct *mm = vma->vm_mm;
>
> -	/*
> -	 * Prevent unmapping a sealed VMA.
> -	 * can_modify_mm assumes we have acquired the lock on MM.
> -	 */
> -	if (unlikely(!can_modify_mm(mm, start, end)))
> -		return -EPERM;
> -
>  	return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
>  }
>
> --
> 2.43.0
>

LGTM,

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Jeff Xu July 24, 2024, 4:29 p.m. UTC | #2 
On Wed, Jul 17, 2024 at 1:07 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
>
> Without an arch_unmap() call so high in the call stack, the check for
> mseal'ed vmas can be moved lower as well.  This has the benefit of only
> actually checking if things are msealed when there is anything to check.
> That is, we know there is at least one vma that is in the way and needs
> to be checked.
>
> Only call the can_modify_mm() in do_vmi_align_munmap() and the MAP_FIXED
> case of mmap_region().
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> Cc: Jeff Xu <jeffxu@chromium.org>
Reviewed-by: Jeff Xu <jeffxu@chromium.org>

> ---
>  mm/mmap.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 117e8240f697..a32f545d3987 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2877,6 +2877,10 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>         struct vma_munmap_struct vms;
>         int error;
>
> +       /* Prevent unmapping a sealed VMA. */
> +       if (unlikely(!can_modify_mm(mm, start, end)))
> +               return -EPERM;
> +
It is nice to consolidate the check for do_vmi_align_munmap and
do_vma_munmap to single check.

>         init_vma_munmap(&vms, vmi, vma, start, end, uf, unlock);
>         error = vms_gather_munmap_vmas(&vms, &mas_detach);
>         if (error)
> @@ -2927,13 +2931,6 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
>         if (end == start)
>                 return -EINVAL;
>
> -       /*
> -        * Prevent unmapping a sealed VMA.
> -        * can_modify_mm assumes we have acquired the lock on MM.
> -        */
> -       if (unlikely(!can_modify_mm(mm, start, end)))
> -               return -EPERM;
> -
>         /* Find the first overlapping VMA */
>         vma = vma_find(vmi, end);
>         if (!vma) {
> @@ -2991,13 +2988,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>         if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
>                 return -ENOMEM;
>
> -       if (unlikely(!can_modify_mm(mm, addr, end)))
> -               return -EPERM;
>
>         /* Find the first overlapping VMA */
>         vma = vma_find(&vmi, end);
>         init_vma_munmap(&vms, &vmi, vma, addr, end, uf, /* unlock = */ false);
>         if (vma) {
> +               /* Prevent unmapping a sealed VMA. */
> +               if (unlikely(!can_modify_mm(mm, addr, end)))
> +                       return -EPERM;
> +
So the optimization here is : when no vma  found in the given addr
range => no need to call can_modify_mm.

>                 mt_init_flags(&mt_detach, vmi.mas.tree->ma_flags & MT_FLAGS_LOCK_MASK);
>                 mt_on_stack(mt_detach);
>                 mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
> @@ -3370,13 +3369,6 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  {
>         struct mm_struct *mm = vma->vm_mm;
>
> -       /*
> -        * Prevent unmapping a sealed VMA.
> -        * can_modify_mm assumes we have acquired the lock on MM.
> -        */
> -       if (unlikely(!can_modify_mm(mm, start, end)))
> -               return -EPERM;
> -
>         return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
>  }
>
> --
> 2.43.0
>
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 117e8240f697..a32f545d3987 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2877,6 +2877,10 @@  do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	struct vma_munmap_struct vms;
 	int error;
 
+	/* Prevent unmapping a sealed VMA. */
+	if (unlikely(!can_modify_mm(mm, start, end)))
+		return -EPERM;
+
 	init_vma_munmap(&vms, vmi, vma, start, end, uf, unlock);
 	error = vms_gather_munmap_vmas(&vms, &mas_detach);
 	if (error)
@@ -2927,13 +2931,6 @@  int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
 	if (end == start)
 		return -EINVAL;
 
-	/*
-	 * Prevent unmapping a sealed VMA.
-	 * can_modify_mm assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm(mm, start, end)))
-		return -EPERM;
-
 	/* Find the first overlapping VMA */
 	vma = vma_find(vmi, end);
 	if (!vma) {
@@ -2991,13 +2988,15 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 	if (!may_expand_vm(mm, vm_flags, pglen - nr_pages))
 		return -ENOMEM;
 
-	if (unlikely(!can_modify_mm(mm, addr, end)))
-		return -EPERM;
 
 	/* Find the first overlapping VMA */
 	vma = vma_find(&vmi, end);
 	init_vma_munmap(&vms, &vmi, vma, addr, end, uf, /* unlock = */ false);
 	if (vma) {
+		/* Prevent unmapping a sealed VMA. */
+		if (unlikely(!can_modify_mm(mm, addr, end)))
+			return -EPERM;
+
 		mt_init_flags(&mt_detach, vmi.mas.tree->ma_flags & MT_FLAGS_LOCK_MASK);
 		mt_on_stack(mt_detach);
 		mas_init(&mas_detach, &mt_detach, /* addr = */ 0);
@@ -3370,13 +3369,6 @@  int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
 {
 	struct mm_struct *mm = vma->vm_mm;
 
-	/*
-	 * Prevent unmapping a sealed VMA.
-	 * can_modify_mm assumes we have acquired the lock on MM.
-	 */
-	if (unlikely(!can_modify_mm(mm, start, end)))
-		return -EPERM;
-
 	return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
 }