remove the vmas parameter from GUP APIs

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) <[email protected]>

Acked-by Greg Kroah-Hartman <[email protected]>

Acked-by David Hildenbrand <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Lorenzo Stoakes <[email protected]>

---
  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(-)

arch/x86/kernel/cpu/sgx/ioctl.c +1 -1

index 21ca0a831b70..5d390df21440 100644
@@ -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;
 

drivers/gpu/drm/radeon/radeon_ttm.c +1 -1

index 1e8e287e113c..0597540f0dde 100644
@@ -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;
 

drivers/misc/sgi-gru/grufault.c +1 -1

index b836936e9747..378cf02a2aa1 100644
@@ -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);

include/linux/mm.h +1 -2

index 37554b08bb28..b14cc4972d0b 100644
@@ -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);

mm/gup.c +3 -6

index 1f72a717232b..7e454d6b157e 100644
@@ -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);
 

mm/gup_test.c +2 -3

index 8ae7307a1bb6..9ba8ea23f84e 100644
@@ -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;

virt/kvm/kvm_main.c +1 -1

index d255964ec331..7f31e0a4adb5 100644
@@ -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;
  }
 

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) <[email protected]>

Acked-by Greg Kroah-Hartman <[email protected]>

Acked-by David Hildenbrand <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Lorenzo Stoakes <[email protected]>

---
 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(-)

arch/x86/kernel/cpu/sgx/ioctl.c +1 -1

index 21ca0a831b70..5d390df21440 100644
@@ -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;
 

drivers/gpu/drm/radeon/radeon_ttm.c +1 -1

index 1e8e287e113c..0597540f0dde 100644
@@ -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;
 

drivers/misc/sgi-gru/grufault.c +1 -1

index b836936e9747..378cf02a2aa1 100644
@@ -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);

include/linux/mm.h +1 -2

index 37554b08bb28..b14cc4972d0b 100644
@@ -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);

mm/gup.c +3 -6

index 1f72a717232b..7e454d6b157e 100644
@@ -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);
 

mm/gup_test.c +2 -3

index 8ae7307a1bb6..9ba8ea23f84e 100644
@@ -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;

virt/kvm/kvm_main.c +1 -1

index d255964ec331..7f31e0a4adb5 100644
@@ -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;
 }
 

Signature · 1 line

2.40.0

The only instances of get_user_pages_remote() invocations which used the vmas parameter were for a single page which can instead simply look up the VMA directly. In particular:-

- __update_ref_ctr() looked up the VMA but did nothing with it so we simply remove it.

- __access_remote_vm() was already using vma_lookup() when the original lookup failed so by doing the lookup directly this also de-duplicates the code.

We are able to perform these VMA operations as we already hold the mmap_lock in order to be able to call get_user_pages_remote().

As part of this work we add get_user_page_vma_remote() which abstracts the VMA lookup, error handling and decrementing the page reference count should the VMA lookup fail.

This forms part of a broader set of patches intended to eliminate the vmas parameter altogether.

Reviewed-by Catalin Marinas <[email protected]> (for arm64)

Acked-by David Hildenbrand <[email protected]>

Signed-off-by Lorenzo Stoakes <[email protected]>

---

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Suggested-by Matthew Wilcox (Oracle) <[email protected]>

Signed-off-by Lorenzo Stoakes <[email protected]>

---
  io_uring/rsrc.c | 55 ++++++++++++++++++++++++++++---------------------
  1 file changed, 31 insertions(+), 24 deletions(-)

io_uring/rsrc.c +0 -0

index 7a43aed8e395..3a927df9d913 100644

Quoted 21 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Suggested-by Matthew Wilcox (Oracle) <[email protected]>

Signed-off-by Lorenzo Stoakes <[email protected]>

---
   io_uring/rsrc.c | 55 ++++++++++++++++++++++++++++---------------------
   1 file changed, 31 insertions(+), 24 deletions(-)

io_uring/rsrc.c +0 -0

index 7a43aed8e395..3a927df9d913 100644
@@ -1138,12 +1138,37 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
   	return ret;
   }
 

io_uring/rsrc.c static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages, · line 1141

Quoted 1 line

io_uring/rsrc.c +1 -0

+static int check_vmas_locked(unsigned long addr, unsigned long len)

TBH, the whole "_locked" suffix is a bit confusing.

