[PATCH 0/3] EDITME: cover title for b4/kunit-user-alloc

[Brendan Jackman]

kunit_attach_mm() leaks an mm_struct (verified with dumb printf
debugging). Fix that. In the process, add a new kthread mm operation,
and clean up some nearby cleanup code in the KUnit lib.

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

---
Brendan Jackman (3):
kunit: test: Delete pointless resource API usage
kthread: Add kthread_take_mm()
kunit: test: fix mm_struct leak in kunit_attach_mm()

include/linux/kthread.h | 1 +
kernel/fork.c | 3 +-
kernel/kthread.c | 36 +++++++++++++++++------
lib/kunit/user_alloc.c | 78 +++++--------------------------------------------
4 files changed, 37 insertions(+), 81 deletions(-)
---
base-commit: 9448598b22c50c8a5bb77a9103e2d49f134c9578
change-id: 20251223-b4-kunit-user-alloc-6ae8df0b8a92

Best regards,
--
Brendan Jackman <jack...@google.com>

[Brendan Jackman]

This code uses the low-level resource API to track parameters of the
vm_mmap call, but it doesn't do anything with them, because the mm
teardown code takes care of tearing down the mmaps. Delete it.

Signed-off-by: Brendan Jackman <jack...@google.com>
---
lib/kunit/user_alloc.c | 76 ++++----------------------------------------------
1 file changed, 6 insertions(+), 70 deletions(-)

diff --git a/lib/kunit/user_alloc.c b/lib/kunit/user_alloc.c
index b8cac765e6204..564f5566641d5 100644
--- a/lib/kunit/user_alloc.c
+++ b/lib/kunit/user_alloc.c
@@ -7,21 +7,6 @@
#include <linux/kthread.h>
#include <linux/mm.h>

-struct kunit_vm_mmap_resource {
- unsigned long addr;
- size_t size;
-};
-
-/* vm_mmap() arguments */
-struct kunit_vm_mmap_params {
- struct file *file;
- unsigned long addr;
- unsigned long len;
- unsigned long prot;
- unsigned long flag;
- unsigned long offset;
-};
-
int kunit_attach_mm(void)
{
struct mm_struct *mm;
@@ -50,67 +35,18 @@ int kunit_attach_mm(void)
}
EXPORT_SYMBOL_GPL(kunit_attach_mm);

-static int kunit_vm_mmap_init(struct kunit_resource *res, void *context)
-{
- struct kunit_vm_mmap_params *p = context;
- struct kunit_vm_mmap_resource vres;
- int ret;
-
- ret = kunit_attach_mm();
- if (ret)
- return ret;
-
- vres.size = p->len;
- vres.addr = vm_mmap(p->file, p->addr, p->len, p->prot, p->flag, p->offset);
- if (!vres.addr)
- return -ENOMEM;
- res->data = kmemdup(&vres, sizeof(vres), GFP_KERNEL);
- if (!res->data) {
- vm_munmap(vres.addr, vres.size);
- return -ENOMEM;
- }
-
- return 0;
-}
-
-static void kunit_vm_mmap_free(struct kunit_resource *res)
-{
- struct kunit_vm_mmap_resource *vres = res->data;
-
- /*
- * Since this is executed from the test monitoring process,
- * the test's mm has already been torn down. We don't need
- * to run vm_munmap(vres->addr, vres->size), only clean up
- * the vres.
- */
-
- kfree(vres);
- res->data = NULL;
-}
-
unsigned long kunit_vm_mmap(struct kunit *test, struct file *file,
unsigned long addr, unsigned long len,
unsigned long prot, unsigned long flag,
unsigned long offset)
{
- struct kunit_vm_mmap_params params = {
- .file = file,
- .addr = addr,
- .len = len,
- .prot = prot,
- .flag = flag,
- .offset = offset,
- };
- struct kunit_vm_mmap_resource *vres;
+ int err;

- vres = kunit_alloc_resource(test,
- kunit_vm_mmap_init,
- kunit_vm_mmap_free,
- GFP_KERNEL,
- &params);
- if (vres)
- return vres->addr;
- return 0;
+ err = kunit_attach_mm();
+ if (err)
+ return err;
+
+ return vm_mmap(file, addr, len, prot, flag, offset);
}
EXPORT_SYMBOL_GPL(kunit_vm_mmap);


--
2.51.2

[Brendan Jackman]

