On Mon, May 20, 2024 at 10:29:06AM +0100, Mark Rutland wrote:
On Sun, May 19, 2024 at 12:12:52PM -0700, Kees Cook wrote:
+/* 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?
It's for lazily creation and for supporting running the KUnit test as a module (where a userspace would exist). The old usercopy test worked against the existing userspace, so I'd want to continue to support that.
- 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()?
When I was originally testing this, it seemed like I couldn't perform a vm_mmap() without an existing VMA.
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.
I'll recheck whether this is needed. I think I had to make some other changes as well, so maybe something here ended up being redundant without my noticing it the first time.
Thanks for looking this over!