On Tue, Feb 14, 2023 at 2:17 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Tue, Feb 14, 2023 at 01:50:46PM -0800, Axel Rasmussen wrote:
> > UFFDIO_COPY already has UFFDIO_COPY_MODE_WP, so when installing a new
> > PTE to resolve a missing fault, one can install a write-protected one.
> > This is useful when using UFFDIO_REGISTER_MODE_{MISSING,WP} in
> > combination.
> >
> > So, add an analogous UFFDIO_CONTINUE_MODE_WP, which does the same thing
> > but for *minor* faults.
> >
> > Rename "wp_copy" arguments to "wp_mode", since the mode now applies
> > more widely than just to the copy operation.
>
> Speaking as someone not familiar with this code, i do not care for
> this rename. before, it says "bool wp_copy". i'm not sure what "wp"
> is, but "copy" i understand. so if it's true, i need to copy and false,
> i don't. now it's "bool wp_mode". so if it's true, it's a mode and if
> it's false it's not a mode? what?
Agree it's confusing. The "copy" refers to the "UFFDIO_COPY" ioctl,
which specifically allocates a new page, and *copies* its contents
from userspace. Continue, on the other hand, doesn't do any page
allocation or copying, it just installs a PTE for a page already in
the page cache. This is why I thought leaving the "copy" around would
be confusing.
Honestly there are other existing cases where this confusion remains;
e.g. the function mcopy_atomic doesn't necessarily do any page
allocation or copying, because the same code path also handles the
continue case.
Agree that at the very least wp_enabled or something would make more
sense, given it's a bool.
>
> stepping back for a second, i think the real problem is that the argument
> list has got out of control. 8 arguments to hugetlb_mcopy_atomic_pte(),
> the same # to shmem_mfill_atomic_pte(), a subtly different 8 to
> mfill_atomic_pte(). someone needs to create a struct with a sensible
> name to contain all of this. i bet it'll shrink the code too; marshalling
> all these arguments is unlikely to be cheap.
Agreed, it would likely be a nice cleanup. Peter, any objections? I
wouldn't mind writing a commit to do this sort of refactor, and rebase
my change on top of that.
>
> but more importantly, it lets you change the arguments without touching
> any of the code which is just passing them through. look at struct
> vm_fault for inspiration (you probably don't want to reuse it, but maybe
> you do?)
>
> > Update the selftest to do some very basic exercising of the new flag.
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
> > ---
> > fs/userfaultfd.c | 6 ++-
> > include/linux/userfaultfd_k.h | 3 +-
> > include/uapi/linux/userfaultfd.h | 7 ++++
> > mm/hugetlb.c | 6 +--
> > mm/shmem.c | 4 +-
> > mm/userfaultfd.c | 49 ++++++++++++++----------
> > tools/testing/selftests/vm/userfaultfd.c | 4 ++
> > 7 files changed, 51 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index cc694846617a..545dc033eec8 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -1910,13 +1910,15 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg)
> > uffdio_continue.range.start) {
> > goto out;
> > }
> > - if (uffdio_continue.mode & ~UFFDIO_CONTINUE_MODE_DONTWAKE)
> > + if (uffdio_continue.mode & ~(UFFDIO_CONTINUE_MODE_DONTWAKE |
> > + UFFDIO_CONTINUE_MODE_WP))
> > goto out;
> >
> > if (mmget_not_zero(ctx->mm)) {
> > ret = mcopy_continue(ctx->mm, uffdio_continue.range.start,
> > uffdio_continue.range.len,
> > - &ctx->mmap_changing);
> > + &ctx->mmap_changing,
> > + uffdio_continue.mode);
> > mmput(ctx->mm);
> > } else {
> > return -ESRCH;
> > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > index 9df0b9a762cc..a53aa56e78ad 100644
> > --- a/include/linux/userfaultfd_k.h
> > +++ b/include/linux/userfaultfd_k.h
> > @@ -69,7 +69,8 @@ extern ssize_t mfill_zeropage(struct mm_struct *dst_mm,
> > unsigned long len,
> > atomic_t *mmap_changing);
> > extern ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long dst_start,
> > - unsigned long len, atomic_t *mmap_changing);
> > + unsigned long len, atomic_t *mmap_changing,
> > + __u64 mode);
> > extern int mwriteprotect_range(struct mm_struct *dst_mm,
> > unsigned long start, unsigned long len,
> > bool enable_wp, atomic_t *mmap_changing);
> > diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h
> > index 005e5e306266..14059a0861bf 100644
> > --- a/include/uapi/linux/userfaultfd.h
> > +++ b/include/uapi/linux/userfaultfd.h
> > @@ -297,6 +297,13 @@ struct uffdio_writeprotect {
> > struct uffdio_continue {
> > struct uffdio_range range;
> > #define UFFDIO_CONTINUE_MODE_DONTWAKE ((__u64)1<<0)
> > + /*
> > + * UFFDIO_CONTINUE_MODE_WP will map the page write protected on
> > + * the fly. UFFDIO_CONTINUE_MODE_WP is available only if the
> > + * write protected ioctl is implemented for the range
> > + * according to the uffdio_register.ioctls.
> > + */
> > +#define UFFDIO_CONTINUE_MODE_WP ((__u64)1<<1)
> > __u64 mode;
> >
> > /*
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index bdbfeb6fb393..a1bd0b0c6c43 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -6169,7 +6169,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > unsigned long src_addr,
> > enum mcopy_atomic_mode mode,
> > struct page **pagep,
> > - bool wp_copy)
> > + bool wp_mode)
> > {
> > bool is_continue = (mode == MCOPY_ATOMIC_CONTINUE);
> > struct hstate *h = hstate_vma(dst_vma);
> > @@ -6306,7 +6306,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > * For either: (1) CONTINUE on a non-shared VMA, or (2) UFFDIO_COPY
> > * with wp flag set, don't set pte write bit.
> > */
> > - if (wp_copy || (is_continue && !vm_shared))
> > + if (wp_mode || (is_continue && !vm_shared))
> > writable = 0;
> > else
> > writable = dst_vma->vm_flags & VM_WRITE;
> > @@ -6321,7 +6321,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> > _dst_pte = huge_pte_mkdirty(_dst_pte);
> > _dst_pte = pte_mkyoung(_dst_pte);
> >
> > - if (wp_copy)
> > + if (wp_mode)
> > _dst_pte = huge_pte_mkuffd_wp(_dst_pte);
> >
> > set_huge_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 0005ab2c29af..725e5d5d85ab 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2402,7 +2402,7 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm,
> > struct vm_area_struct *dst_vma,
> > unsigned long dst_addr,
> > unsigned long src_addr,
> > - bool zeropage, bool wp_copy,
> > + bool zeropage, bool wp_mode,
> > struct page **pagep)
> > {
> > struct inode *inode = file_inode(dst_vma->vm_file);
> > @@ -2493,7 +2493,7 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm,
> > goto out_release;
> >
> > ret = mfill_atomic_install_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
> > - &folio->page, true, wp_copy);
> > + &folio->page, true, wp_mode);
> > if (ret)
> > goto out_delete_from_cache;
> >
> > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > index 0499907b6f1a..2ad4dcf72968 100644
> > --- a/mm/userfaultfd.c
> > +++ b/mm/userfaultfd.c
> > @@ -58,7 +58,7 @@ struct vm_area_struct *find_dst_vma(struct mm_struct *dst_mm,
> > int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
> > struct vm_area_struct *dst_vma,
> > unsigned long dst_addr, struct page *page,
> > - bool newly_allocated, bool wp_copy)
> > + bool newly_allocated, bool wp_mode)
> > {
> > int ret;
> > pte_t _dst_pte, *dst_pte;
> > @@ -79,7 +79,7 @@ int mfill_atomic_install_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd,
> > * Always mark a PTE as write-protected when needed, regardless of
> > * VM_WRITE, which the user might change.
> > */
> > - if (wp_copy) {
> > + if (wp_mode) {
> > _dst_pte = pte_mkuffd_wp(_dst_pte);
> > writable = false;
> > }
> > @@ -147,7 +147,7 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
> > unsigned long dst_addr,
> > unsigned long src_addr,
> > struct page **pagep,
> > - bool wp_copy)
> > + bool wp_mode)
> > {
> > void *page_kaddr;
> > int ret;
> > @@ -208,7 +208,7 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
> > goto out_release;
> >
> > ret = mfill_atomic_install_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
> > - page, true, wp_copy);
> > + page, true, wp_mode);
> > if (ret)
> > goto out_release;
> > out:
> > @@ -258,7 +258,7 @@ static int mcontinue_atomic_pte(struct mm_struct *dst_mm,
> > pmd_t *dst_pmd,
> > struct vm_area_struct *dst_vma,
> > unsigned long dst_addr,
> > - bool wp_copy)
> > + bool wp_mode)
> > {
> > struct inode *inode = file_inode(dst_vma->vm_file);
> > pgoff_t pgoff = linear_page_index(dst_vma, dst_addr);
> > @@ -284,7 +284,7 @@ static int mcontinue_atomic_pte(struct mm_struct *dst_mm,
> > }
> >
> > ret = mfill_atomic_install_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
> > - page, false, wp_copy);
> > + page, false, wp_mode);
> > if (ret)
> > goto out_release;
> >
> > @@ -330,7 +330,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
> > unsigned long src_start,
> > unsigned long len,
> > enum mcopy_atomic_mode mode,
> > - bool wp_copy)
> > + bool wp_mode)
> > {
> > int vm_shared = dst_vma->vm_flags & VM_SHARED;
> > ssize_t err;
> > @@ -427,7 +427,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
> >
> > err = hugetlb_mcopy_atomic_pte(dst_mm, dst_pte, dst_vma,
> > dst_addr, src_addr, mode, &page,
> > - wp_copy);
> > + wp_mode);
> >
> > hugetlb_vma_unlock_read(dst_vma);
> > mutex_unlock(&hugetlb_fault_mutex_table[hash]);
> > @@ -483,7 +483,7 @@ extern ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
> > unsigned long src_start,
> > unsigned long len,
> > enum mcopy_atomic_mode mode,
> > - bool wp_copy);
> > + bool wp_mode);
> > #endif /* CONFIG_HUGETLB_PAGE */
> >
> > static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,
> > @@ -493,13 +493,13 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,
> > unsigned long src_addr,
> > struct page **page,
> > enum mcopy_atomic_mode mode,
> > - bool wp_copy)
> > + bool wp_mode)
> > {
> > ssize_t err;
> >
> > if (mode == MCOPY_ATOMIC_CONTINUE) {
> > return mcontinue_atomic_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
> > - wp_copy);
> > + wp_mode);
> > }
> >
> > /*
> > @@ -516,7 +516,7 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,
> > if (mode == MCOPY_ATOMIC_NORMAL)
> > err = mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma,
> > dst_addr, src_addr, page,
> > - wp_copy);
> > + wp_mode);
> > else
> > err = mfill_zeropage_pte(dst_mm, dst_pmd,
> > dst_vma, dst_addr);
> > @@ -524,12 +524,21 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,
> > err = shmem_mfill_atomic_pte(dst_mm, dst_pmd, dst_vma,
> > dst_addr, src_addr,
> > mode != MCOPY_ATOMIC_NORMAL,
> > - wp_copy, page);
> > + wp_mode, page);
> > }
> >
> > return err;
> > }
> >
> > +static inline bool wp_mode_enabled(enum mcopy_atomic_mode mcopy_mode, __u64 mode)
> > +{
> > + switch (mode) {
> > + case MCOPY_ATOMIC_NORMAL: return mode & UFFDIO_COPY_MODE_WP;
> > + case MCOPY_ATOMIC_CONTINUE: return mode & UFFDIO_CONTINUE_MODE_WP;
> > + default: return false;
> > + }
> > +}
> > +
> > static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
> > unsigned long dst_start,
> > unsigned long src_start,
> > @@ -544,7 +553,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
> > unsigned long src_addr, dst_addr;
> > long copied;
> > struct page *page;
> > - bool wp_copy;
> > + bool wp_mode;
> >
> > /*
> > * Sanitize the command parameters:
> > @@ -594,8 +603,8 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
> > * validate 'mode' now that we know the dst_vma: don't allow
> > * a wrprotect copy if the userfaultfd didn't register as WP.
> > */
> > - wp_copy = mode & UFFDIO_COPY_MODE_WP;
> > - if (wp_copy && !(dst_vma->vm_flags & VM_UFFD_WP))
> > + wp_mode = wp_mode_enabled(mcopy_mode, mode);
> > + if (wp_mode && !(dst_vma->vm_flags & VM_UFFD_WP))
> > goto out_unlock;
> >
> > /*
> > @@ -604,7 +613,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
> > if (is_vm_hugetlb_page(dst_vma))
> > return __mcopy_atomic_hugetlb(dst_mm, dst_vma, dst_start,
> > src_start, len, mcopy_mode,
> > - wp_copy);
> > + wp_mode);
> >
> > if (!vma_is_anonymous(dst_vma) && !vma_is_shmem(dst_vma))
> > goto out_unlock;
> > @@ -656,7 +665,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
> > BUG_ON(pmd_trans_huge(*dst_pmd));
> >
> > err = mfill_atomic_pte(dst_mm, dst_pmd, dst_vma, dst_addr,
> > - src_addr, &page, mcopy_mode, wp_copy);
> > + src_addr, &page, mcopy_mode, wp_mode);
> > cond_resched();
> >
> > if (unlikely(err == -ENOENT)) {
> > @@ -718,10 +727,10 @@ ssize_t mfill_zeropage(struct mm_struct *dst_mm, unsigned long start,
> > }
> >
> > ssize_t mcopy_continue(struct mm_struct *dst_mm, unsigned long start,
> > - unsigned long len, atomic_t *mmap_changing)
> > + unsigned long len, atomic_t *mmap_changing, __u64 mode)
> > {
> > return __mcopy_atomic(dst_mm, start, 0, len, MCOPY_ATOMIC_CONTINUE,
> > - mmap_changing, 0);
> > + mmap_changing, mode);
> > }
> >
> > void uffd_wp_range(struct mm_struct *dst_mm, struct vm_area_struct *dst_vma,
> > diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> > index 7f22844ed704..41c1f9abc481 100644
> > --- a/tools/testing/selftests/vm/userfaultfd.c
> > +++ b/tools/testing/selftests/vm/userfaultfd.c
> > @@ -585,6 +585,8 @@ static void continue_range(int ufd, __u64 start, __u64 len)
> > req.range.start = start;
> > req.range.len = len;
> > req.mode = 0;
> > + if (test_uffdio_wp)
> > + req.mode |= UFFDIO_CONTINUE_MODE_WP;
> >
> > if (ioctl(ufd, UFFDIO_CONTINUE, &req))
> > err("UFFDIO_CONTINUE failed for address 0x%" PRIx64,
> > @@ -1332,6 +1334,8 @@ static int userfaultfd_minor_test(void)
> > uffdio_register.range.start = (unsigned long)area_dst_alias;
> > uffdio_register.range.len = nr_pages * page_size;
> > uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR;
> > + if (test_uffdio_wp)
> > + uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
> > if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
> > err("register failure");
> >
> > --
> > 2.39.1.581.gbfd45094c4-goog
> >
> >