I was wondering why you'd want to check whether the VMAs are locked ...

Quoted 18 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Suggested-by Matthew Wilcox (Oracle) <[email protected]>

Signed-off-by Lorenzo Stoakes <[email protected]>

---
  io_uring/rsrc.c | 55 ++++++++++++++++++++++++++++---------------------
  1 file changed, 31 insertions(+), 24 deletions(-)

io_uring/rsrc.c +0 -0

index 7a43aed8e395..3a927df9d913 100644

io_uring/rsrc.c static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages, · line 1140

Quoted 3 lines

io_uring/rsrc.c +1 -0

@@ -1138,12 +1138,37 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
  	return ret;
  }
+static int check_vmas_locked(unsigned long addr, unsigned long len)

TBH, the whole "_locked" suffix is a bit confusing.

I was wondering why you'd want to check whether the VMAs are locked ...

Quoted 6 lines

io_uring/rsrc.c +6 -0

+{
+	struct file *file;
+	VMA_ITERATOR(vmi, current->mm, addr);
+	struct vm_area_struct *vma = vma_next(&vmi);
+	unsigned long end = addr + len;
+

io_uring/rsrc.c static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages, · line 1152

Quoted 6 lines

io_uring/rsrc.c +6 -0

+	if (WARN_ON_ONCE(!vma))
+		return -EINVAL;
+
+	file = vma->vm_file;
+	if (file && !is_file_hugepages(file))
+		return -EOPNOTSUPP;

You'd now be rejecting vma_is_shmem() here, no?

Signature · 4 lines

-- 
Thanks,

David / dhildenb

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

io_uring/rsrc.c +15 -0

index 7a43aed8e395..3a927df9d913 100644
@@ -1138,12 +1138,37 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
 	return ret;
 }
 
+static int check_vmas_locked(unsigned long addr, unsigned long len)
+{
+	struct file *file;
+	VMA_ITERATOR(vmi, current->mm, addr);
+	struct vm_area_struct *vma = vma_next(&vmi);
+	unsigned long end = addr + len;
+
+	if (WARN_ON_ONCE(!vma))
+		return -EINVAL;
+
+	file = vma->vm_file;
+	if (file && !is_file_hugepages(file))
+		return -EOPNOTSUPP;
+
+	/* don't support file backed memory */

Quoted 6 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Quoted 6 lines

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

I'll reply here too rather than keep some of this conversaion out-of-band.

I don't necessarily think that making io buffer registration dumber and less efficient by needing a separate vma lookup after the fact is a huge deal, as I would imagine most workloads register buffers at setup time and then don't change them. But if people do switch sets at runtime, it's not necessarily a slow path. That said, I suspect the other bits that we do in here, like the GUP, is going to dominate the overhead anyway.

My main question is, why don't we just have a __pin_user_pages or something helper that still takes the vmas argument, and drop it from pin_user_pages() only? That'd still allow the cleanup of the other users that don't care about the vma at all, while retaining the bundled functionality for the case/cases that do? That would avoid needing explicit vma iteration in io_uring.

--
Jens Axboe

Quoted 19 lines · Jens Axboe

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Quoted 6 lines

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

I'll reply here too rather than keep some of this conversaion out-of-band.

Quoted 7 lines

I don't necessarily think that making io buffer registration dumber and less efficient by needing a separate vma lookup after the fact is a huge deal, as I would imagine most workloads register buffers at setup time and then don't change them. But if people do switch sets at runtime, it's not necessarily a slow path. That said, I suspect the other bits that we do in here, like the GUP, is going to dominate the overhead anyway.

Thanks, and indeed I expect the GUP will dominate.

Quoted 7 lines

My main question is, why don't we just have a __pin_user_pages or something helper that still takes the vmas argument, and drop it from pin_user_pages() only? That'd still allow the cleanup of the other users that don't care about the vma at all, while retaining the bundled functionality for the case/cases that do? That would avoid needing explicit vma iteration in io_uring.

The desire here is to completely eliminate vmas as an externally available parameter from GUP. While we do have a newly introduced helper that returns a VMA, doing the lookup manually for all other vma cases (which look up a single page and vma), that is more so a helper that sits outside of GUP.

Having a separate function that still bundled the vmas would essentially undermine the purpose of the series altogether which is not just to clean up some NULL's but rather to eliminate vmas as part of the GUP interface altogether.

The reason for this is that by doing so we simplify the GUP interface, eliminate a whole class of possible future bugs with people holding onto pointers to vmas which may dangle and lead the way to future changes in GUP which might be more impactful, such as trying to find means to use the fast paths in more areas with an eye to gradual eradication of the use of mmap_lock.

While we return VMAs, none of this is possible and it also makes the interface more confusing - without vmas GUP takes flags which define its behaviour and in most cases returns page objects. The odd rules about what can and cannot return vmas under what circumstances are not helpful for new users.

Another point here is that Jason suggested adding a new FOLL_ALLOW_BROKEN_FILE_MAPPINGS flag which would, by default, not be set. This could assert that only shmem/hugetlb file mappings are permitted which would eliminate the need for you to perform this check at all.

This leads into the larger point that GUP-writing file mappings is fundamentally broken due to e.g. GUP not honouring write notify so this check should at least in theory not be necessary.

So it may be the case that should such a flag be added this code will simply be deleted at a future point :)

