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

[Wei Yang <richard.weiyang@gmail.com>]

On Thu, May 01, 2025 at 10:41:54AM +0100, Lorenzo Stoakes wrote:
>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.

Sure. Thank you and David, pointing me a correct direction. And yes, bug has
higher priority.

>
>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.

Ah, I am glad you like it. Would try to be more clear on expressing my point
next time. And thanks for your patience on reading my "confusing" mail.

>
>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
>>

-- 
Wei Yang
Help you, Help me