linux-kernel - Re: [PATCH 3/3] kunit: test: fix mm_struct leak in kunit_attach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSnGH=nvJPsww5KjeQmTpX46HE0PJ20uOWpcWDi11MyOgw@mail.gmail.com>
Date: Mon, 5 Jan 2026 17:56:13 +0800
From: David Gow <davidgow@...gle.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <raemoar63@...il.com>, 
	Kees Cook <kees@...nel.org>, Ingo Molnar <mingo@...hat.com>, 
	Peter Zijlstra <peterz@...radead.org>, Juri Lelli <juri.lelli@...hat.com>, 
	Vincent Guittot <vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>, 
	Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	David Hildenbrand <david@...nel.org>, Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, 
	"Liam R. Howlett" <Liam.Howlett@...cle.com>, Vlastimil Babka <vbabka@...e.cz>, 
	Mike Rapoport <rppt@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 3/3] kunit: test: fix mm_struct leak in kunit_attach_mm()

On Wed, 24 Dec 2025 at 00:18, Brendan Jackman <jackmanb@...gle.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@...gle.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@...gle.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
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)