| Message ID | 20240519191254.651865-1-keescook@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | usercopy: Convert test_user_copy to KUnit test | expand |
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, ¤t->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, > + ¶ms); > + 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 >
On Mon, 20 May 2024 at 03:12, Kees Cook <keescook@chromium.org> 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> > --- Thanks very much for this! A few high-level thoughts: - Do we want to move this into a separate file? test.{c,h} is already getting pretty big, and this would probably fit more comfortably with some of the resource-management bits, which are in their own files. Not every test will need mm support. - It'd be nice for there to be a way to explicitly teardown/reset this: I agree that this is made more awkward by KUnit cleanup normally running on a different thread, but I could definitely see why a test might want to unset/reset this, and it would be more consistent with other resources. Otherwise, I have a few small questions below, but nothing essential. There are a couple of test failures/hangs for the usercopy test (on i386 and m68k), which may have origins here: I've mentioned them there. Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > 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 Are there any circumstances where we'd want a _different_ base address here? Could it conflict with something / could tests require something different? I suspect it's fine to leave it like this until such a case actually shows up. > + > +/* 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; > + > + 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); > + > + /* Make sure we can allocate new VMAs. */ > + arch_pick_mmap_layout(mm, ¤t->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, > + ¶ms); It could be easier to use kunit_add_action() here, rather than kunit_alloc_resource(), as you wouldn't need the params struct to pass things through. The advantage to keeping the separate resource is that we can more easily look it up later if we, for example, wanted to be able to make it current on other threads (is that something we'd ever want to do?). > + 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 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240519191254.651865-1-keescook%40chromium.org.
On Sat, Jun 08, 2024 at 04:44:16PM +0800, David Gow wrote: > On Mon, 20 May 2024 at 03:12, Kees Cook <keescook@chromium.org> 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> > > --- > > Thanks very much for this! > > A few high-level thoughts: > - Do we want to move this into a separate file? test.{c,h} is already > getting pretty big, and this would probably fit more comfortably with > some of the resource-management bits, which are in their own files. > Not every test will need mm support. I'm happy to do that -- I was just following where kunit_kmalloc() was defined. I'll create a new file for it. > - It'd be nice for there to be a way to explicitly teardown/reset > this: I agree that this is made more awkward by KUnit cleanup normally > running on a different thread, but I could definitely see why a test > might want to unset/reset this, and it would be more consistent with > other resources. Yeah, it's weird, but it's naturally managed? > Otherwise, I have a few small questions below, but nothing essential. > There are a couple of test failures/hangs for the usercopy test (on > i386 and m68k), which may have origins here: I've mentioned them > there. I'll look into this. I must have some 64/32 oversight... > Reviewed-by: David Gow <davidgow@google.com> Thanks! > > +/* > > + * Arbitrarily chosen user address for the base allocation. > > + */ > > +#define UBUF_ADDR_BASE SZ_2M > > Are there any circumstances where we'd want a _different_ base address > here? Could it conflict with something / could tests require something > different? > > I suspect it's fine to leave it like this until such a case actually shows up. Yeah, it shouldn't be important, and as Mark has pointed out, it might not be needed at all. I'll see what I can do. > > + vres = kunit_alloc_resource(test, > > + kunit_vm_mmap_init, > > + kunit_vm_mmap_free, > > + GFP_KERNEL, > > + ¶ms); > > It could be easier to use kunit_add_action() here, rather than > kunit_alloc_resource(), as you wouldn't need the params struct to pass > things through. > > The advantage to keeping the separate resource is that we can more > easily look it up later if we, for example, wanted to be able to make > it current on other threads (is that something we'd ever want to do?). I like having it follow the pattern of the other resource allocators, but if there's not a strong reason to switch, I'll leave it as-is.
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; + + 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); + + /* Make sure we can allocate new VMAs. */ + arch_pick_mmap_layout(mm, ¤t->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, + ¶ms); + if (vres) + return vres->addr; + return 0; +} +EXPORT_SYMBOL_GPL(kunit_vm_mmap); + void kunit_cleanup(struct kunit *test) { struct kunit_resource *res;