Quoted 28 lines · Lorenzo Stoakes

Quoted 19 lines · Jens Axboe

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Quoted 6 lines

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

I'll reply here too rather than keep some of this conversaion out-of-band.

Quoted 7 lines

I don't necessarily think that making io buffer registration dumber and less efficient by needing a separate vma lookup after the fact is a huge deal, as I would imagine most workloads register buffers at setup time and then don't change them. But if people do switch sets at runtime, it's not necessarily a slow path. That said, I suspect the other bits that we do in here, like the GUP, is going to dominate the overhead anyway.

Quoted 1 line

Thanks, and indeed I expect the GUP will dominate.

Unless you have a lot of vmas... Point is, it's _probably_ not a problem, but it might and it's making things worse for no real gain as far as I can tell outside of some notion of "cleaning up the code".

Quoted 36 lines

Quoted 7 lines

My main question is, why don't we just have a __pin_user_pages or something helper that still takes the vmas argument, and drop it from pin_user_pages() only? That'd still allow the cleanup of the other users that don't care about the vma at all, while retaining the bundled functionality for the case/cases that do? That would avoid needing explicit vma iteration in io_uring.

The desire here is to completely eliminate vmas as an externally available parameter from GUP. While we do have a newly introduced helper that returns a VMA, doing the lookup manually for all other vma cases (which look up a single page and vma), that is more so a helper that sits outside of GUP.

Having a separate function that still bundled the vmas would essentially undermine the purpose of the series altogether which is not just to clean up some NULL's but rather to eliminate vmas as part of the GUP interface altogether.

The reason for this is that by doing so we simplify the GUP interface, eliminate a whole class of possible future bugs with people holding onto pointers to vmas which may dangle and lead the way to future changes in GUP which might be more impactful, such as trying to find means to use the fast paths in more areas with an eye to gradual eradication of the use of mmap_lock.

While we return VMAs, none of this is possible and it also makes the interface more confusing - without vmas GUP takes flags which define its behaviour and in most cases returns page objects. The odd rules about what can and cannot return vmas under what circumstances are not helpful for new users.

Another point here is that Jason suggested adding a new FOLL_ALLOW_BROKEN_FILE_MAPPINGS flag which would, by default, not be set. This could assert that only shmem/hugetlb file mappings are permitted which would eliminate the need for you to perform this check at all.

Quoted 6 lines

This leads into the larger point that GUP-writing file mappings is fundamentally broken due to e.g. GUP not honouring write notify so this check should at least in theory not be necessary.

So it may be the case that should such a flag be added this code will simply be deleted at a future point :)

Why don't we do that first then? There's nothing more permanent than a temporary workaround/fix. Once it's in there, motivation to get rid of it for most people is zero because they just never see it. Seems like that'd be a much saner approach rather than the other way around, and make this patchset simpler/cleaner too as it'd only be removing code in all of the callers.

--
Jens Axboe

Quoted 78 lines · Jens Axboe

Quoted 28 lines · Lorenzo Stoakes

Quoted 19 lines · Jens Axboe

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Quoted 6 lines

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

I'll reply here too rather than keep some of this conversaion out-of-band.

Quoted 7 lines

