Re: [RFC Patch 2/5] anon_vma: add skeleton code for userland testing of anon_vma logic

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, May 01, 2025 at 01:31:27AM +0000, Wei Yang wrote:
> On Tue, Apr 29, 2025 at 09:06:36AM +0000, Wei Yang wrote:
> [...]
> >+
> >+static bool test_fork_grand_child(void)
> >+{
> >+	struct vm_area_struct *root_vma, *grand_vma, *vma1, *vma2;
> >+	struct anon_vma_chain *avc;
> >+	struct anon_vma *root_anon_vma;
> >+	DECLARE_BITMAP(expected, 10);
> >+	DECLARE_BITMAP(found, 10);
> >+
> >+	bitmap_zero(expected, 10);
> >+	bitmap_zero(found, 10);
> >+
> >+	/*
> >+	 *  root_anon_vma      root_vma
> >+	 *  +-----------+      +-----------+
> >+	 *  |           | ---> |           |
> >+	 *  +-----------+      +-----------+
> >+	 */
> >+
> >+	root_vma = alloc_vma(0x3000, 0x5000, 3);
> >+	/* First fault on parent anonymous vma. */
> >+	__anon_vma_prepare(root_vma);
> >+	root_anon_vma = root_vma->anon_vma;
> >+	ASSERT_NE(NULL, root_anon_vma);
> >+	bitmap_set(expected, root_vma->index, 1);
> >+
> >+	/* First fork */
> >+	/*
> >+	 *  root_anon_vma      root_vma
> >+	 *  +-----------+      +-----------+
> >+	 *  |           | ---> |           |
> >+	 *  +-----------+      +-----------+
> >+	 *                \
> >+	 *                 \   vma1
> >+	 *                  \  +-----------+
> >+	 *                   > |           |
> >+	 *                     +-----------+
> >+	 */
> >+	vma1 = alloc_vma(0x3000, 0x5000, 3);
> >+	anon_vma_fork(vma1, root_vma);
> >+	ASSERT_NE(NULL, vma1->anon_vma);
> >+	bitmap_set(expected, vma1->index, 1);
> >+	/* Parent/Root is root_vma->anon_vma */
> >+	ASSERT_EQ(vma1->anon_vma->parent, root_vma->anon_vma);
> >+	ASSERT_EQ(vma1->anon_vma->root, root_vma->anon_vma);
> >+
> >+	/* Second fork */
> >+	/*
> >+	 *  root_anon_vma      root_vma
> >+	 *  +-----------+      +-----------+
> >+	 *  |           | ---> |           |
> >+	 *  +-----------+      +-----------+
> >+	 *               \
> >+	 *                \------------------+
> >+	 *                 \   vma1           \   vma2
> >+	 *                  \  +-----------+   \  +-----------+
> >+	 *                   > |           |    > |           |
> >+	 *                     +-----------+      +-----------+
> >+	 */
> >+	vma2 = alloc_vma(0x3000, 0x5000, 3);
> >+	anon_vma_fork(vma2, root_vma);
> >+	ASSERT_NE(NULL, vma2->anon_vma);
> >+	bitmap_set(expected, vma2->index, 1);
> >+	/* Parent/Root is root_vma->anon_vma */
> >+	ASSERT_EQ(vma2->anon_vma->parent, root_vma->anon_vma);
> >+	ASSERT_EQ(vma2->anon_vma->root, root_vma->anon_vma);
> >+	dump_anon_vma_interval_tree(root_vma->anon_vma);
> >+
> >+	/* Fork grand child from second child */
> >+	/*
> >+	 *  root_anon_vma      root_vma
> >+	 *  +-----------+      +-----------+
> >+	 *  |           | ---> |           |
> >+	 *  +-----------+      +-----------+
> >+	 *               \
> >+	 *                \------------------+
> >+	 *                |\   vma1           \   vma2
> >+	 *                | \  +-----------+   \  +-----------+
> >+	 *                |  > |           |    > |           |
> >+	 *                |    +-----------+      +-----------+
> >+	 *                \
> >+	 *                 \   grand_vma
> >+	 *                  \  +-----------+
> >+	 *                   > |           |
> >+	 *                     +-----------+
> >+	 */
> >+	grand_vma = alloc_vma(0x3000, 0x5000, 3);
> >+	anon_vma_fork(grand_vma, vma2);
> >+	ASSERT_NE(NULL, grand_vma->anon_vma);
> >+	bitmap_set(expected, grand_vma->index, 1);
> >+	/* Root is root_vma->anon_vma */
> >+	ASSERT_EQ(grand_vma->anon_vma->root, root_vma->anon_vma);
> >+	/* Parent is vma2->anon_vma */
> >+	ASSERT_EQ(grand_vma->anon_vma->parent, vma2->anon_vma);
>
> Hi, Lorenzo
>
> Here is the case I am talking about in another thread[1].
>
> The naming is a little different from that.
>
>   * root_vma  is VMA A
>   * vma2      is VMA B
>   * grand_vma is VMA C
>
> If you add following debug code here.
>
> ```
> 	printf("root num_children %d\n", root_vma->anon_vma->num_children);
> 	printf("vma2 num_children %d\n", vma2->anon_vma->num_children);
> 	printf("grand_vma num_children %d\n", grand_vma->anon_vma->num_children);
> ```
>
> You would see vma2 num_children is 1, but it has a child.
>
> If I missed something, feel free to correct me.
>
> [1]: https://lkml.kernel.org/r/20250501011845.ktbfgymor4oz5sok@master
>

See the thread over there. This explanation isn't quite right, and the
diagram is wrong, but indeed anon_vma reuse is a thing that needs
addressing.

Having looked at this more I'm not a huge fan of these tests, you're
essentially open-coding fork over and over again, any change in overlying
fork logic will break.

The VMA and maple tree, etc. unit tests work much better as the whole thing
essentially can be separated out, but in this case you really can't. This
is obviously in addition to the aforementioned issues with causing
separation in an area where I want to try to unify.

So I definitely think that, again, you'd be better off looking at tests as
suggested by David, but moreover I think looking at bugs etc. is more
helpful.

This report was in effect reporting a bug before it happened so all good
for this kind of thing :>) and that is _always_ welcome. But do try to be
more direct and to the point if you can be.

