| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <256bb98e-4bc8-f094-5ecb-500827fbbfa5@suse.cz>
Date: Wed, 22 Mar 2023 10:28:11 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Lorenzo Stoakes <lstoakes@...il.com>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>
Cc: David Hildenbrand <david@...hat.com>,
Matthew Wilcox <willy@...radead.org>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
maple-tree@...ts.infradead.org, Vernon Yang <vernon2gm@...il.com>
Subject: Re: [PATCH v3 4/4] mm/mmap/vma_merge: init cleanup, be explicit about
the non-mergeable case
On 3/22/23 08:13, Lorenzo Stoakes wrote:
> Rather than setting err = -1 and only resetting if we hit merge cases,
> explicitly check the non-mergeable case to make it abundantly clear that we
> only proceed with the rest if something is mergeable, default err to 0 and
> only update if an error might occur.
>
> Move the merge_prev, merge_next cases closer to the logic determining curr,
> next and reorder initial variables so they are more logically grouped.
>
> This has no functional impact.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@...il.com>
Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
Some nits:
> ---
> mm/mmap.c | 57 ++++++++++++++++++++++++++++---------------------------
> 1 file changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 2a4f63716231..642f3d063be1 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -909,18 +909,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> struct vm_userfaultfd_ctx vm_userfaultfd_ctx,
> struct anon_vma_name *anon_name)
> {
> - pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
> - pgoff_t vma_pgoff;
> struct vm_area_struct *curr, *next, *res;
> struct vm_area_struct *vma, *adjust, *remove, *remove2;
> - int err = -1;
> + struct vma_prepare vp;
> + pgoff_t vma_pgoff;
> + int err = 0;
> bool merge_prev = false;
> bool merge_next = false;
> bool vma_expanded = false;
> - struct vma_prepare vp;
> + unsigned long vma_start = addr;
> unsigned long vma_end = end;
> + pgoff_t pglen = (end - addr) >> PAGE_SHIFT;
> long adj_start = 0;
> - unsigned long vma_start = addr;
>
> validate_mm(mm);
> /*
> @@ -939,36 +939,38 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> else
> next = NULL; /* case 5 */
>
> - /*
> - * By default, we return prev. Cases 3, 4, 8 will instead return next
> - * and cases 3, 8 will also update vma to point at next.
> - */
> - res = vma = prev;
> -
> - /* Verify some invariant that must be enforced by the caller. */
> - VM_WARN_ON(prev && addr <= prev->vm_start);
> - VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end));
> - VM_WARN_ON(addr >= end);
> -
> if (prev) {
> vma_start = prev->vm_start;
> vma_pgoff = prev->vm_pgoff;
> +
> /* Can we merge the predecessor? */
> - if (prev->vm_end == addr && mpol_equal(vma_policy(prev), policy)
> + if (addr == prev->vm_end && mpol_equal(vma_policy(prev), policy)
> && can_vma_merge_after(prev, vm_flags, anon_vma, file,
> - pgoff, vm_userfaultfd_ctx, anon_name)) {
> + pgoff, vm_userfaultfd_ctx, anon_name)) {
> merge_prev = true;
> vma_prev(vmi);
> }
> }
>
> /* Can we merge the successor? */
> - if (next && mpol_equal(policy, vma_policy(next)) &&
> - can_vma_merge_before(next, vm_flags,
> - anon_vma, file, pgoff+pglen,
> - vm_userfaultfd_ctx, anon_name)) {
> - merge_next = true;
> - }
> + merge_next = next && mpol_equal(policy, vma_policy(next)) &&
> + can_vma_merge_before(next, vm_flags,
> + anon_vma, file, pgoff+pglen,
> + vm_userfaultfd_ctx, anon_name);
Not a great fan of this, I think the if() is more readable, but if you and
Liam agree, I won't mind much. Either way could consolidate the parameters
on less lines maybe.
> +
> + if (!merge_prev && !merge_next)
> + return NULL; /* Not mergeable. */
> +
> + /*
> + * By default, we return prev. Cases 3, 4, 8 will instead return next
> + * and cases 3, 8 will also update vma to point at next.
> + */
> + res = vma = prev;
> +
> + /* Verify some invariant that must be enforced by the caller. */
> + VM_WARN_ON(prev && addr <= prev->vm_start);
> + VM_WARN_ON(curr && (addr != curr->vm_start || end > curr->vm_end));
> + VM_WARN_ON(addr >= end);
>
> remove = remove2 = adjust = NULL;
> /* Can we merge both the predecessor and the successor? */
> @@ -984,7 +986,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> err = dup_anon_vma(prev, curr);
> }
> } else if (merge_prev) {
> - err = 0; /* case 2 */
> + /* case 2 */
Move the comment from this now weirdly empty line to the "else if" one above?
> if (curr) {
> err = dup_anon_vma(prev, curr);
> if (end == curr->vm_end) { /* case 7 */
> @@ -994,7 +996,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> adj_start = (end - curr->vm_start);
> }
> }
> - } else if (merge_next) {
> + } else { /* merge_next */
> res = next;
> if (prev && addr < prev->vm_end) { /* case 4 */
> vma_end = addr;
> @@ -1010,7 +1012,6 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> vma_start = addr;
> vma_end = next->vm_end;
> vma_pgoff = next->vm_pgoff;
> - err = 0;
> if (curr) { /* case 8 */
> vma_pgoff = curr->vm_pgoff;
> remove = curr;
> @@ -1019,7 +1020,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> }
> }
>
> - /* Cannot merge or error in anon_vma clone */
> + /* Error in anon_vma clone. */
> if (err)
> return NULL;
>
Powered by blists - more mailing lists