I don't necessarily think that making io buffer registration dumber and less efficient by needing a separate vma lookup after the fact is a huge deal, as I would imagine most workloads register buffers at setup time and then don't change them. But if people do switch sets at runtime, it's not necessarily a slow path. That said, I suspect the other bits that we do in here, like the GUP, is going to dominate the overhead anyway.

Quoted 1 line

Thanks, and indeed I expect the GUP will dominate.

Unless you have a lot of vmas... Point is, it's _probably_ not a problem, but it might and it's making things worse for no real gain as far as I can tell outside of some notion of "cleaning up the code".

Quoted 36 lines

Quoted 7 lines

My main question is, why don't we just have a __pin_user_pages or something helper that still takes the vmas argument, and drop it from pin_user_pages() only? That'd still allow the cleanup of the other users that don't care about the vma at all, while retaining the bundled functionality for the case/cases that do? That would avoid needing explicit vma iteration in io_uring.

The desire here is to completely eliminate vmas as an externally available parameter from GUP. While we do have a newly introduced helper that returns a VMA, doing the lookup manually for all other vma cases (which look up a single page and vma), that is more so a helper that sits outside of GUP.

Having a separate function that still bundled the vmas would essentially undermine the purpose of the series altogether which is not just to clean up some NULL's but rather to eliminate vmas as part of the GUP interface altogether.

The reason for this is that by doing so we simplify the GUP interface, eliminate a whole class of possible future bugs with people holding onto pointers to vmas which may dangle and lead the way to future changes in GUP which might be more impactful, such as trying to find means to use the fast paths in more areas with an eye to gradual eradication of the use of mmap_lock.

While we return VMAs, none of this is possible and it also makes the interface more confusing - without vmas GUP takes flags which define its behaviour and in most cases returns page objects. The odd rules about what can and cannot return vmas under what circumstances are not helpful for new users.

Another point here is that Jason suggested adding a new FOLL_ALLOW_BROKEN_FILE_MAPPINGS flag which would, by default, not be set. This could assert that only shmem/hugetlb file mappings are permitted which would eliminate the need for you to perform this check at all.

Quoted 6 lines

This leads into the larger point that GUP-writing file mappings is fundamentally broken due to e.g. GUP not honouring write notify so this check should at least in theory not be necessary.

So it may be the case that should such a flag be added this code will simply be deleted at a future point :)

Quoted 7 lines

Why don't we do that first then? There's nothing more permanent than a temporary workaround/fix. Once it's in there, motivation to get rid of it for most people is zero because they just never see it. Seems like that'd be a much saner approach rather than the other way around, and make this patchset simpler/cleaner too as it'd only be removing code in all of the callers.

Because I'd then need to audit all GUP callers to see whether they in some way brokenly access files in order to know which should and should not use this new flag. It'd change this series from 'remove the vmas parameter' to something a lot more involved.

I think it's much safer to do the two separately, as I feel that change would need quite a bit of scrutiny too.

As for temporary, I can assure you I will be looking at introducing this flag, for what it's worth :) and Jason is certainly minded to do work in this area also.

Quoted 92 lines · Lorenzo Stoakes

Quoted 78 lines · Jens Axboe

Quoted 28 lines · Lorenzo Stoakes

Quoted 19 lines · Jens Axboe

Quoted 8 lines · Jens Axboe

Quoted 6 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Doing this eliminates the one instance of vmas being used by pin_user_pages().

Quoted 6 lines

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

I'll reply here too rather than keep some of this conversaion out-of-band.

Quoted 7 lines

I don't necessarily think that making io buffer registration dumber and less efficient by needing a separate vma lookup after the fact is a huge deal, as I would imagine most workloads register buffers at setup time and then don't change them. But if people do switch sets at runtime, it's not necessarily a slow path. That said, I suspect the other bits that we do in here, like the GUP, is going to dominate the overhead anyway.

Quoted 1 line

Thanks, and indeed I expect the GUP will dominate.

Unless you have a lot of vmas... Point is, it's _probably_ not a problem, but it might and it's making things worse for no real gain as far as I can tell outside of some notion of "cleaning up the code".

Quoted 36 lines

Quoted 7 lines

My main question is, why don't we just have a __pin_user_pages or something helper that still takes the vmas argument, and drop it from pin_user_pages() only? That'd still allow the cleanup of the other users that don't care about the vma at all, while retaining the bundled functionality for the case/cases that do? That would avoid needing explicit vma iteration in io_uring.