Thanks!


> >+
> >+	/* Expect to find only vmas from second fork */
> >+	anon_vma_interval_tree_foreach(avc, &vma2->anon_vma->rb_root, 3, 4) {
> >+		ASSERT_TRUE(avc->vma == vma2 || avc->vma == grand_vma);
> >+	}
> >+
> >+	anon_vma_interval_tree_foreach(avc, &root_vma->anon_vma->rb_root, 3, 4) {
> >+		bitmap_set(found, avc->vma->index, 1);
> >+	}
> >+	/* Expect to find all vma including child and grand child. */
> >+	ASSERT_TRUE(bitmap_equal(expected, found, 10));
> >+
> >+	/* Root process exit or unmap root_vma. */
> >+	/*
> >+	 *  root_anon_vma
> >+	 *  +-----------+
> >+	 *  |           |
> >+	 *  +-----------+
> >+	 *               \
> >+	 *                \------------------+
> >+	 *                |\   vma1           \   vma2
> >+	 *                | \  +-----------+   \  +-----------+
> >+	 *                |  > |           |    > |           |
> >+	 *                |    +-----------+      +-----------+
> >+	 *                \
> >+	 *                 \   grand_vma
> >+	 *                  \  +-----------+
> >+	 *                   > |           |
> >+	 *                     +-----------+
> >+	 */
> >+	bitmap_clear(expected, root_vma->index, 1);
> >+	unlink_anon_vmas(root_vma);
> >+	ASSERT_EQ(0, root_anon_vma->num_active_vmas);
> >+
> >+	bitmap_zero(found, 10);
> >+	anon_vma_interval_tree_foreach(avc, &root_anon_vma->rb_root, 3, 4) {
> >+		bitmap_set(found, avc->vma->index, 1);
> >+	}
> >+	/* Expect to find all vmas even root_vma released. */
> >+	ASSERT_TRUE(bitmap_equal(expected, found, 10));
> >+
> >+	cleanup();
> >+
> >+	ASSERT_EQ(0, nr_allocated);
> >+	return true;
> >+}
> >+
>
>
> --
> Wei Yang
> Help you, Help me
>