| Message ID | cd05b41d6d15ee9ff94273bc116ed3db3f5125bf.1681831798.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove the vmas parameter from GUP APIs | expand |
Am 18.04.23 um 17:49 schrieb Lorenzo Stoakes: > No invocation of get_user_pages() uses the vmas parameter, so remove > it. > > The GUP API is confusing and caveated. Recent changes have done much to > improve that, however there is more we can do. Exporting vmas is a prime > target as the caller has to be extremely careful to preclude their use > after the mmap_lock has expired or otherwise be left with dangling > pointers. > > Removing the vmas parameter focuses the GUP functions upon their primary > purpose - pinning (and outputting) pages as well as performing the actions > implied by the input flags. > > This is part of a patch series aiming to remove the vmas parameter > altogether. Astonishing that there are so few users of the original get_user_pages() API left. > > Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Acked-by: David Hildenbrand <david@redhat.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> Acked-by: Christian König <christian.koenig@amd.com> for the radeon parts. Regards, Christian. > --- > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- > drivers/misc/sgi-gru/grufault.c | 2 +- > include/linux/mm.h | 3 +-- > mm/gup.c | 9 +++------ > mm/gup_test.c | 5 ++--- > virt/kvm/kvm_main.c | 2 +- > 7 files changed, 10 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 21ca0a831b70..5d390df21440 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -214,7 +214,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, > if (!(vma->vm_flags & VM_MAYEXEC)) > return -EACCES; > > - ret = get_user_pages(src, 1, 0, &src_page, NULL); > + ret = get_user_pages(src, 1, 0, &src_page); > if (ret < 1) > return -EFAULT; > > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > index 1e8e287e113c..0597540f0dde 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -362,7 +362,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm > struct page **pages = ttm->pages + pinned; > > r = get_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0, > - pages, NULL); > + pages); > if (r < 0) > goto release_pages; > > diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c > index b836936e9747..378cf02a2aa1 100644 > --- a/drivers/misc/sgi-gru/grufault.c > +++ b/drivers/misc/sgi-gru/grufault.c > @@ -185,7 +185,7 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, > #else > *pageshift = PAGE_SHIFT; > #endif > - if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page, NULL) <= 0) > + if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page) <= 0) > return -EFAULT; > *paddr = page_to_phys(page); > put_page(page); > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 37554b08bb28..b14cc4972d0b 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2380,8 +2380,7 @@ long pin_user_pages_remote(struct mm_struct *mm, > unsigned int gup_flags, struct page **pages, > struct vm_area_struct **vmas, int *locked); > long get_user_pages(unsigned long start, unsigned long nr_pages, > - unsigned int gup_flags, struct page **pages, > - struct vm_area_struct **vmas); > + unsigned int gup_flags, struct page **pages); > long pin_user_pages(unsigned long start, unsigned long nr_pages, > unsigned int gup_flags, struct page **pages, > struct vm_area_struct **vmas); > diff --git a/mm/gup.c b/mm/gup.c > index 1f72a717232b..7e454d6b157e 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2251,8 +2251,6 @@ long get_user_pages_remote(struct mm_struct *mm, > * @pages: array that receives pointers to the pages pinned. > * Should be at least nr_pages long. Or NULL, if caller > * only intends to ensure the pages are faulted in. > - * @vmas: array of pointers to vmas corresponding to each page. > - * Or NULL if the caller does not require them. > * > * This is the same as get_user_pages_remote(), just with a less-flexible > * calling convention where we assume that the mm being operated on belongs to > @@ -2260,16 +2258,15 @@ long get_user_pages_remote(struct mm_struct *mm, > * obviously don't pass FOLL_REMOTE in here. > */ > long get_user_pages(unsigned long start, unsigned long nr_pages, > - unsigned int gup_flags, struct page **pages, > - struct vm_area_struct **vmas) > + unsigned int gup_flags, struct page **pages) > { > int locked = 1; > > - if (!is_valid_gup_args(pages, vmas, NULL, &gup_flags, FOLL_TOUCH)) > + if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, FOLL_TOUCH)) > return -EINVAL; > > return __get_user_pages_locked(current->mm, start, nr_pages, pages, > - vmas, &locked, gup_flags); > + NULL, &locked, gup_flags); > } > EXPORT_SYMBOL(get_user_pages); > > diff --git a/mm/gup_test.c b/mm/gup_test.c > index 8ae7307a1bb6..9ba8ea23f84e 100644 > --- a/mm/gup_test.c > +++ b/mm/gup_test.c > @@ -139,8 +139,7 @@ static int __gup_test_ioctl(unsigned int cmd, > pages + i); > break; > case GUP_BASIC_TEST: > - nr = get_user_pages(addr, nr, gup->gup_flags, pages + i, > - NULL); > + nr = get_user_pages(addr, nr, gup->gup_flags, pages + i); > break; > case PIN_FAST_BENCHMARK: > nr = pin_user_pages_fast(addr, nr, gup->gup_flags, > @@ -161,7 +160,7 @@ static int __gup_test_ioctl(unsigned int cmd, > pages + i, NULL); > else > nr = get_user_pages(addr, nr, gup->gup_flags, > - pages + i, NULL); > + pages + i); > break; > default: > ret = -EINVAL; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index d255964ec331..7f31e0a4adb5 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2474,7 +2474,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) > { > int rc, flags = FOLL_HWPOISON | FOLL_WRITE; > > - rc = get_user_pages(addr, 1, flags, NULL, NULL); > + rc = get_user_pages(addr, 1, flags, NULL); > return rc == -EHWPOISON; > } >
On Tue Apr 18, 2023 at 6:49 PM EEST, Lorenzo Stoakes wrote: > No invocation of get_user_pages() uses the vmas parameter, so remove > it. > > The GUP API is confusing and caveated. Recent changes have done much to > improve that, however there is more we can do. Exporting vmas is a prime > target as the caller has to be extremely careful to preclude their use > after the mmap_lock has expired or otherwise be left with dangling > pointers. > > Removing the vmas parameter focuses the GUP functions upon their primary > purpose - pinning (and outputting) pages as well as performing the actions > implied by the input flags. > > This is part of a patch series aiming to remove the vmas parameter > altogether. > > Suggested-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Acked-by: David Hildenbrand <david@redhat.com> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> > --- > arch/x86/kernel/cpu/sgx/ioctl.c | 2 +- > drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- > drivers/misc/sgi-gru/grufault.c | 2 +- > include/linux/mm.h | 3 +-- > mm/gup.c | 9 +++------ > mm/gup_test.c | 5 ++--- > virt/kvm/kvm_main.c | 2 +- > 7 files changed, 10 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c > index 21ca0a831b70..5d390df21440 100644 > --- a/arch/x86/kernel/cpu/sgx/ioctl.c > +++ b/arch/x86/kernel/cpu/sgx/ioctl.c > @@ -214,7 +214,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, > if (!(vma->vm_flags & VM_MAYEXEC)) > return -EACCES; > > - ret = get_user_pages(src, 1, 0, &src_page, NULL); > + ret = get_user_pages(src, 1, 0, &src_page); > if (ret < 1) > return -EFAULT; > > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c > index 1e8e287e113c..0597540f0dde 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -362,7 +362,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm > struct page **pages = ttm->pages + pinned; > > r = get_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0, > - pages, NULL); > + pages); > if (r < 0) > goto release_pages; > > diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c > index b836936e9747..378cf02a2aa1 100644 > --- a/drivers/misc/sgi-gru/grufault.c > +++ b/drivers/misc/sgi-gru/grufault.c > @@ -185,7 +185,7 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, > #else > *pageshift = PAGE_SHIFT; > #endif > - if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page, NULL) <= 0) > + if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page) <= 0) > return -EFAULT; > *paddr = page_to_phys(page); > put_page(page); > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 37554b08bb28..b14cc4972d0b 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -2380,8 +2380,7 @@ long pin_user_pages_remote(struct mm_struct *mm, > unsigned int gup_flags, struct page **pages, > struct vm_area_struct **vmas, int *locked); > long get_user_pages(unsigned long start, unsigned long nr_pages, > - unsigned int gup_flags, struct page **pages, > - struct vm_area_struct **vmas); > + unsigned int gup_flags, struct page **pages); > long pin_user_pages(unsigned long start, unsigned long nr_pages, > unsigned int gup_flags, struct page **pages, > struct vm_area_struct **vmas); > diff --git a/mm/gup.c b/mm/gup.c > index 1f72a717232b..7e454d6b157e 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2251,8 +2251,6 @@ long get_user_pages_remote(struct mm_struct *mm, > * @pages: array that receives pointers to the pages pinned. > * Should be at least nr_pages long. Or NULL, if caller > * only intends to ensure the pages are faulted in. > - * @vmas: array of pointers to vmas corresponding to each page. > - * Or NULL if the caller does not require them. > * > * This is the same as get_user_pages_remote(), just with a less-flexible > * calling convention where we assume that the mm being operated on belongs to > @@ -2260,16 +2258,15 @@ long get_user_pages_remote(struct mm_struct *mm, > * obviously don't pass FOLL_REMOTE in here. > */ > long get_user_pages(unsigned long start, unsigned long nr_pages, > - unsigned int gup_flags, struct page **pages, > - struct vm_area_struct **vmas) > + unsigned int gup_flags, struct page **pages) > { > int locked = 1; > > - if (!is_valid_gup_args(pages, vmas, NULL, &gup_flags, FOLL_TOUCH)) > + if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, FOLL_TOUCH)) > return -EINVAL; > > return __get_user_pages_locked(current->mm, start, nr_pages, pages, > - vmas, &locked, gup_flags); > + NULL, &locked, gup_flags); > } > EXPORT_SYMBOL(get_user_pages); > > diff --git a/mm/gup_test.c b/mm/gup_test.c > index 8ae7307a1bb6..9ba8ea23f84e 100644 > --- a/mm/gup_test.c > +++ b/mm/gup_test.c > @@ -139,8 +139,7 @@ static int __gup_test_ioctl(unsigned int cmd, > pages + i); > break; > case GUP_BASIC_TEST: > - nr = get_user_pages(addr, nr, gup->gup_flags, pages + i, > - NULL); > + nr = get_user_pages(addr, nr, gup->gup_flags, pages + i); > break; > case PIN_FAST_BENCHMARK: > nr = pin_user_pages_fast(addr, nr, gup->gup_flags, > @@ -161,7 +160,7 @@ static int __gup_test_ioctl(unsigned int cmd, > pages + i, NULL); > else > nr = get_user_pages(addr, nr, gup->gup_flags, > - pages + i, NULL); > + pages + i); > break; > default: > ret = -EINVAL; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index d255964ec331..7f31e0a4adb5 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2474,7 +2474,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) > { > int rc, flags = FOLL_HWPOISON | FOLL_WRITE; > > - rc = get_user_pages(addr, 1, flags, NULL, NULL); > + rc = get_user_pages(addr, 1, flags, NULL); > return rc == -EHWPOISON; > } > > -- > 2.40.0 Acked-by: Jarkko Sakkinen <jarkko@kernel.org> BR, Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/ioctl.c b/arch/x86/kernel/cpu/sgx/ioctl.c index 21ca0a831b70..5d390df21440 100644 --- a/arch/x86/kernel/cpu/sgx/ioctl.c +++ b/arch/x86/kernel/cpu/sgx/ioctl.c @@ -214,7 +214,7 @@ static int __sgx_encl_add_page(struct sgx_encl *encl, if (!(vma->vm_flags & VM_MAYEXEC)) return -EACCES; - ret = get_user_pages(src, 1, 0, &src_page, NULL); + ret = get_user_pages(src, 1, 0, &src_page); if (ret < 1) return -EFAULT; diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 1e8e287e113c..0597540f0dde 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -362,7 +362,7 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_device *bdev, struct ttm_tt *ttm struct page **pages = ttm->pages + pinned; r = get_user_pages(userptr, num_pages, write ? FOLL_WRITE : 0, - pages, NULL); + pages); if (r < 0) goto release_pages; diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c index b836936e9747..378cf02a2aa1 100644 --- a/drivers/misc/sgi-gru/grufault.c +++ b/drivers/misc/sgi-gru/grufault.c @@ -185,7 +185,7 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, #else *pageshift = PAGE_SHIFT; #endif - if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page, NULL) <= 0) + if (get_user_pages(vaddr, 1, write ? FOLL_WRITE : 0, &page) <= 0) return -EFAULT; *paddr = page_to_phys(page); put_page(page); diff --git a/include/linux/mm.h b/include/linux/mm.h index 37554b08bb28..b14cc4972d0b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -2380,8 +2380,7 @@ long pin_user_pages_remote(struct mm_struct *mm, unsigned int gup_flags, struct page **pages, struct vm_area_struct **vmas, int *locked); long get_user_pages(unsigned long start, unsigned long nr_pages, - unsigned int gup_flags, struct page **pages, - struct vm_area_struct **vmas); + unsigned int gup_flags, struct page **pages); long pin_user_pages(unsigned long start, unsigned long nr_pages, unsigned int gup_flags, struct page **pages, struct vm_area_struct **vmas); diff --git a/mm/gup.c b/mm/gup.c index 1f72a717232b..7e454d6b157e 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2251,8 +2251,6 @@ long get_user_pages_remote(struct mm_struct *mm, * @pages: array that receives pointers to the pages pinned. * Should be at least nr_pages long. Or NULL, if caller * only intends to ensure the pages are faulted in. - * @vmas: array of pointers to vmas corresponding to each page. - * Or NULL if the caller does not require them. * * This is the same as get_user_pages_remote(), just with a less-flexible * calling convention where we assume that the mm being operated on belongs to @@ -2260,16 +2258,15 @@ long get_user_pages_remote(struct mm_struct *mm, * obviously don't pass FOLL_REMOTE in here. */ long get_user_pages(unsigned long start, unsigned long nr_pages, - unsigned int gup_flags, struct page **pages, - struct vm_area_struct **vmas) + unsigned int gup_flags, struct page **pages) { int locked = 1; - if (!is_valid_gup_args(pages, vmas, NULL, &gup_flags, FOLL_TOUCH)) + if (!is_valid_gup_args(pages, NULL, NULL, &gup_flags, FOLL_TOUCH)) return -EINVAL; return __get_user_pages_locked(current->mm, start, nr_pages, pages, - vmas, &locked, gup_flags); + NULL, &locked, gup_flags); } EXPORT_SYMBOL(get_user_pages); diff --git a/mm/gup_test.c b/mm/gup_test.c index 8ae7307a1bb6..9ba8ea23f84e 100644 --- a/mm/gup_test.c +++ b/mm/gup_test.c @@ -139,8 +139,7 @@ static int __gup_test_ioctl(unsigned int cmd, pages + i); break; case GUP_BASIC_TEST: - nr = get_user_pages(addr, nr, gup->gup_flags, pages + i, - NULL); + nr = get_user_pages(addr, nr, gup->gup_flags, pages + i); break; case PIN_FAST_BENCHMARK: nr = pin_user_pages_fast(addr, nr, gup->gup_flags, @@ -161,7 +160,7 @@ static int __gup_test_ioctl(unsigned int cmd, pages + i, NULL); else nr = get_user_pages(addr, nr, gup->gup_flags, - pages + i, NULL); + pages + i); break; default: ret = -EINVAL; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index d255964ec331..7f31e0a4adb5 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2474,7 +2474,7 @@ static inline int check_user_page_hwpoison(unsigned long addr) { int rc, flags = FOLL_HWPOISON | FOLL_WRITE; - rc = get_user_pages(addr, 1, flags, NULL, NULL); + rc = get_user_pages(addr, 1, flags, NULL); return rc == -EHWPOISON; }