The desire here is to completely eliminate vmas as an externally available parameter from GUP. While we do have a newly introduced helper that returns a VMA, doing the lookup manually for all other vma cases (which look up a single page and vma), that is more so a helper that sits outside of GUP.

Having a separate function that still bundled the vmas would essentially undermine the purpose of the series altogether which is not just to clean up some NULL's but rather to eliminate vmas as part of the GUP interface altogether.

The reason for this is that by doing so we simplify the GUP interface, eliminate a whole class of possible future bugs with people holding onto pointers to vmas which may dangle and lead the way to future changes in GUP which might be more impactful, such as trying to find means to use the fast paths in more areas with an eye to gradual eradication of the use of mmap_lock.

While we return VMAs, none of this is possible and it also makes the interface more confusing - without vmas GUP takes flags which define its behaviour and in most cases returns page objects. The odd rules about what can and cannot return vmas under what circumstances are not helpful for new users.

Another point here is that Jason suggested adding a new FOLL_ALLOW_BROKEN_FILE_MAPPINGS flag which would, by default, not be set. This could assert that only shmem/hugetlb file mappings are permitted which would eliminate the need for you to perform this check at all.

Quoted 6 lines

This leads into the larger point that GUP-writing file mappings is fundamentally broken due to e.g. GUP not honouring write notify so this check should at least in theory not be necessary.

So it may be the case that should such a flag be added this code will simply be deleted at a future point :)

Quoted 7 lines

Why don't we do that first then? There's nothing more permanent than a temporary workaround/fix. Once it's in there, motivation to get rid of it for most people is zero because they just never see it. Seems like that'd be a much saner approach rather than the other way around, and make this patchset simpler/cleaner too as it'd only be removing code in all of the callers.

Because I'd then need to audit all GUP callers to see whether they in some way brokenly access files in order to know which should and should not use this new flag. It'd change this series from 'remove the vmas parameter' to something a lot more involved.

Quoted 6 lines

I think it's much safer to do the two separately, as I feel that change would need quite a bit of scrutiny too.

As for temporary, I can assure you I will be looking at introducing this flag, for what it's worth :) and Jason is certainly minded to do work in this area also.

It's either feasible or it's not, and it didn't sound too bad in terms of getting it done to remove the temporary addition. Since we're now days away from the merge window and any of this would need to soak in for-next anyway for a bit, why not just do that other series first? It really is backward. And this happens sometimes when developing patchsets, at some point you realize that things would be easier/cleaner with another prep series first. Nothing wrong with that, but let's not be hesitant to shift direction a bit when it makes sense to do so.

I keep getting this sense of urgency for a cleanup series. Why not just do it right from the get-go and make this series simpler? At that point there would be no discussion on it at all, as it would be a straight forward cleanup without adding an intermediate step that'd get deleted later anyway.

--
Jens Axboe

I'd also argue that we are doing things right with this patch series as-is, io_uring is the only sticking point because, believe it or not, it is the only place in the kernel that uses multiple vmas (it has been interesting to get a view on GUP use as a whole here).

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

However, if you really do feel that you can't accept this change as-is, I can put this series on hold and look at FOLL_ALLOW_BROKEN_FILE_MAPPING and we can return to this afterwards.

Quoted 4 lines · Lorenzo Stoakes

I'd also argue that we are doing things right with this patch series as-is, io_uring is the only sticking point because, believe it or not, it is the only place in the kernel that uses multiple vmas (it has been interesting to get a view on GUP use as a whole here).

I would say io_uring is the only place trying to open-code bug fixes for MM problems :\ As Jens says, these sorts of temporary work arounds become lingering problems that nobody wants to fix properly.

Quoted 5 lines

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

Yeah, that should just be reverted.

Quoted 3 lines

However, if you really do feel that you can't accept this change as-is, I can put this series on hold and look at FOLL_ALLOW_BROKEN_FILE_MAPPING and we can return to this afterwards.

It is probably not as bad as you think, I suspect only RDMA really wants to set the flag. Maybe something in media too, maybe.

Then again that stuff doesn't work so incredibly badly maybe there really is no user of it and we should just block it completely.

Jason

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

Quoted 26 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Quoted 5 lines

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

