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.