Re: [PATCH v3 4/5] mm: userfaultfd: don't separate addr + len arguments

On Mon, Mar 06, 2023 at 02:50:23PM -0800, Axel Rasmussen wrote:

We have a lot of functions which take an address + length pair, currently passed as separate arguments. However, in our userspace API we already have struct uffdio_range, which is exactly this pair, and this is what we get from userspace when ioctls are called.

Instead of splitting the struct up into two separate arguments, just plumb the struct through to the functions which use it (once we get to the mfill_atomic_pte level, we're dealing with single (huge)pages, so we don't need both parts).

Relatedly, for waking, just re-use this existing structure instead of defining a new "struct uffdio_wake_range".

Signed-off-by: Axel Rasmussen axelrasmussen@google.com

fs/userfaultfd.c | 107 +++++++++++++--------------------- include/linux/userfaultfd_k.h | 17 +++--- mm/userfaultfd.c | 92 ++++++++++++++--------------- 3 files changed, 96 insertions(+), 120 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index b8e328123b71..984b63b0fc75 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -95,11 +95,6 @@ struct userfaultfd_wait_queue { bool waken; }; -struct userfaultfd_wake_range {

• unsigned long start;
• unsigned long len;

-};

Would there still be a difference on e.g. 32 bits systems?

[...]

static __always_inline int validate_range(struct mm_struct *mm,

• 			  __u64 start, __u64 len)
  

• 			  const struct uffdio_range *range)
  

{ __u64 task_size = mm->task_size;

• if (start & ~PAGE_MASK)

• if (range->start & ~PAGE_MASK) return -EINVAL;

• if (len & ~PAGE_MASK)

• if (range->len & ~PAGE_MASK) return -EINVAL;

• if (!len)

• if (!range->len) return -EINVAL;

• if (start < mmap_min_addr)

• if (range->start < mmap_min_addr) return -EINVAL;

• if (start >= task_size)

• if (range->start >= task_size) return -EINVAL;

• if (len > task_size - start)

• if (range->len > task_size - range->start) return -EINVAL; return 0;

}

Personally I don't like a lot on such a change. :( It avoids one parameter being passed over but it can add a lot indirections.

Do you strongly suggest this? Shall we move on without this so to not block the last patch (which I assume is the one you're looking for)?

Thanks,