Quoted 27 lines · Jason Gunthorpe

Quoted 26 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Quoted 7 lines

Quoted 5 lines

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

So um, it's disallowed because Pavel couldn't understand why it should be allowed? This gets less and less convincing.

FWIW, what I was suggesting was that we should have a FOLL_SINGLE_VMA flag, which would use our shiny new VMA lock infrastructure to look up and lock _one_ VMA instead of having the caller take the mmap_lock. Passing that flag would be a tighter restriction that Pavel implemented, but would certainly relieve some of his mental load.

By the way, even if all pages are from the same VMA, they may still be a mixture of anon and file pages; think a MAP_PRIVATE of a file when only some pages have been written to. Or an anon MAP_SHARED which is accessible by a child process.

For example, imagine if a user (yes it'd be weird) mlock'd some pages in a buffer and not others, then we'd break their use case. Also (perhaps?) more feasibly, a user might mix hugetlb and anon pages. So I think that'd be too restrictive here.

I do like the idea of a FOLL_SINGLE_VMA for other use cases though, the majority of which want one and one page only. Perhaps worth taking the helper added in this series (get_user_page_vma_remote() from [1]) and replacing it with an a full GUP function which has an interface explicitly for this common single page/vma case.

Quoted 4 lines · Lorenzo Stoakes

For example, imagine if a user (yes it'd be weird) mlock'd some pages in a buffer and not others, then we'd break their use case. Also (perhaps?) more feasibly, a user might mix hugetlb and anon pages. So I think that'd be too restrictive here.

Yeah, I agree we should not add a broad single-vma restriction to GUP. It turns any split of a VMA into a potentially uABI breaking change and we just don't need that headache in the mm..

Quoted 5 lines

I do like the idea of a FOLL_SINGLE_VMA for other use cases though, the majority of which want one and one page only. Perhaps worth taking the helper added in this series (get_user_page_vma_remote() from [1]) and replacing it with an a full GUP function which has an interface explicitly for this common single page/vma case.

Like I showed in another thread a function signature that can only do one page and also returns the VMA would force it to be used properly and we don't need a FOLL flag.

Jason

Quoted 30 lines · Jason Gunthorpe

Quoted 29 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

Quoted 4 lines

Quoted 2 lines

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

So um, it's disallowed because Pavel couldn't understand why it should be allowed? This gets less and less convincing.

FWIW, what I was suggesting was that we should have a FOLL_SINGLE_VMA flag, which would use our shiny new VMA lock infrastructure to look up and lock _one_ VMA instead of having the caller take the mmap_lock. Passing that flag would be a tighter restriction that Pavel implemented, but would certainly relieve some of his mental load.

By the way, even if all pages are from the same VMA, they may still be a mixture of anon and file pages; think a MAP_PRIVATE of a file when only some pages have been written to. Or an anon MAP_SHARED which is accessible by a child process.

Quoted 31 lines · Matthew Wilcox

Quoted 30 lines · Jason Gunthorpe

Quoted 29 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

So um, it's disallowed because Pavel couldn't understand why it should be allowed? This gets less and less convincing.

Excuse me? I'm really sorry you "couldn't understand" the explanation as it has probably been too much of a "mental load", but let me try to elaborate.

Because it's currently limited what can be registered, it's indeed not a big deal, but that will most certainly change, and I usually and apparently nonsensically prefer to tighten things up _before_ it becomes a problem. And again, taking a random set of buffers created for different purposes and registering it as a single entity is IMHO not a sane approach.

Take p2pdma for instance, if would have been passed without intermixing there might not have been is_pci_p2pdma_page()/etc. for every single page in a bvec. That's why in general, it won't change for p2pdma but there might be other cases in the future.

Quoted 11 lines

FWIW, what I was suggesting was that we should have a FOLL_SINGLE_VMA flag, which would use our shiny new VMA lock infrastructure to look up and lock _one_ VMA instead of having the caller take the mmap_lock. Passing that flag would be a tighter restriction that Pavel implemented, but would certainly relieve some of his mental load.

By the way, even if all pages are from the same VMA, they may still be a mixture of anon and file pages; think a MAP_PRIVATE of a file when only some pages have been written to. Or an anon MAP_SHARED which is accessible by a child process.

--
Pavel Begunkov

So I guess the question is, do you feel the requirement for vm_file being the same should apply across _any_ GUP operation over a range or is it io_uring-specific?

Quoted 26 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

Quoted 5 lines

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

Quoted 30 lines · Jason Gunthorpe

Quoted 29 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

Quoted 4 lines

Quoted 2 lines

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

Let's just kill that patch that, I can add a revert for 6.4. I had forgotten about that patch and guess I didn't realize that most of the issue do in fact just stem from that.

Quoted 30 lines · Jason Gunthorpe

Quoted 29 lines · Matthew Wilcox

Quoted 5 lines · Lorenzo Stoakes

So even if I did the FOLL_ALLOW_BROKEN_FILE_MAPPING patch series first, I would still need to come along and delete a bunch of your code afterwards. And unfortunately Pavel's recent change which insists on not having different vm_file's across VMAs for the buffer would have to be reverted so I expect it might not be entirely without discussion.

I don't even understand why Pavel wanted to make this change. The commit log really doesn't say.

commit edd478269640

Author: Pavel Begunkov <[email protected]>
Date:   Wed Feb 22 14:36:48 2023 +0000

io_uring/rsrc: disallow multi-source reg buffers

If two or more mappings go back to back to each other they can be passed into io_uring to be registered as a single registered buffer. That would even work if mappings came from different sources, e.g. it's possible to mix in this way anon pages and pages from shmem or hugetlb. That is not a problem but it'd rather be less prone if we forbid such mixing.

Cc <[email protected]>

Signed-off-by Pavel Begunkov <[email protected]>

Signed-off-by Jens Axboe <[email protected]>

It even says "That is not a problem"! So why was this patch merged if it's not fixing a problem?

Quoted 4 lines

Quoted 2 lines

It's now standing in the way of an actual cleanup. So why don't we revert it? There must be more to it than this ...

https://lore.kernel.org/all/[email protected]/

Let's just kill that patch that, I can add a revert for 6.4. I had forgotten about that patch and guess I didn't realize that most of the issue do in fact just stem from that.

Quoted 4 lines · Lorenzo Stoakes

We are shortly to remove pin_user_pages(), and instead perform the required VMA checks ourselves. In most cases there will be a single VMA so this should caues no undue impact on an already slow path.

Quoted 2 lines

Doing this eliminates the one instance of vmas being used by pin_user_pages().

First up, please don't just send single patches from a series. It's really annoying when you are trying to get the full picture. Just CC the whole series, so reviews don't have to look it up separately.

So when you're doing a respin for what I'll mention below and the issue that David found, please don't just show us patch 4+5 of the series.

Quoted 24 lines

io_uring/rsrc.c +15 -0

index 7a43aed8e395..3a927df9d913 100644
@@ -1138,12 +1138,37 @@ static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages,
 	return ret;
 }
 
+static int check_vmas_locked(unsigned long addr, unsigned long len)
+{
+	struct file *file;
+	VMA_ITERATOR(vmi, current->mm, addr);
+	struct vm_area_struct *vma = vma_next(&vmi);
+	unsigned long end = addr + len;
+
+	if (WARN_ON_ONCE(!vma))
+		return -EINVAL;
+
+	file = vma->vm_file;
+	if (file && !is_file_hugepages(file))
+		return -EOPNOTSUPP;
+
+	/* don't support file backed memory */

io_uring/rsrc.c static int io_buffer_account_pin(struct io_ring_ctx *ctx, struct page **pages, · line 1165

Quoted 10 lines

io_uring/rsrc.c +10 -0

+	for_each_vma_range(vmi, vma, end) {
+		if (vma->vm_file != file)
+			return -EINVAL;
+
+		if (file && !vma_is_shmem(vma))
+			return -EOPNOTSUPP;
+	}
+
+	return 0;
+}

I really dislike this naming. There's no point to doing locked in the naming here, it just makes people think it's checking whether the vmas are locked. Which is not at all what it does. Because what else would we think, there's nothing else in the name that suggests what it is actually checking.

Don't put implied locking in the naming, the way to do that is to do something ala:

lockdep_assert_held_read(&current->mm->mmap_lock);

though I don't think it's needed here at all, as there's just one caller and it's clearly inside. You could even just make a comment instead.

So please rename this to indicate what it's ACTUALLY checking.

--
Jens Axboe