* Vlastimil Babka <vbabka@xxxxxxx> [230222 11:17]:
> On 1/20/23 17:26, Liam R. Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> >
> > Inline the work of __vma_adjust() into vma_merge(). This reduces code
> > size and has the added benefits of the comments for the cases being
> > located with the code.
> >
> > Change the comments referencing vma_adjust() accordingly.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
>
> ...
>
> > @@ -1054,32 +945,85 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> > vm_userfaultfd_ctx, anon_name)) {
> > merge_next = true;
> > }
> > +
> > + remove = remove2 = adjust = NULL;
> > /* Can we merge both the predecessor and the successor? */
> > if (merge_prev && merge_next &&
> > - is_mergeable_anon_vma(prev->anon_vma,
> > - next->anon_vma, NULL)) { /* cases 1, 6 */
> > - err = __vma_adjust(vmi, prev, prev->vm_start,
> > - next->vm_end, prev->vm_pgoff, prev);
> > - res = prev;
> > - } else if (merge_prev) { /* cases 2, 5, 7 */
> > - err = __vma_adjust(vmi, prev, prev->vm_start,
> > - end, prev->vm_pgoff, prev);
> > - res = prev;
> > + is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL)) {
> > + remove = mid; /* case 1 */
> > + vma_end = next->vm_end;
> > + err = dup_anon_vma(res, remove);
> > + if (mid != next) { /* case 6 */
> > + remove2 = next;
> > + if (!remove->anon_vma)
> > + err = dup_anon_vma(res, remove2);
> > + }
> > + } else if (merge_prev) {
> > + err = 0; /* case 2 */
> > + if (mid && end > mid->vm_start) {
> > + err = dup_anon_vma(res, mid);
> > + if (end == mid->vm_end) { /* case 7 */
> > + remove = mid;
> > + } else { /* case 5 */
> > + adjust = mid;
> > + adj_next = (end - mid->vm_start);
> > + }
> > + }
> > } else if (merge_next) {
> > - if (prev && addr < prev->vm_end) /* case 4 */
> > - err = __vma_adjust(vmi, prev, prev->vm_start,
> > - addr, prev->vm_pgoff, next);
> > - else /* cases 3, 8 */
> > - err = __vma_adjust(vmi, mid, addr, next->vm_end,
> > - next->vm_pgoff - pglen, next);
> > res = next;
> > + if (prev && addr < prev->vm_end) { /* case 4 */
> > + vma_end = addr;
> > + adjust = mid;
> > + adj_next = -(vma->vm_end - addr);
> > + err = dup_anon_vma(res, adjust);
>
> I think this one is wrong, and should be fixed as below. I'm not
> exactly sure about user visible effects, but shouldn't matter if
> we fix before rc1? I guess what can happen is we end up with pages
> becoming part of 'prev' that have anon_vma originally from 'mid'
> which is not connected to 'prev', so eventually some rmap operation
> will fail to do the right thing etc. Or 'mid' is unmapped, its
> anon_vma freed and we have a use-after free. Probably rare to happen,
> but nasty enough.
Yes, you are correct. Thanks for the closer look here.
>
> ----8<----
> From 854f4cef0fecde9a0a89ff1a5beb0a1e2115363f Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@xxxxxxx>
> Date: Wed, 22 Feb 2023 16:51:46 +0100
> Subject: [PATCH urgent for 6.3-rc1] mm/mremap: fix dup_anon_vma() in vma_merge() case 4
>
> In case 4, we are shrinking 'prev' (PPPP in the comment) and expanding
> 'mid' (NNNN). So we need to make sure 'mid' clones the anon_vma from
> 'prev', if it doesn't have any. After commit 0503ea8f5ba7 ("mm/mmap:
> remove __vma_adjust()") we can fail to do that due to wrong parameters
> for dup_anon_vma(). The call is a no-op because res == next, adjust ==
> mid and mid == next. Fix it.
>
> Fixes: 0503ea8f5ba7 ("mm/mmap: remove __vma_adjust()")
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
> mm/mmap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 20f21f0949dd..740b54be3ed4 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -973,7 +973,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> vma_end = addr;
> adjust = mid;
> adj_next = -(vma->vm_end - addr);
> - err = dup_anon_vma(res, adjust);
> + err = dup_anon_vma(adjust, prev);
Reviewed-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> } else {
> vma = next; /* case 3 */
> vma_start = addr;
> --
> 2.39.2
>
>
>