lib/kunit/user_alloc.c currently uses kthread_use_mm() without a
corresponding kthread_unuse_mm(). This is a bug, but fixing it in KUnit
makes writing tests that use mms more difficult, because of KUnit's
resource/try-catch model.

Therefore, introduce a new operation that does what kunit_attach_mm()
wants, namely an unbalanced call with cleanup deferred to
kthread_exit().

This is actually just the same as kthread_use_mm() but without taking a
reference on the mm_struct.

While adding this, clarify the reference returned by mm_alloc(), since
that is what kthread_take_mm() is gonna be paired with, in practice.

Signed-off-by: Brendan Jackman <jack...@google.com>
---

include/linux/kthread.h | 1 +
kernel/fork.c | 3 ++-
kernel/kthread.c | 36 +++++++++++++++++++++++++++---------
3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 8d27403888ce9..2e6244d8ff1a3 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -259,6 +259,7 @@ bool kthread_cancel_delayed_work_sync(struct kthread_delayed_work *work);

void kthread_destroy_worker(struct kthread_worker *worker);

+void kthread_take_mm(struct mm_struct *mm);
void kthread_use_mm(struct mm_struct *mm);
void kthread_unuse_mm(struct mm_struct *mm);

diff --git a/kernel/fork.c b/kernel/fork.c
index b1f3915d5f8ec..761e6232ea75a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1147,7 +1147,8 @@ static struct mm_struct *mm_init(struct mm_struct *mm, struct task_struct *p,
}

/*
- * Allocate and initialize an mm_struct.
+ * Allocate and initialize an mm_struct. The caller gets a single reference to
+ * the mm's address space, which should be released with a call to mmput().
*/
struct mm_struct *mm_alloc(void)
{
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 99a3808d086f0..c660c04a1b627 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -1589,10 +1589,16 @@ void kthread_destroy_worker(struct kthread_worker *worker)
EXPORT_SYMBOL(kthread_destroy_worker);

/**
- * kthread_use_mm - make the calling kthread operate on an address space
+ * kthread_take_mm - make the calling kthread own an address space.
+ *
+ * Unlike kthread_use_mm(), this doesn't have a cleanup, instead that happens
+ * automatically on kthread exit. Correspondingly, it does not take any
+ * references, by calling this function you donate your reference to the address
+ * space (from mmget()/mm_users).
+ *
* @mm: address space to operate on
*/
-void kthread_use_mm(struct mm_struct *mm)
+void kthread_take_mm(struct mm_struct *mm)
{
struct mm_struct *active_mm;
struct task_struct *tsk = current;
@@ -1600,13 +1606,6 @@ void kthread_use_mm(struct mm_struct *mm)
WARN_ON_ONCE(!(tsk->flags & PF_KTHREAD));
WARN_ON_ONCE(tsk->mm);

- /*
- * It is possible for mm to be the same as tsk->active_mm, but
- * we must still mmgrab(mm) and mmdrop_lazy_tlb(active_mm),
- * because these references are not equivalent.
- */
- mmgrab(mm);
-
task_lock(tsk);
/* Hold off tlb flush IPIs while switching mm's */
local_irq_disable();
@@ -1632,6 +1631,25 @@ void kthread_use_mm(struct mm_struct *mm)
*/
mmdrop_lazy_tlb(active_mm);
}
+EXPORT_SYMBOL_GPL(kthread_take_mm);
+
+/**
+ * kthread_use_mm - make the calling kthread operate on an address space.
+ *
+ * This must be paired with a call to kthread_unuse_mm().
+ *
+ * @mm: address space to operate on
+ */
+void kthread_use_mm(struct mm_struct *mm)
+{
+ /*
+ * It is possible for mm to be the same as tsk->active_mm, but we must
+ * still mmgrab(mm) and mmdrop_lazy_tlb(active_mm) (in
+ * kthread_take_mm()), because these references are not equivalent.
+ */
+ mmgrab(mm);
+ kthread_take_mm(mm);
+}
EXPORT_SYMBOL_GPL(kthread_use_mm);

/**

--
2.51.2

[Brendan Jackman]

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 <jack...@google.com>
---

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

[Brendan Jackman]

> Subject: Re: [PATCH 0/3] EDITME: cover title for b4/kunit-user-alloc

I hope everyone is suitably impressed by this. My talent for discovering
new ways of screwing up LKML submissions is simply unparalleled.

This should say "fix mm_struct leak in kunit_attach_mm()".