On Tue, Jan 17, 2023 at 11:58:32AM -0400, Jason Gunthorpe wrote:
> __get_user_pages_locked() and __gup_longterm_locked() both require the
> mmap lock to be held. They have a slightly unusual locked parameter that
> is used to allow these functions to unlock and relock the mmap lock and
> convey that fact to the caller.
>
> Several places wrapper these functions with a simple mmap_read_lock() just
Nit: ^wrap
> so they can follow the optimized locked protocol.
>
> Consolidate this internally to the functions. Allow internal callers to
> set locked = 0 to cause the functions to obtain and release the lock on
I'd s/obtain/acquire/g
> their own.
>
> Reorganize __gup_longterm_locked() to use the autolocking in
> __get_user_pages_locked().
>
> Replace all the places obtaining the mmap_read_lock() just to call
> __get_user_pages_locked() with the new mechanism. Replace all the internal
> callers of get_user_pages_unlocked() with direct calls to
> __gup_longterm_locked() using the new mechanism.
>
> A following patch will add assertions ensuring the external interface
> continues to always pass in locked = 1.
>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> mm/gup.c | 92 +++++++++++++++++++++++++++++---------------------------
> 1 file changed, 47 insertions(+), 45 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index f45a3a5be53a48..3a9f764165f50b 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1343,13 +1343,22 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
> unsigned int flags)
> {
> long ret, pages_done;
> - bool lock_dropped;
> + bool lock_dropped = false;
>
> if (locked) {
> /* if VM_FAULT_RETRY can be returned, vmas become invalid */
> BUG_ON(vmas);
> - /* check caller initialized locked */
> - BUG_ON(*locked != 1);
> + }
> +
> + /*
> + * The internal caller expects GUP to manage the lock internally and the
> + * lock must be released when this returns.
> + */
> + if (locked && !*locked) {
> + if (mmap_read_lock_killable(mm))
> + return -EAGAIN;
> + lock_dropped = true;
> + *locked = 1;
> }
>
> if (flags & FOLL_PIN)
> @@ -1368,7 +1377,6 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
> flags |= FOLL_GET;
>
> pages_done = 0;
> - lock_dropped = false;
> for (;;) {
> ret = __get_user_pages(mm, start, nr_pages, flags, pages,
> vmas, locked);
> @@ -1659,9 +1667,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> unsigned int foll_flags)
> {
> struct vm_area_struct *vma;
> + bool must_unlock = false;
> unsigned long vm_flags;
> long i;
>
> + if (!nr_pages)
> + return 0;
> +
> + /*
> + * The internal caller expects GUP to manage the lock internally and the
> + * lock must be released when this returns.
> + */
> + if (locked && !*locked) {
> + if (mmap_read_lock_killable(mm))
> + return -EAGAIN;
> + must_unlock = true;
> + *locked = 1;
> + }
> +
> /* calculate required read or write permissions.
> * If FOLL_FORCE is set, we only require the "MAY" flags.
> */
> @@ -1673,12 +1696,12 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> for (i = 0; i < nr_pages; i++) {
> vma = find_vma(mm, start);
> if (!vma)
> - goto finish_or_fault;
> + break;
>
> /* protect what we can, including chardevs */
> if ((vma->vm_flags & (VM_IO | VM_PFNMAP)) ||
> !(vm_flags & vma->vm_flags))
> - goto finish_or_fault;
> + break;
>
> if (pages) {
> pages[i] = virt_to_page((void *)start);
> @@ -1690,9 +1713,11 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> start = (start + PAGE_SIZE) & PAGE_MASK;
> }
>
> - return i;
> + if (must_unlock && *locked) {
> + mmap_read_unlock(mm);
> + *locked = 0;
> + }
>
> -finish_or_fault:
> return i ? : -EFAULT;
> }
> #endif /* !CONFIG_MMU */
> @@ -1861,17 +1886,13 @@ EXPORT_SYMBOL(fault_in_readable);
> #ifdef CONFIG_ELF_CORE
> struct page *get_dump_page(unsigned long addr)
> {
> - struct mm_struct *mm = current->mm;
> struct page *page;
> - int locked = 1;
> + int locked = 0;
> int ret;
>
> - if (mmap_read_lock_killable(mm))
> - return NULL;
> - ret = __get_user_pages_locked(mm, addr, 1, &page, NULL, &locked,
> + ret = __get_user_pages_locked(current->mm, addr, 1, &page, NULL,
> + &locked,
> FOLL_FORCE | FOLL_DUMP | FOLL_GET);
> - if (locked)
> - mmap_read_unlock(mm);
> return (ret == 1) ? page : NULL;
> }
> #endif /* CONFIG_ELF_CORE */
> @@ -2047,13 +2068,9 @@ static long __gup_longterm_locked(struct mm_struct *mm,
> int *locked,
> unsigned int gup_flags)
> {
> - bool must_unlock = false;
> unsigned int flags;
> long rc, nr_pinned_pages;
>
> - if (locked && WARN_ON_ONCE(!*locked))
> - return -EINVAL;
> -
> if (!(gup_flags & FOLL_LONGTERM))
> return __get_user_pages_locked(mm, start, nr_pages, pages, vmas,
> locked, gup_flags);
> @@ -2070,11 +2087,6 @@ static long __gup_longterm_locked(struct mm_struct *mm,
> return -EINVAL;
> flags = memalloc_pin_save();
> do {
> - if (locked && !*locked) {
> - mmap_read_lock(mm);
> - must_unlock = true;
> - *locked = 1;
> - }
> nr_pinned_pages = __get_user_pages_locked(mm, start, nr_pages,
> pages, vmas, locked,
> gup_flags);
> @@ -2085,11 +2097,6 @@ static long __gup_longterm_locked(struct mm_struct *mm,
> rc = check_and_migrate_movable_pages(nr_pinned_pages, pages);
> } while (rc == -EAGAIN);
> memalloc_pin_restore(flags);
> -
> - if (locked && *locked && must_unlock) {
> - mmap_read_unlock(mm);
> - *locked = 0;
> - }
> return rc ? rc : nr_pinned_pages;
> }
>
> @@ -2242,16 +2249,10 @@ EXPORT_SYMBOL(get_user_pages);
> long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> struct page **pages, unsigned int gup_flags)
> {
> - struct mm_struct *mm = current->mm;
> - int locked = 1;
> - long ret;
> + int locked = 0;
>
> - mmap_read_lock(mm);
> - ret = __gup_longterm_locked(mm, start, nr_pages, pages, NULL, &locked,
> - gup_flags | FOLL_TOUCH);
> - if (locked)
> - mmap_read_unlock(mm);
> - return ret;
> + return __gup_longterm_locked(current->mm, start, nr_pages, pages, NULL,
> + &locked, gup_flags | FOLL_TOUCH);
> }
> EXPORT_SYMBOL(get_user_pages_unlocked);
>
> @@ -2904,6 +2905,7 @@ static int internal_get_user_pages_fast(unsigned long start,
> {
> unsigned long len, end;
> unsigned long nr_pinned;
> + int locked = 0;
> int ret;
>
> if (WARN_ON_ONCE(gup_flags & ~(FOLL_WRITE | FOLL_LONGTERM |
> @@ -2932,8 +2934,9 @@ static int internal_get_user_pages_fast(unsigned long start,
> /* Slow path: try to get the remaining pages with get_user_pages */
> start += nr_pinned << PAGE_SHIFT;
> pages += nr_pinned;
> - ret = get_user_pages_unlocked(start, nr_pages - nr_pinned, pages,
> - gup_flags);
> + ret = __gup_longterm_locked(current->mm, start, nr_pages - nr_pinned,
> + pages, NULL, &locked,
> + gup_flags | FOLL_TOUCH);
> if (ret < 0) {
> /*
> * The caller has to unpin the pages we already pinned so
> @@ -3180,14 +3183,13 @@ EXPORT_SYMBOL(pin_user_pages);
> long pin_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
> struct page **pages, unsigned int gup_flags)
> {
> - /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> - if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> - return -EINVAL;
> + int locked = 0;
>
> if (WARN_ON_ONCE(!pages))
> return -EINVAL;
>
> - gup_flags |= FOLL_PIN;
> - return get_user_pages_unlocked(start, nr_pages, pages, gup_flags);
> + gup_flags |= FOLL_PIN | FOLL_TOUCH;
> + return __gup_longterm_locked(current->mm, start, nr_pages, pages, NULL,
> + &locked, gup_flags);
> }
> EXPORT_SYMBOL(pin_user_pages_unlocked);
> --
> 2.39.0
>
>
--
Sincerely yours,
Mike.
