Re: [PATCH 3/3] kunit: test: fix mm_struct leak in kunit_attach_mm()

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3374 bytes --]

On Wed, 24 Dec 2025 at 00:18, Brendan Jackman <jackmanb@google.com> wrote:
>
> Here's how I understand mm refcounts:
>
>   funcs             | counter  | manages lifecycle of...
>   --------------------------------------------------------
>   mmgrab()/mmdrop() | mm_count | mm_struct and PGD
>   --------------------------------------------------------
>   mmget()/mmput()   | mm_users | userspace address space
>
>     All mm_users references share a single reference to the mm_struct.
>
> mm_alloc() returns the mm with a single reference to the user address
> space, i.e. with mm_users=1, mm_count=1.
>
> kunit_attach_mm() then passes the mm to kthread_use_mm(). It does not
> call kthread_unuse_mm(), instead it relies on the kthread exit path to
> release the relevant resources. It does this because KUnit's resource
> cleanup logic works by running cleanups in a different kthread from the
> test. You can't have cleanups that operate on the kthread, because
> the kthread is already gone by the time the cleanup is called.
>
> The kthread exit path will indeed drop the reference to the address
> space, i.e. it will call mmput(task->mm), decrementing mm_users.
> However, it does not release the reference taken on the mm_struct when
> kthread_use_mm() called mmgrab().
>
> To fix this, use the new kthread_take_mm() which provides the API KUnit
> needs.
>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---

This seems right. We did have the ability to run cleanups on the test
kthread, but we couldn't guarantee it (because the test thread could
be aborted early, due to test failures, timeouts, etc).

I'm assuming that we can't just throw an mmdrop() in a resource
cleanup function (e.g. kunit_add_action()), since we can't do a full
kthread_unuse_mm()? I gave it a quick try and nothing crashed, but
that's not exactly a ringing endorsement that it's correct:

diff --git a/lib/kunit/user_alloc.c b/lib/kunit/user_alloc.c
index 3fca4ae223f6..5177537f592b 100644
--- a/lib/kunit/user_alloc.c
+++ b/lib/kunit/user_alloc.c
@@ -7,6 +7,8 @@
#include <linux/kthread.h>
#include <linux/mm.h>

+KUNIT_DEFINE_ACTION_WRAPPER(kunit_mmdrop_action, mmdrop, struct mm_struct*);
+
int kunit_attach_mm(void)
{
       struct mm_struct *mm;
@@ -29,7 +31,9 @@ int kunit_attach_mm(void)
       arch_pick_mmap_layout(mm, &current->signal->rlim[RLIMIT_STACK]);

       /* Attach the mm. It will be cleaned up when the process dies. */
-       kthread_take_mm(mm);
+       kthread_use_mm(mm);
+
+       kunit_add_action(current->kunit_test, kunit_mmdrop_action, mm);

       return 0;
}


That being said, if mm folks are okay with the kthread_take_mm()
approach, that certainly makes things easier on our end, so I'm happy
with this as an option.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David



>  lib/kunit/user_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/kunit/user_alloc.c b/lib/kunit/user_alloc.c
> index 564f5566641d5..3fca4ae223f67 100644
> --- a/lib/kunit/user_alloc.c
> +++ b/lib/kunit/user_alloc.c
> @@ -29,7 +29,7 @@ int kunit_attach_mm(void)
>         arch_pick_mmap_layout(mm, &current->signal->rlim[RLIMIT_STACK]);
>
>         /* Attach the mm. It will be cleaned up when the process dies. */
> -       kthread_use_mm(mm);
> +       kthread_take_mm(mm);
>
>         return 0;
>  }
>
> --
> 2.51.2
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]