| 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: <2e6fac60-3b73-4e7a-a824-21cbe2c326e6@suse.cz>
Date: Wed, 29 Jan 2025 15:13:06 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: "Liam R . Howlett" <Liam.Howlett@...cle.com>, Jann Horn
<jannh@...gle.com>, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] mm: eliminate adj_start parameter from commit_merge()
On 1/27/25 16:50, Lorenzo Stoakes wrote:
> Introduce internal __VMG_FLAG_ADJUST_MIDDLE_START and
> __VMG_FLAG_ADJUST_NEXT_START merge flags, enabling us to indicate to
> commit_merge() that we are performing a merge which either spans only part
> of vmg->middle, or part of vmg->next respectively.
>
> In the former instance, we change the start of vmg->middle to match the
> attributes of vmg->prev, without spanning all of vmg->middle.
>
> This implies that vmg->prev->vm_end and vmg->middle->vm_start are both
> increased to form the new merged VMA (vmg->prev) and the new subsequent VMA
> (vmg->middle).
>
> In the latter case, we change the end of vmg->middle to match the
> attributes of vmg->next, without spanning all of vmg->next.
>
> This implies that vmg->middle->vm_end and vmg->next->vm_start are both
> decreased to form the new merged VMA (vmg->next) and the new prior VMA
> (vmg->middle).
>
> Since we now have a stable set of prev, middle, next VMAs threaded through
> vmg and with these flags set know what is happening, we can perform
> the calculation in commit_merge() instead.
>
> This allows us to drop the confusing adj_start parameter and instead pass
> semantic information to commit_merge().
>
> In the latter case the -(middle->vm_end - start) calculation becomes
> -(middle->vm-end - vmg->end), however this is correct as vmg->end is set to
> the start parameter.
>
> This is because in this case (rather confusingly), we manipulate
> vmg->middle, but ultimately return vmg->next, whose range will be correctly
> specified. At this point vmg->start, end is the new range for the prior VMA
> rather than the merged one.
>
> This patch has no change in functional behaviour.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Reviewed-by: Vlastimil Babka <vbabka@...e.cz>
> ---
> mm/vma.c | 53 ++++++++++++++++++++++++++++++++---------------------
> mm/vma.h | 14 ++++++++++++--
> 2 files changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 955c5ebd5739..e78d65de734b 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -633,29 +633,45 @@ void validate_mm(struct mm_struct *mm)
> *
> * On success, returns the merged VMA. Otherwise returns NULL.
> */
> -static struct vm_area_struct *commit_merge(struct vma_merge_struct *vmg,
> - long adj_start)
> +static struct vm_area_struct *commit_merge(struct vma_merge_struct *vmg)
> {
> - struct vma_prepare vp;
> struct vm_area_struct *remove = NULL;
> struct vm_area_struct *remove2 = NULL;
> + unsigned long flags = vmg->merge_flags;
> + struct vma_prepare vp;
> struct vm_area_struct *adjust = NULL;
> + long adj_start;
> + bool merge_target;
> +
> /*
> - * In all cases but that of merge right, shrink next, we write
> - * vmg->target to the maple tree and return this as the merged VMA.
> + * If modifying an existing VMA and we don't remove vmg->middle, then we
> + * shrink the adjacent VMA.
> */
> - bool merge_target = adj_start >= 0;
> + if (flags & __VMG_FLAG_ADJUST_MIDDLE_START) {
> + adjust = vmg->middle;
> + /* The POSITIVE value by which we offset vmg->middle->vm_start. */
> + adj_start = vmg->end - vmg->middle->vm_start;
> + merge_target = true;
> + } else if (flags & __VMG_FLAG_ADJUST_NEXT_START) {
> + adjust = vmg->next;
> + /* The NEGATIVE value by which we offset vmg->next->vm_start. */
> + adj_start = -(vmg->middle->vm_end - vmg->end);
Wouldn't it make more sense to use vmg->next->vm_start here, which AFAIU is
the same value as vmg->middle->vm_end, but perhaps a bit more obvious
considering the flag and comment says we're adjusting vmg->next->vm_start.
(somewhat less important if this code is temporary)
> + /*
> + * In all cases but this - merge right, shrink next - we write
> + * vmg->target to the maple tree and return this as the merged VMA.
> + */
> + merge_target = false;
That comment reads like it would make sense to init merge_target as true and
only here change it to false.
> + } else {
> + adjust = NULL;
> + adj_start = 0;
> + merge_target = true;
> + }
I guess all these could be initial assignments and we don't need the else
branch at all.
> - if (vmg->merge_flags & __VMG_FLAG_REMOVE_MIDDLE)
> + if (flags & __VMG_FLAG_REMOVE_MIDDLE)
> remove = vmg->middle;
> if (vmg->merge_flags & __VMG_FLAG_REMOVE_NEXT)
> remove2 = vmg->next;
>
> - if (adj_start > 0)
> - adjust = vmg->middle;
> - else if (adj_start < 0)
> - adjust = vmg->next;
> -
> init_multi_vma_prep(&vp, vmg->target, adjust, remove, remove2);
>
> VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma &&
> @@ -739,7 +755,6 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
> bool left_side = middle && start == middle->vm_start;
> bool right_side = middle && end == middle->vm_end;
> int err = 0;
> - long adj_start = 0;
> bool merge_will_delete_middle, merge_will_delete_next;
> bool merge_left, merge_right, merge_both;
>
> @@ -860,11 +875,8 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
> vmg->start = prev->vm_start;
> vmg->pgoff = prev->vm_pgoff;
>
> - /*
> - * We both expand prev and shrink middle.
> - */
> if (!merge_will_delete_middle)
> - adj_start = vmg->end - middle->vm_start;
> + vmg->merge_flags |= __VMG_FLAG_ADJUST_MIDDLE_START;
>
> err = dup_anon_vma(prev, middle, &anon_dup);
> } else { /* merge_right */
> @@ -893,12 +905,11 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
> * IMPORTANT: This is the ONLY case where the final
> * merged VMA is NOT vmg->target, but rather vmg->next.
> */
> + vmg->merge_flags |= __VMG_FLAG_ADJUST_NEXT_START;
> vmg->target = middle;
> vmg->start = middle->vm_start;
> vmg->end = start;
> vmg->pgoff = middle->vm_pgoff;
> -
> - adj_start = -(middle->vm_end - start);
> }
>
> err = dup_anon_vma(next, middle, &anon_dup);
> @@ -912,7 +923,7 @@ static __must_check struct vm_area_struct *vma_merge_existing_range(
> if (merge_will_delete_next)
> vmg->merge_flags |= __VMG_FLAG_REMOVE_NEXT;
>
> - res = commit_merge(vmg, adj_start);
> + res = commit_merge(vmg);
> if (!res) {
> if (anon_dup)
> unlink_anon_vmas(anon_dup);
> @@ -1087,7 +1098,7 @@ int vma_expand(struct vma_merge_struct *vmg)
> if (remove_next)
> vmg->merge_flags |= __VMG_FLAG_REMOVE_NEXT;
>
> - if (!commit_merge(vmg, 0))
> + if (!commit_merge(vmg))
> goto nomem;
>
> return 0;
> diff --git a/mm/vma.h b/mm/vma.h
> index ffbfefb9a83d..ddf567359880 100644
> --- a/mm/vma.h
> +++ b/mm/vma.h
> @@ -67,16 +67,26 @@ enum vma_merge_flags {
> * at the gap.
> */
> VMG_FLAG_JUST_EXPAND = 1 << 0,
> + /*
> + * Internal flag indicating the merge increases vmg->middle->vm_start
> + * (and thereby, vmg->prev->vm_end).
> + */
> + __VMG_FLAG_ADJUST_MIDDLE_START = 1 << 1,
> + /*
> + * Internal flag indicating the merge decreases vmg->next->vm_start
> + * (and thereby, vmg->middle->vm_end).
> + */
> + __VMG_FLAG_ADJUST_NEXT_START = 1 << 2,
> /*
> * Internal flag used during the merge operation to indicate we will
> * remove vmg->middle.
> */
> - __VMG_FLAG_REMOVE_MIDDLE = 1 << 1,
> + __VMG_FLAG_REMOVE_MIDDLE = 1 << 3,
> /*
> * Internal flag used during the merge operationr to indicate we will
> * remove vmg->next.
> */
> - __VMG_FLAG_REMOVE_NEXT = 1 << 2,
> + __VMG_FLAG_REMOVE_NEXT = 1 << 4,
> };
>
> /*
Powered by blists - more mailing lists