Re: [PATCH 1/2] kunit: test: Add vm_mmap() allocation resource manager

Mark Rutland
2024-05-20T09:29:14Z

On Sun, May 19, 2024 at 12:12:52PM -0700, Kees Cook wrote:
> For tests that need to allocate using vm_mmap() (e.g. usercopy and
> execve), provide the interface to have the allocation tracked by KUnit
> itself. This requires bringing up a placeholder userspace mm.
>
> This combines my earlier attempt at this with Mark Rutland's version[1].
>
> Link: https://lore.kernel.org/lkml/20230321122514.1743889-2-mark.rutland@arm.com/ [1]
> Co-developed-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/kunit/test.h | 17 ++++++
> lib/kunit/test.c | 139 ++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 155 insertions(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 61637ef32302..8c3835a6f282 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -478,6 +478,23 @@ static inline void *kunit_kcalloc(struct kunit *test, size_t n, size_t size, gfp
> return kunit_kmalloc_array(test, n, size, gfp | __GFP_ZERO);
> }
>
> +/**
> + * kunit_vm_mmap() - Allocate KUnit-tracked vm_mmap() area
> + * @test: The test context object.
> + * @file: struct file pointer to map from, if any
> + * @addr: desired address, if any
> + * @len: how many bytes to allocate
> + * @prot: mmap PROT_* bits
> + * @flag: mmap flags
> + * @offset: offset into @file to start mapping from.
> + *
> + * See vm_mmap() for more information.
> + */
> +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);
> +
> void kunit_cleanup(struct kunit *test);
>
> void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ...);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 1d1475578515..09194dbffb63 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -11,13 +11,14 @@
> #include <kunit/test-bug.h>
> #include <kunit/attributes.h>
> #include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/mm.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/mutex.h>
> #include <linux/panic.h>
> #include <linux/sched/debug.h>
> #include <linux/sched.h>
> -#include <linux/mm.h>
>
> #include "debugfs.h"
> #include "device-impl.h"
> @@ -871,6 +872,142 @@ void kunit_kfree(struct kunit *test, const void *ptr)
> }
> EXPORT_SYMBOL_GPL(kunit_kfree);
>
> +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;
> +};
> +
> +/*
> + * Arbitrarily chosen user address for the base allocation.
> + */
> +#define UBUF_ADDR_BASE SZ_2M
> +
> +/* Create and attach a new mm if it doesn't already exist. */
> +static int kunit_attach_mm(void)
> +{
> + struct vm_area_struct *vma;
> + struct mm_struct *mm;
> +
> + if (current->mm)
> + return 0;

My tests deliberately created/destroyed the mm for each test; surely we
don't want to inherit an MM in some arbitrary state? ... or is this just
so the mm can be allocated lazily upon the first mmap() within a test?

> +
> + mm = mm_alloc();
> + if (!mm)
> + return -ENOMEM;
> +
> + if (mmap_write_lock_killable(mm))
> + goto out_free;
> +
> + /* Define the task size. */
> + mm->task_size = TASK_SIZE;
> +
> + /* Prepare the base VMA. */
> + vma = vm_area_alloc(mm);
> + if (!vma)
> + goto out_unlock;
> +
> + vma_set_anonymous(vma);
> + vma->vm_start = UBUF_ADDR_BASE;
> + vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE;
> + vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE);
> + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> +
> + if (insert_vm_struct(mm, vma))
> + goto out_free_vma;
> +
> + mmap_write_unlock(mm);

Why do we need this VMA given you have kunit_vm_mmap()?

This existed in my uaccess tests because I didn't use vm_mmap(), and I
wanted complete control over the addresses used.

Given you add kunit_vm_mmap(), I don't think we want this VMA -- it
doesn't serve any real purpose to tests, and accesses can erroneously
hit it, which is problematic.

UBUF_ADDR_BASE shouldn't be necessary either with kunit_vm_mmap(),
unless you want to use fixed addresses. That was just arbitrarily chosen
to be above NULL and the usual minimum mmap limit.

Mark.

> +
> + /* Make sure we can allocate new VMAs. */
> + 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);
> +
> + return 0;
> +
> +out_free_vma:
> + vm_area_free(vma);
> +out_unlock:
> + mmap_write_unlock(mm);
> +out_free:
> + mmput(mm);
> + return -ENOMEM;
> +}
> +
> +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;
> +
> + vres = kunit_alloc_resource(test,
> + kunit_vm_mmap_init,
> + kunit_vm_mmap_free,
> + GFP_KERNEL,
> + &params);
> + if (vres)
> + return vres->addr;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_vm_mmap);
> +
> void kunit_cleanup(struct kunit *test)
> {
> struct kunit_resource *res;
> --
> 2.34.1
>