Re: [PATCH v3 0/9] mm: clean up anon_vma implementation

[Andrew Morton <akpm@linux-foundation.org>]

On Sun, 18 Jan 2026 14:50:36 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> The anon_vma logic is hugely confusing and, much like a bundle of wires
> entangled with one another, pulling on one thread seems only to lead to
> more entanglement elsewhere.
> 
> There is a mish-mash of the core implementation, how that implementation is
> invoked, how helper functions are invoked and concepts such as adjacent
> anon_vma merge and anon_vma object reuse.
> 
> This series tries to improve the situation somewhat.

Updated, thanks.

> v3:
> * Propagate tags (thanks everyone!)
> * Fold fix-patches into series.
> * Add fix for syzbot report about an accursed partially-initialised VMA
>   fault injection error path.
> * Fixed a typo, a comment whitespace error I noticed and add some comments
>   to anon_vma_fork(), set anon_vma->num_active_vmas = 1 to make it clear
>   that we're setting this on a newly allocated anon_vma.

Below is how this update altered mm.git:


--- a/mm/rmap.c~b
+++ a/mm/rmap.c
@@ -333,10 +333,10 @@ int anon_vma_clone(struct vm_area_struct
 	 * are not updating the anon_vma rbtree nor are we changing
 	 * anon_vma statistics.
 	 *
-	* Either src, dst have the same mm for which we hold an exclusive mmap
-	* write lock, or we are forking and we hold it on src->vm_mm and dst is
-	* not yet accessible to other threads so there's no possibliity of the
-	* unlinked AVC's being observed yet.
+	 * Either src, dst have the same mm for which we hold an exclusive mmap
+	 * write lock, or we are forking and we hold it on src->vm_mm and dst is
+	 * not yet accessible to other threads so there's no possibliity of the
+	 * unlinked AVC's being observed yet.
 	 */
 	list_for_each_entry(pavc, &src->anon_vma_chain, same_vma) {
 		avc = anon_vma_chain_alloc(GFP_KERNEL);
@@ -379,7 +379,7 @@ int anon_vma_fork(struct vm_area_struct
 {
 	struct anon_vma_chain *avc;
 	struct anon_vma *anon_vma;
-	int error;
+	int rc;
 
 	/* Don't bother if the parent process has no anon_vma here. */
 	if (!pvma->anon_vma)
@@ -388,27 +388,35 @@ int anon_vma_fork(struct vm_area_struct
 	/* Drop inherited anon_vma, we'll reuse existing or allocate new. */
 	vma->anon_vma = NULL;
 
+	anon_vma = anon_vma_alloc();
+	if (!anon_vma)
+		return -ENOMEM;
+	avc = anon_vma_chain_alloc(GFP_KERNEL);
+	if (!avc) {
+		put_anon_vma(anon_vma);
+		return -ENOMEM;
+	}
+
 	/*
 	 * First, attach the new VMA to the parent VMA's anon_vmas,
 	 * so rmap can find non-COWed pages in child processes.
 	 */
-	error = anon_vma_clone(vma, pvma, VMA_OP_FORK);
-	if (error)
-		return error;
-
-	/* An existing anon_vma has been reused, all done then. */
-	if (vma->anon_vma)
-		return 0;
+	rc = anon_vma_clone(vma, pvma, VMA_OP_FORK);
+	/* An error arose or an existing anon_vma was reused, all done then. */
+	if (rc || vma->anon_vma) {
+		put_anon_vma(anon_vma);
+		anon_vma_chain_free(avc);
+		return rc;
+	}
 
-	/* Then add our own anon_vma. */
-	anon_vma = anon_vma_alloc();
-	if (!anon_vma)
-		goto out_error;
-	anon_vma->num_active_vmas++;
-	avc = anon_vma_chain_alloc(GFP_KERNEL);
-	if (!avc)
-		goto out_error_free_anon_vma;
+	/*
+	 * OK no reuse, so add our own anon_vma.
+	 *
+	 * Since it is not linked anywhere we can safely manipulate anon_vma
+	 * fields without a lock.
+	 */
 
+	anon_vma->num_active_vmas = 1;
 	/*
 	 * The root anon_vma's rwsem is the lock actually used when we
 	 * lock any of the anon_vmas in this anon_vma tree.
@@ -431,12 +439,6 @@ int anon_vma_fork(struct vm_area_struct
 	anon_vma_unlock_write(anon_vma);
 
 	return 0;
-
- out_error_free_anon_vma:
-	put_anon_vma(anon_vma);
- out_error:
-	unlink_anon_vmas(vma);
-	return -ENOMEM;
 }
 
 /*
_