| 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: <c2d88a2f-01d2-4810-9ac3-50be94bf8d04@lucifer.local>
Date: Wed, 29 Jan 2025 14:19:02 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
"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 Wed, Jan 29, 2025 at 03:13:06PM +0100, Vlastimil Babka wrote:
> 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>
Thanks!
>
> > ---
> > 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)
Maybe, but the 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.
I'd rather explicitly assign the value in each branch though just so you don't
have to figure out there's not an assignment so default value is used.
But also... maybe, but the code is temporary ;)
>
> > + } 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.
Again, I feel it's simpler to have 3 branches where values are set according to
branch conditions so you don't have to figure out which value is set to which by
cross-referencing default values and assignments.
But again I also will say (you know what's coming...) maybe, but the code is
temporary ;)
>
> > - 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