Re: [PATCH] maple_tree: Fix sparse reported issues

[Liam Howlett]

* Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [220713 13:50]:
> * Hugh Dickins <hughd@xxxxxxxxxx> [220713 11:56]:
> > On Wed, 13 Jul 2022, Liam Howlett wrote:
> > > * David Hildenbrand <david@xxxxxxxxxx> [220713 04:34]:
> > > > On 12.07.22 16:24, Liam Howlett wrote:
> > > > > When building with C=1, the maple tree had some rcu type mismatch &
> > > > > locking mismatches in the destroy functions.  There were cosmetic only
> > > > > since this happens after the nodes are removed from the tree.
> > > > > 
> > > > > Fixes: f8acc5e9581e (Maple Tree: add new data structure)
> > > > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > > > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> > > > 
> > > > Sorry to say, but the fixes become hard to follow (what/where/why). :)
> > > > 
> > > > I guess it's time for a new series soon. Eventually it makes sense to
> > > > send the fixes as reply to the individual problematic patches. (instead
> > > > of fixes to commit ids that are not upstream)
> > > > 
> > > > [yes, I'll do more review soon :) ]
> > > 
> > > I appreciate the feedback, it's much better than yelling into the void.
> > > I have one more fix in the works - for __vma_adjust() of all functions
> > > so that'll be impossible to follow anyways :)  I'll work on a v11 to
> > > include that last one.
> > 
> > Please do also post the incremental for that "one more fix" once it's
> > ready: I have been keeping up with what you've been posting so far,
> > folding them into my debugging here, and believe we have made some but
> > still not enough progress on the bugs I hit.  Folding in one more fix
> > will be easy for me, advancing to v11 of a 69-part patchset will be...
> > dispiriting.
> 
> 
> Okay, thanks.  I don't think it will fix your outstanding issues but it
> is necessary to fix case 6 of vma_merge() on memory allocation failure
> as reported by syzbot.

Hugh,

Please find attached the last outstanding fix for this series.  It
covers a rare failure scenario that one of the build bots reported.
Ironically, it changes __vma_adjust() more than the rest of the series,
but leaves the locking in the existing order.

Thanks,
Liam

From 081995cf1347406cb8be8c7ce11fbb256158c83e Mon Sep 17 00:00:00 2001
From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
Date: Wed, 13 Jul 2022 10:13:34 -0400
Subject: [PATCH 1/1] mm/mmap: Fix __vma_adjust() issue on memory failure

In case 6 of vma_merge, two VMAs will be freed, but when allocation
fails after the first __vma_adjust() completes and jumps back to the
"again" label, then the second VMA is not merged.  Upon returning from
the __vma_adjust() call with an error code, the calling process assumes
the first VMA is still valid, but it has been freed.

Reported-by: syzbot+68771c0e74f7bb7804e5@xxxxxxxxxxxxxxxxxxxxxxxxx
Fixes: d3ccd17e7c96 ("mm: start tracking VMAs with maple tree")
Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
---
 mm/mmap.c | 42 ++++++++++--------------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index f25f53d7600d..5ed06870a3f3 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -713,8 +713,6 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 				next_next = find_vma(mm, next->vm_end);
 				VM_WARN_ON(remove_next == 2 &&
 					   end != next_next->vm_end);
-				/* trim end to next, for case 6 first pass */
-				end = next->vm_end;
 			}
 
 			exporter = next;
@@ -762,7 +760,7 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 				return error;
 		}
 	}
-again:
+
 	vma_adjust_trans_huge(orig_vma, start, end, adjust_next);
 
 	if (mas_preallocate(&mas, vma, GFP_KERNEL)) {
@@ -853,6 +851,9 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 
 	if (remove_next && file) {
 		__remove_shared_vm_struct(next, file, mapping);
+		if (remove_next == 2)
+			__remove_shared_vm_struct(next_next, file, mapping);
+
 	} else if (insert) {
 		/*
 		 * split_vma has split insert from vma, and needs
@@ -880,47 +881,24 @@ int __vma_adjust(struct vm_area_struct *vma, unsigned long start,
 	}
 
 	if (remove_next) {
+again:
 		if (file) {
 			uprobe_munmap(next, next->vm_start, next->vm_end);
 			fput(file);
 		}
 		if (next->anon_vma)
 			anon_vma_merge(vma, next);
+
 		mm->map_count--;
 		mpol_put(vma_policy(next));
-		BUG_ON(vma->vm_end < next->vm_end);
+		if (remove_next != 2)
+			BUG_ON(vma->vm_end < next->vm_end);
 		vm_area_free(next);
 
-		/*
-		 * In mprotect's case 6 (see comments on vma_merge),
-		 * we must remove another next too. It would clutter
-		 * up the code too much to do both in one go.
-		 */
-		if (remove_next != 3) {
-			/*
-			 * If "next" was removed and vma->vm_end was
-			 * expanded (up) over it, in turn
-			 * "next->prev->vm_end" changed and the
-			 * "vma->next" gap must be updated.
-			 */
-			next = next_next;
-		} else {
-			/*
-			 * For the scope of the comment "next" and
-			 * "vma" considered pre-swap(): if "vma" was
-			 * removed, next->vm_start was expanded (down)
-			 * over it and the "next" gap must be updated.
-			 * Because of the swap() the post-swap() "vma"
-			 * actually points to pre-swap() "next"
-			 * (post-swap() "next" as opposed is now a
-			 * dangling pointer).
-			 */
-			next = vma;
-		}
 		if (remove_next == 2) {
-			mas_reset(&mas);
+			/* Case 6 (see vma_merge comments).  Clean up next_next. */
 			remove_next = 1;
-			end = next->vm_end;
+			next = next_next;
 			goto again;
 		}
 	}
-- 
2.35.1