mm/gup: disallow GUP writing to file-backed mappings by default

vma_wants_writenotify() is specifically intended for setting PTE page table flags, accounting for existing PTE flag state and whether that might already be read-only while mixing this check with a check whether the filesystem performs dirty tracking.

Separate out the notions of dirty tracking and a PTE write notify checking in order that we can invoke the dirty tracking check from elsewhere.

Note that this change introduces a very small duplicate check of the separated out vm_ops_needs_writenotify(). This is necessary to avoid making vma_needs_dirty_tracking() needlessly complicated (e.g. passing a check_writenotify flag or having it assume this check was already performed). This is such a small check that it doesn't seem too egregious to do this.

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

Reviewed-by John Hubbard <[email protected]>

Reviewed-by Mika Penttilä <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

---
  include/linux/mm.h |  1 +
  mm/mmap.c          | 36 +++++++++++++++++++++++++++---------
  2 files changed, 28 insertions(+), 9 deletions(-)

include/linux/mm.h +1 -0

index 27ce77080c79..7b1d4e7393ef 100644
@@ -2422,6 +2422,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
  #define  MM_CP_UFFD_WP_ALL                 (MM_CP_UFFD_WP | \
  					    MM_CP_UFFD_WP_RESOLVE)
 
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma);
  int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
  static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma)
  {

mm/mmap.c +2 -0

index 5522130ae606..295c5f2e9bd9 100644
@@ -1475,6 +1475,31 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
  }
  #endif /* __ARCH_WANT_SYS_OLD_MMAP */
 
+/* Do VMA operations imply write notify is required? */
+static bool vm_ops_needs_writenotify(const struct vm_operations_struct *vm_ops)

Quoted 49 lines · Lorenzo Stoakes

vma_wants_writenotify() is specifically intended for setting PTE page table flags, accounting for existing PTE flag state and whether that might already be read-only while mixing this check with a check whether the filesystem performs dirty tracking.

Separate out the notions of dirty tracking and a PTE write notify checking in order that we can invoke the dirty tracking check from elsewhere.

Note that this change introduces a very small duplicate check of the separated out vm_ops_needs_writenotify(). This is necessary to avoid making vma_needs_dirty_tracking() needlessly complicated (e.g. passing a check_writenotify flag or having it assume this check was already performed). This is such a small check that it doesn't seem too egregious to do this.

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

Reviewed-by John Hubbard <[email protected]>

Reviewed-by Mika Penttilä <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

---
  include/linux/mm.h |  1 +
  mm/mmap.c          | 36 +++++++++++++++++++++++++++---------
  2 files changed, 28 insertions(+), 9 deletions(-)

include/linux/mm.h +1 -0

index 27ce77080c79..7b1d4e7393ef 100644
@@ -2422,6 +2422,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
  #define  MM_CP_UFFD_WP_ALL                 (MM_CP_UFFD_WP | \
  					    MM_CP_UFFD_WP_RESOLVE)
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma);
  int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
  static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma)
  {

mm/mmap.c +6 -0

index 5522130ae606..295c5f2e9bd9 100644
@@ -1475,6 +1475,31 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
  }
  #endif /* __ARCH_WANT_SYS_OLD_MMAP */
+/* Do VMA operations imply write notify is required? */
+static bool vm_ops_needs_writenotify(const struct vm_operations_struct *vm_ops)
+{
+	return vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite);
+}
+

mm/mmap.c SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg) · line 1489

Quoted 6 lines

mm/mmap.c +6 -0

+/*
+ * Does this VMA require the underlying folios to have their dirty state
+ * tracked?
+ */
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma)
+{

Sorry for not noticing this earlier, but ...

what about MAP_PRIVATE mappings? When we write, we populate an anon page, which will work as expected ... because we don't have to notify the fs?

I think you really also want the "If it was private or non-writable, the write bit is already clear */" part as well and remove "false" in that case.

Or was there a good reason to disallow private mappings as well?

--
Thanks,

David / dhildenb

Quoted 50 lines · David Hildenbrand

Quoted 49 lines · Lorenzo Stoakes

vma_wants_writenotify() is specifically intended for setting PTE page table flags, accounting for existing PTE flag state and whether that might already be read-only while mixing this check with a check whether the filesystem performs dirty tracking.

Separate out the notions of dirty tracking and a PTE write notify checking in order that we can invoke the dirty tracking check from elsewhere.

Note that this change introduces a very small duplicate check of the separated out vm_ops_needs_writenotify(). This is necessary to avoid making vma_needs_dirty_tracking() needlessly complicated (e.g. passing a check_writenotify flag or having it assume this check was already performed). This is such a small check that it doesn't seem too egregious to do this.

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

Reviewed-by John Hubbard <[email protected]>

Reviewed-by Mika Penttilä <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

---
  include/linux/mm.h |  1 +
  mm/mmap.c          | 36 +++++++++++++++++++++++++++---------
  2 files changed, 28 insertions(+), 9 deletions(-)

include/linux/mm.h +1 -0

index 27ce77080c79..7b1d4e7393ef 100644
@@ -2422,6 +2422,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
  #define  MM_CP_UFFD_WP_ALL                 (MM_CP_UFFD_WP | \
  					    MM_CP_UFFD_WP_RESOLVE)
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma);
  int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
  static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma)
  {

mm/mmap.c +6 -0

index 5522130ae606..295c5f2e9bd9 100644
@@ -1475,6 +1475,31 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
  }
  #endif /* __ARCH_WANT_SYS_OLD_MMAP */
+/* Do VMA operations imply write notify is required? */
+static bool vm_ops_needs_writenotify(const struct vm_operations_struct *vm_ops)
+{
+	return vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite);
+}
+

Quoted 8 lines

mm/mmap.c SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg) · line 1489

Quoted 6 lines

mm/mmap.c +6 -0

+/*
+ * Does this VMA require the underlying folios to have their dirty state
+ * tracked?
+ */
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma)
+{

Sorry for not noticing this earlier, but ...

pints_owed++

Quoted 7 lines

what about MAP_PRIVATE mappings? When we write, we populate an anon page, which will work as expected ... because we don't have to notify the fs?

I think you really also want the "If it was private or non-writable, the write bit is already clear */" part as well and remove "false" in that case.

Not sure a 'write bit is already clear' case is relevant to checking whether a filesystem dirty tracks? That seems specific entirely to the page table bits.

That's why I didn't include it,

A !VM_WRITE shouldn't be GUP-writable except for FOLL_FORCE, and that surely could be problematic if VM_MAYWRITE later?

Thinking about it though a !VM_SHARE should probably can be safely assumed to not be dirty-trackable, so we probably do need to add a check for !VM_SHARED -> !vma_needs_dirty_tracking

Quoted 2 lines

Or was there a good reason to disallow private mappings as well?

Until the page is CoW'd walking the page tables will get you to the page cache page right? This was the reason I (perhaps rather too quickly) felt MAP_PRIVATE should be excluded.

However a FOLL_WRITE would trigger CoW... and then we'd be trivially OK.

So yeah, ok perhaps I dismissed that a little too soon. I was concerned about some sort of egregious FOLL_FORCE case where somehow we'd end up with the page cache folio. But actually, that probably can't happen...

Quoted 5 lines

--
Thanks,

David / dhildenb

Quoted 60 lines · Lorenzo Stoakes

Quoted 58 lines · David Hildenbrand

Quoted 45 lines · Lorenzo Stoakes

vma_wants_writenotify() is specifically intended for setting PTE page table flags, accounting for existing PTE flag state and whether that might already be read-only while mixing this check with a check whether the filesystem performs dirty tracking.

Separate out the notions of dirty tracking and a PTE write notify checking in order that we can invoke the dirty tracking check from elsewhere.

Note that this change introduces a very small duplicate check of the separated out vm_ops_needs_writenotify(). This is necessary to avoid making vma_needs_dirty_tracking() needlessly complicated (e.g. passing a check_writenotify flag or having it assume this check was already performed). This is such a small check that it doesn't seem too egregious to do this.

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

Reviewed-by John Hubbard <[email protected]>

Reviewed-by Mika Penttilä <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

---
   include/linux/mm.h |  1 +
   mm/mmap.c          | 36 +++++++++++++++++++++++++++---------
   2 files changed, 28 insertions(+), 9 deletions(-)

include/linux/mm.h +1 -0

index 27ce77080c79..7b1d4e7393ef 100644
@@ -2422,6 +2422,7 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,
   #define  MM_CP_UFFD_WP_ALL                 (MM_CP_UFFD_WP | \
   					    MM_CP_UFFD_WP_RESOLVE)
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma);
   int vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);
   static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma)
   {

mm/mmap.c +2 -0

index 5522130ae606..295c5f2e9bd9 100644
@@ -1475,6 +1475,31 @@ SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg)
   }
   #endif /* __ARCH_WANT_SYS_OLD_MMAP */
+/* Do VMA operations imply write notify is required? */
+static bool vm_ops_needs_writenotify(const struct vm_operations_struct *vm_ops)

mm/mmap.c SYSCALL_DEFINE1(old_mmap, struct mmap_arg_struct __user *, arg) · line 1488

Quoted 10 lines

mm/mmap.c +10 -0

+{
+	return vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite);
+}
+
+/*
+ * Does this VMA require the underlying folios to have their dirty state
+ * tracked?
+ */
+bool vma_needs_dirty_tracking(struct vm_area_struct *vma)
+{

Sorry for not noticing this earlier, but ...

Quoted 1 line

pints_owed++

Quoted 13 lines

Quoted 8 lines

what about MAP_PRIVATE mappings? When we write, we populate an anon page, which will work as expected ... because we don't have to notify the fs?

I think you really also want the "If it was private or non-writable, the write bit is already clear */" part as well and remove "false" in that case.

Not sure a 'write bit is already clear' case is relevant to checking whether a filesystem dirty tracks? That seems specific entirely to the page table bits.

Quoted 9 lines

That's why I didn't include it,

A !VM_WRITE shouldn't be GUP-writable except for FOLL_FORCE, and that surely could be problematic if VM_MAYWRITE later?

Thinking about it though a !VM_SHARE should probably can be safely assumed to not be dirty-trackable, so we probably do need to add a check for !VM_SHARED -> !vma_needs_dirty_tracking

On second thoughts, we explicitly check FOLL_FORCE && !is_cow_mapping() in check_vma_flags() so that case cannot occur.

So actually yes we should probably include this on the basis of that and the fact that a FOLL_WRITE operation will CoW the MAP_PRIVATE mapping.

Writing to file-backed mappings which require folio dirty tracking using GUP is a fundamentally broken operation, as kernel write access to GUP mappings do not adhere to the semantics expected by a file system.

A GUP caller uses the direct mapping to access the folio, which does not cause write notify to trigger, nor does it enforce that the caller marks the folio dirty.

The problem arises when, after an initial write to the folio, writeback results in the folio being cleaned and then the caller, via the GUP interface, writes to the folio again.

As a result of the use of this secondary, direct, mapping to the folio no write notify will occur, and if the caller does mark the folio dirty, this will be done so unexpectedly.

For example, consider the following scenario:-

1. A folio is written to via GUP which write-faults the memory, notifying the file system and dirtying the folio.
2. Later, writeback is triggered, resulting in the folio being cleaned and the PTE being marked read-only.
3. The GUP caller writes to the folio, as it is mapped read/write via the direct mapping.
4. The GUP caller, now done with the page, unpins it and sets it dirty (though it does not have to).

This results in both data being written to a folio without writenotify, and the folio being dirtied unexpectedly (if the caller decides to do so).

This issue was first reported by Jan Kara [1] in 2018, where the problem resulted in file system crashes.

This is only relevant when the mappings are file-backed and the underlying file system requires folio dirty tracking. File systems which do not, such as shmem or hugetlb, are not at risk and therefore can be written to without issue.

Unfortunately this limitation of GUP has been present for some time and requires future rework of the GUP API in order to provide correct write access to such mappings.

However, for the time being we introduce this check to prevent the most egregious case of this occurring, use of the FOLL_LONGTERM pin.

These mappings are considerably more likely to be written to after folios are cleaned and thus simply must not be permitted to do so.

This patch changes only the slow-path GUP functions, a following patch adapts the GUP-fast path along similar lines.

[1]:https://lore.kernel.org/linux-mm/[email protected]/

Suggested-by Jason Gunthorpe <[email protected]>

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

Reviewed-by John Hubbard <[email protected]>

Reviewed-by Mika Penttilä <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

---
  mm/gup.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 42 insertions(+), 1 deletion(-)

mm/gup.c +18 -0

index ff689c88a357..6e209ca10967 100644
@@ -959,16 +959,53 @@ static int faultin_page(struct vm_area_struct *vma,
  	return 0;
  }
 
+/*
+ * Writing to file-backed mappings which require folio dirty tracking using GUP
+ * is a fundamentally broken operation, as kernel write access to GUP mappings
+ * do not adhere to the semantics expected by a file system.
+ *
+ * Consider the following scenario:-
+ *
+ * 1. A folio is written to via GUP which write-faults the memory, notifying
+ *    the file system and dirtying the folio.
+ * 2. Later, writeback is triggered, resulting in the folio being cleaned and
+ *    the PTE being marked read-only.
+ * 3. The GUP caller writes to the folio, as it is mapped read/write via the
+ *    direct mapping.
+ * 4. The GUP caller, now done with the page, unpins it and sets it dirty
+ *    (though it does not have to).
+ *
+ * This results in both data being written to a folio without writenotify, and
+ * the folio being dirtied unexpectedly (if the caller decides to do so).

Quoted 88 lines · Lorenzo Stoakes

Writing to file-backed mappings which require folio dirty tracking using GUP is a fundamentally broken operation, as kernel write access to GUP mappings do not adhere to the semantics expected by a file system.

A GUP caller uses the direct mapping to access the folio, which does not cause write notify to trigger, nor does it enforce that the caller marks the folio dirty.

The problem arises when, after an initial write to the folio, writeback results in the folio being cleaned and then the caller, via the GUP interface, writes to the folio again.

As a result of the use of this secondary, direct, mapping to the folio no write notify will occur, and if the caller does mark the folio dirty, this will be done so unexpectedly.

For example, consider the following scenario:-

1. A folio is written to via GUP which write-faults the memory, notifying the file system and dirtying the folio.
2. Later, writeback is triggered, resulting in the folio being cleaned and the PTE being marked read-only.
3. The GUP caller writes to the folio, as it is mapped read/write via the direct mapping.
4. The GUP caller, now done with the page, unpins it and sets it dirty (though it does not have to).

This results in both data being written to a folio without writenotify, and the folio being dirtied unexpectedly (if the caller decides to do so).

This issue was first reported by Jan Kara [1] in 2018, where the problem resulted in file system crashes.

This is only relevant when the mappings are file-backed and the underlying file system requires folio dirty tracking. File systems which do not, such as shmem or hugetlb, are not at risk and therefore can be written to without issue.

Unfortunately this limitation of GUP has been present for some time and requires future rework of the GUP API in order to provide correct write access to such mappings.

However, for the time being we introduce this check to prevent the most egregious case of this occurring, use of the FOLL_LONGTERM pin.

These mappings are considerably more likely to be written to after folios are cleaned and thus simply must not be permitted to do so.

This patch changes only the slow-path GUP functions, a following patch adapts the GUP-fast path along similar lines.

[1]:https://lore.kernel.org/linux-mm/[email protected]/

Suggested-by Jason Gunthorpe <[email protected]>

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

Reviewed-by John Hubbard <[email protected]>

Reviewed-by Mika Penttilä <[email protected]>

Reviewed-by Jan Kara <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

---
  mm/gup.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 42 insertions(+), 1 deletion(-)

mm/gup.c +18 -0

index ff689c88a357..6e209ca10967 100644
@@ -959,16 +959,53 @@ static int faultin_page(struct vm_area_struct *vma,
  	return 0;
  }
+/*
+ * Writing to file-backed mappings which require folio dirty tracking using GUP
+ * is a fundamentally broken operation, as kernel write access to GUP mappings
+ * do not adhere to the semantics expected by a file system.
+ *
+ * Consider the following scenario:-
+ *
+ * 1. A folio is written to via GUP which write-faults the memory, notifying
+ *    the file system and dirtying the folio.
+ * 2. Later, writeback is triggered, resulting in the folio being cleaned and
+ *    the PTE being marked read-only.
+ * 3. The GUP caller writes to the folio, as it is mapped read/write via the
+ *    direct mapping.
+ * 4. The GUP caller, now done with the page, unpins it and sets it dirty
+ *    (though it does not have to).
+ *
+ * This results in both data being written to a folio without writenotify, and
+ * the folio being dirtied unexpectedly (if the caller decides to do so).

mm/gup.c static int faultin_page(struct vm_area_struct *vma, · line 988

Quoted 10 lines

mm/gup.c +10 -0

+ */
+static bool writeable_file_mapping_allowed(struct vm_area_struct *vma,
+					   unsigned long gup_flags)
+{
+	/*
+	 * If we aren't pinning then no problematic write can occur. A long term
+	 * pin is the most egregious case so this is the case we disallow.
+	 */
+	if (!(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
+		return true;

If you really want to keep FOLL_PIN here ... this has to be

if ((gup_flags & (FOLL_PIN | FOLL_LONGTERM)) != (FOLL_PIN | FOLL_LONGTERM))

or two separate checks.

Otherwise you'd also proceed if only FOLL_PIN is set.

Unless my tired eyes betrayed me.

--
Thanks,

David / dhildenb

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c +5 -2

+}
+
  /**
   * try_grab_folio() - Attempt to get or pin a folio.
   * @page:  pointer to page to be grabbed

@@ -123,6 +170,8 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
   */
  struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
  {
+	bool is_longterm = flags & FOLL_LONGTERM;
+
  	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && is_pci_p2pdma_page(page)))
  		return NULL;
 

@@ -136,8 +185,7 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
  		 * right zone, so fail and let the caller fall back to the slow
  		 * path.
  		 */
-		if (unlikely((flags & FOLL_LONGTERM) &&
-			     !is_longterm_pinnable_page(page)))
+		if (unlikely(is_longterm && !is_longterm_pinnable_page(page)))
  			return NULL;
 
  		/*

@@ -148,6 +196,16 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
  		if (!folio)
  			return NULL;

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

Quoted 40 lines · David Hildenbrand

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

Quoted 4 lines

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

So how stable is folio->mapping at this point? Can two subsequent reads get different values? (eg. an actual mapping and NULL)

If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid the compiler from emitting the load multiple times.

Quoted 46 lines · Peter Zijlstra

Quoted 40 lines · David Hildenbrand

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

Quoted 4 lines

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

Quoted 5 lines

So how stable is folio->mapping at this point? Can two subsequent reads get different values? (eg. an actual mapping and NULL)

If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid the compiler from emitting the load multiple times.

I can only talk about anon pages in this specific call order here (check first, then test if the PTE changed in the meantime): we don't care if we get two different values. If we get a different value the second time, surely we (temporarily) pinned an anon page that is no longer mapped (freed in the meantime). But in that case (even if we read garbage folio->mapping and made the wrong call here), we'll detect afterwards that the PTE changed, and unpin what we (temporarily) pinned. As folio_test_anon() only checks two bits in folio->mapping it's fine, because we won't dereference garbage folio->mapping.

With folio_mapping() on !anon and READ_ONCE() ... good question. Kirill said it would be fairly stable, but I suspect that it could change (especially if we call it before validating if the PTE changed as I described further below).

Now, if we read folio->mapping after checking if the page we pinned is still mapped (PTE unchanged), at least the page we pinned cannot be reused in the meantime. I suspect that we can still read "NULL" on the second read. But whatever we dereference from the first read should still be valid, even if the second read would have returned NULL ("rcu freeing").

--
Thanks,

David / dhildenb

Quoted 68 lines · David Hildenbrand

Quoted 46 lines · Peter Zijlstra

Quoted 40 lines · David Hildenbrand

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

Quoted 4 lines

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

Quoted 5 lines

So how stable is folio->mapping at this point? Can two subsequent reads get different values? (eg. an actual mapping and NULL)

If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid the compiler from emitting the load multiple times.

I can only talk about anon pages in this specific call order here (check first, then test if the PTE changed in the meantime): we don't care if we get two different values. If we get a different value the second time, surely we (temporarily) pinned an anon page that is no longer mapped (freed in the meantime). But in that case (even if we read garbage folio->mapping and made the wrong call here), we'll detect afterwards that the PTE changed, and unpin what we (temporarily) pinned. As folio_test_anon() only checks two bits in folio->mapping it's fine, because we won't dereference garbage folio->mapping.

With folio_mapping() on !anon and READ_ONCE() ... good question. Kirill said it would be fairly stable, but I suspect that it could change (especially if we call it before validating if the PTE changed as I described further below).

Quoted 6 lines

Now, if we read folio->mapping after checking if the page we pinned is still mapped (PTE unchanged), at least the page we pinned cannot be reused in the meantime. I suspect that we can still read "NULL" on the second read. But whatever we dereference from the first read should still be valid, even if the second read would have returned NULL ("rcu freeing").

On a specific point - if mapping turns out to be NULL after we confirm stable PTE, I'd be inclined to reject and let the slow path take care of it, would you agree that that's the correct approach?

On a specific point - if mapping turns out to be NULL after we confirm stable PTE, I'd be inclined to reject and let the slow path take care of it, would you agree that that's the correct approach?

Quoted 3 lines · Lorenzo Stoakes

On a specific point - if mapping turns out to be NULL after we confirm stable PTE, I'd be inclined to reject and let the slow path take care of it, would you agree that that's the correct approach?

I think in general if GUP fast detects any kind of race it should bail to the slow path.

The races it tries to resolve itself should have really safe and obvious solutions.

I think this comment is misleading:

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 120

Quoted 4 lines

mm/gup.c +4 -0

+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).

True, but that is not important here since we are not reading page tables

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 124

Quoted 3 lines

mm/gup.c +3 -0

+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.

Not really clear. We have a valid folio refcount here, that is all.

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 128

Quoted 4 lines

mm/gup.c +3 -0

+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.

Which is the real point:

1) GUP-fast disables IRQs which means this is the same context as rcu_read_lock()
2) We have a valid ref on the folio due to normal GUP fast operation Thus derefing struct folio is OK
3) folio->mapping can be deref'd safely under RCU since mapping is RCU free'd It may be zero if we are racing a page free path
Can it be zero for other reasons?

If it can't be zero for any other reason then go to GUP slow and let it sort it out

Otherwise you have to treat NULL as a success.

Really what you are trying to do here is remove the folio lock which would normally protect folio->mapping. Ie this test really boils down to just 'folio_get_mapping_a_ops_rcu() == shmem_aops'

The hugetlb test is done on a page flag which should be stable under the pageref.

So.. Your function really ought to be doing this logic:

        // Should be impossible for a slab page to be in a VMA
	if (unlikely(folio_test_slab(folio)))
	   return do gup slow;

	// Can a present PTE even be a swap cache?
   	if (unlikely(folio_test_swapcache(folio)))
	   return do gup slow;

	if (folio_test_hugetlb(folio))
	   return safe for fast

	// Safe without the folio lock ?

struct address_space *mapping = READ_ONCE(folio->mapping)

	if ((mapping & PAGE_MAPPING_FLAGS) == PAGE_MAPPING_ANON)
	   return safe for fast
	if ((mapping & PAGE_MAPPING_FLAGS) == 0 && mapping)
	   return mapping->a_ops == shmem_aops;

        // Depends on what mapping = NULL means
	return do gup slow

Jason

Quoted 22 lines · Jason Gunthorpe

Quoted 3 lines · Lorenzo Stoakes

On a specific point - if mapping turns out to be NULL after we confirm stable PTE, I'd be inclined to reject and let the slow path take care of it, would you agree that that's the correct approach?

I think in general if GUP fast detects any kind of race it should bail to the slow path.

The races it tries to resolve itself should have really safe and obvious solutions.

I think this comment is misleading:

Review

Quoted 4 lines

(quoted diff) +4 -0

+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).

True, but that is not important here since we are not reading page tables

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.

Not really clear. We have a valid folio refcount here, that is all.

Some of this is a product of mixed signals from different commenters and my being perhaps a little _too_ willing to just go with the flow.

With interrupts disabled and IPI blocked, plus the assurances that interrupts being disabled implied the RCU version of page table manipulation is also blocked, my understanding was that remapping in this process to another page could not occur.

Of course the folio is 'stable' in the sense we have a refcount on it, but it is unlocked so things can change.

I'm guessing the RCU guarantees in the TLB logic are not as solid as IPI, because in the IPI case it seems to me you couldn't even clear the PTE entry before getting to the page table case.

Otherwise, I'm a bit uncertain actually as to how we can get to the point where the folio->mapping is being manipulated. Is this why?

Quoted 44 lines · Lorenzo Stoakes

Quoted 22 lines · Jason Gunthorpe

Quoted 3 lines · Lorenzo Stoakes

On a specific point - if mapping turns out to be NULL after we confirm stable PTE, I'd be inclined to reject and let the slow path take care of it, would you agree that that's the correct approach?

I think in general if GUP fast detects any kind of race it should bail to the slow path.

The races it tries to resolve itself should have really safe and obvious solutions.

I think this comment is misleading:

Review

Quoted 4 lines

(quoted diff) +4 -0

+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).

True, but that is not important here since we are not reading page tables

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.

Not really clear. We have a valid folio refcount here, that is all.

Some of this is a product of mixed signals from different commenters and my being perhaps a little _too_ willing to just go with the flow.

With interrupts disabled and IPI blocked, plus the assurances that interrupts being disabled implied the RCU version of page table manipulation is also blocked, my understanding was that remapping in this process to another page could not occur.

Of course the folio is 'stable' in the sense we have a refcount on it, but it is unlocked so things can change.

I'm guessing the RCU guarantees in the TLB logic are not as solid as IPI, because in the IPI case it seems to me you couldn't even clear the PTE entry before getting to the page table case.

Quoted 2 lines

Otherwise, I'm a bit uncertain actually as to how we can get to the point where the folio->mapping is being manipulated. Is this why?

I'll just stress again that I think there are cases where we unmap and free a page without synchronizing against GUP-fast using an IPI or RCU.

That's one of the reasons why we recheck if the PTE changed to back off, so I've been told.

I'm happy if someone proves me wrong and a page we just (temporarily) pinned cannot have been freed+reused in the meantime.

--
Thanks,

David / dhildenb

I'll just stress again that I think there are cases where we unmap and free a page without synchronizing against GUP-fast using an IPI or RCU.

That's one of the reasons why we recheck if the PTE changed to back off, so I've been told.

I'm happy if someone proves me wrong and a page we just (temporarily) pinned cannot have been freed+reused in the meantime.

Otherwise, I'm a bit uncertain actually as to how we can get to the point where the folio->mapping is being manipulated. Is this why?

In any case, I will try to clarify these, I do agree the _key_ point is that we can rely on safely derefing the mapping, at least READ_ONCE()'d, as use-after-free or dereffing garbage is the fear here.

Well that was literally the question :) and I've got somewhat contradictory feedback. My instinct aligns with yours in that, just fallback to slow path, so that's what I'll do. But just wanted to confirm.

The READ_ONCE() approach is precisely how I wanted to do it in thet first instance, but feared feedback about duplication and wondered if it made much difference, but now it's clear this is ther ight way.

Now, if we read folio->mapping after checking if the page we pinned is still mapped (PTE unchanged), at least the page we pinned cannot be reused in the meantime. I suspect that we can still read "NULL" on the second read. But whatever we dereference from the first read should still be valid, even if the second read would have returned NULL ("rcu freeing").

Quoted 5 lines · David Hildenbrand

Now, if we read folio->mapping after checking if the page we pinned is still mapped (PTE unchanged), at least the page we pinned cannot be reused in the meantime. I suspect that we can still read "NULL" on the second read. But whatever we dereference from the first read should still be valid, even if the second read would have returned NULL ("rcu freeing").

Right, but given it's the compiler adding loads we're not sure what if anything it uses and it gets very hard to reason about the code.

This is where READ_ONCE() helps, we instruct the compiler to only do a single load and we can still reason about the code.

Quoted 40 lines · David Hildenbrand

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

Quoted 4 lines

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

So how stable is folio->mapping at this point? Can two subsequent reads get different values? (eg. an actual mapping and NULL)

If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid the compiler from emitting the load multiple times.

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

Quoted 25 lines

mm/gup.c +5 -2

+}
+
  /**
   * try_grab_folio() - Attempt to get or pin a folio.
   * @page:  pointer to page to be grabbed

@@ -123,6 +170,8 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
   */
  struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
  {
+	bool is_longterm = flags & FOLL_LONGTERM;
+
  	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && is_pci_p2pdma_page(page)))
  		return NULL;

@@ -136,8 +185,7 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
  		 * right zone, so fail and let the caller fall back to the slow
  		 * path.
  		 */
-		if (unlikely((flags & FOLL_LONGTERM) &&
-			     !is_longterm_pinnable_page(page)))
+		if (unlikely(is_longterm && !is_longterm_pinnable_page(page)))
  			return NULL;
  		/*

@@ -148,6 +196,16 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
  		if (!folio)

mm/gup.c struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags) · line 206

Quoted 10 lines

mm/gup.c +9 -0

  			return NULL;
+		/*
+		 * Can this folio be safely pinned? We need to perform this
+		 * check after the folio is stabilised.
+		 */
+		if ((flags & FOLL_WRITE) && is_longterm &&
+		    !folio_longterm_write_pin_allowed(folio)) {
+			folio_put_refs(folio, refs);
+			return NULL;
+		}

So we perform this change before validating whether the PTE changed.

Hmm, naturally, I would have done it afterwards.

IIRC, without IPI syncs during TLB flush (i.e., CONFIG_MMU_GATHER_RCU_TABLE_FREE), there is the possibility that
(1) We lookup the pte
(2) The page was unmapped and free
(3) The page gets reallocated and used
(4) We pin the page
(5) We dereference page->mapping

If we then de-reference page->mapping that gets used by whoever allocated it for something completely different (not a pointer to something reasonable), I wonder if we might be in trouble.

Checking first, whether the PTE changed makes sure that what we pinned and what we're looking at is what we expected.

... I can spot that the page_is_secretmem() check is also done before that. But it at least makes sure that it's still an LRU page before staring at the mapping (making it a little safer?).

BUT, I keep messing up this part of the story. Maybe it all works as expected because we will be synchronizing RCU somehow before actually freeing the page in the !IPI case. ... but I think that's only true for page tables with CONFIG_MMU_GATHER_RCU_TABLE_FREE.

--
Thanks,

David / dhildenb

Quoted 35 lines · David Hildenbrand

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

Quoted 4 lines

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

How could that happen?

Quoted 5 lines

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

I literally made this change based on your suggestion :) but perhaps I misinterpreted what you meant.

I do spell it out in the comment that the page can be anonymous, But perhaps explicitly checking the mapping flags is the way to go.

Quoted 40 lines

Quoted 25 lines

mm/gup.c +5 -2

+}
+
   /**
    * try_grab_folio() - Attempt to get or pin a folio.
    * @page:  pointer to page to be grabbed

@@ -123,6 +170,8 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
    */
   struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
   {
+	bool is_longterm = flags & FOLL_LONGTERM;
+
   	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && is_pci_p2pdma_page(page)))
   		return NULL;

@@ -136,8 +185,7 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
   		 * right zone, so fail and let the caller fall back to the slow
   		 * path.
   		 */
-		if (unlikely((flags & FOLL_LONGTERM) &&
-			     !is_longterm_pinnable_page(page)))
+		if (unlikely(is_longterm && !is_longterm_pinnable_page(page)))
   			return NULL;
   		/*

@@ -148,6 +196,16 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
   		if (!folio)

mm/gup.c struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags) · line 206

Quoted 10 lines

mm/gup.c +9 -0

   			return NULL;
+		/*
+		 * Can this folio be safely pinned? We need to perform this
+		 * check after the folio is stabilised.
+		 */
+		if ((flags & FOLL_WRITE) && is_longterm &&
+		    !folio_longterm_write_pin_allowed(folio)) {
+			folio_put_refs(folio, refs);
+			return NULL;
+		}

So we perform this change before validating whether the PTE changed.

Hmm, naturally, I would have done it afterwards.

Quoted 7 lines

IIRC, without IPI syncs during TLB flush (i.e., CONFIG_MMU_GATHER_RCU_TABLE_FREE), there is the possibility that
(1) We lookup the pte
(2) The page was unmapped and free
(3) The page gets reallocated and used
(4) We pin the page
(5) We dereference page->mapping

But we have an implied RCU lock from disabled IRQs right? Unless that CONFIG option does something odd (I've not really dug into its brehaviour). It feels like that would break GUP-fast as a whole.

Quoted 11 lines

If we then de-reference page->mapping that gets used by whoever allocated it for something completely different (not a pointer to something reasonable), I wonder if we might be in trouble.

Checking first, whether the PTE changed makes sure that what we pinned and what we're looking at is what we expected.

... I can spot that the page_is_secretmem() check is also done before that. But it at least makes sure that it's still an LRU page before staring at the mapping (making it a little safer?).

As do we :)

We also via try_get_folio() check to ensure that we aren't subject to a split.

Quoted 1 line

Quoted 4 lines

BUT, I keep messing up this part of the story. Maybe it all works as expected because we will be synchronizing RCU somehow before actually freeing the page in the !IPI case. ... but I think that's only true for page tables with CONFIG_MMU_GATHER_RCU_TABLE_FREE.

My understanding based on what Peter said is that the IRQs being disabled should prevent anything bad from happening here.

Quoted 124 lines · Lorenzo Stoakes

Quoted 35 lines · David Hildenbrand

[...]

Quoted 19 lines

mm/gup.c +19 -0

+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 142

Quoted 10 lines

mm/gup.c +10 -0

+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *
+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);

"Folios in the swap cache return the swap mapping" -- you might disallow pinning anonymous pages that are in the swap cache.

Quoted 4 lines

I recall that there are corner cases where we can end up with an anon page that's mapped writable but still in the swap cache ... so you'd fallback to the GUP slow path (acceptable for these corner cases, I guess), however especially the comment is a bit misleading then.

How could that happen?

Quoted 5 lines

So I'd suggest not dropping the folio_test_anon() check, or open-coding it ... which will make this piece of code most certainly easier to get when staring at folio_mapping(). Or to spell it out in the comment (usually I prefer code over comments).

I literally made this change based on your suggestion :) but perhaps I misinterpreted what you meant.

I do spell it out in the comment that the page can be anonymous, But perhaps explicitly checking the mapping flags is the way to go.

Quoted 40 lines

Quoted 25 lines

mm/gup.c +5 -2

+}
+
   /**
    * try_grab_folio() - Attempt to get or pin a folio.
    * @page:  pointer to page to be grabbed

@@ -123,6 +170,8 @@ static inline struct folio *try_get_folio(struct page *page, int refs)
    */
   struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
   {
+	bool is_longterm = flags & FOLL_LONGTERM;
+
   	if (unlikely(!(flags & FOLL_PCI_P2PDMA) && is_pci_p2pdma_page(page)))
   		return NULL;

@@ -136,8 +185,7 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
   		 * right zone, so fail and let the caller fall back to the slow
   		 * path.
   		 */
-		if (unlikely((flags & FOLL_LONGTERM) &&
-			     !is_longterm_pinnable_page(page)))
+		if (unlikely(is_longterm && !is_longterm_pinnable_page(page)))
   			return NULL;
   		/*

@@ -148,6 +196,16 @@ struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags)
   		if (!folio)

mm/gup.c struct folio *try_grab_folio(struct page *page, int refs, unsigned int flags) · line 206

Quoted 10 lines

mm/gup.c +9 -0

   			return NULL;
+		/*
+		 * Can this folio be safely pinned? We need to perform this
+		 * check after the folio is stabilised.
+		 */
+		if ((flags & FOLL_WRITE) && is_longterm &&
+		    !folio_longterm_write_pin_allowed(folio)) {
+			folio_put_refs(folio, refs);
+			return NULL;
+		}

So we perform this change before validating whether the PTE changed.

Hmm, naturally, I would have done it afterwards.

Quoted 7 lines

IIRC, without IPI syncs during TLB flush (i.e., CONFIG_MMU_GATHER_RCU_TABLE_FREE), there is the possibility that
(1) We lookup the pte
(2) The page was unmapped and free
(3) The page gets reallocated and used
(4) We pin the page
(5) We dereference page->mapping

But we have an implied RCU lock from disabled IRQs right? Unless that CONFIG option does something odd (I've not really dug into its brehaviour). It feels like that would break GUP-fast as a whole.

Quoted 11 lines

If we then de-reference page->mapping that gets used by whoever allocated it for something completely different (not a pointer to something reasonable), I wonder if we might be in trouble.

Checking first, whether the PTE changed makes sure that what we pinned and what we're looking at is what we expected.

... I can spot that the page_is_secretmem() check is also done before that. But it at least makes sure that it's still an LRU page before staring at the mapping (making it a little safer?).

As do we :)

We also via try_get_folio() check to ensure that we aren't subject to a split.

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

BUT, I keep messing up this part of the story. Maybe it all works as expected because we will be synchronizing RCU somehow before actually freeing the page in the !IPI case. ... but I think that's only true for page tables with CONFIG_MMU_GATHER_RCU_TABLE_FREE.

My understanding based on what Peter said is that the IRQs being disabled should prevent anything bad from happening here.

... only if we verified that the PTE didn't change IIUC. IRQs disabled only protect you from the mapping getting freed and reused (because mappings are freed via RCU IIUC).

But as far as I can tell, it doesn't protect you from the page itself getting freed and reused, and whoever freed the page uses page->mapping to store something completely different.

But, again, it's all complicated and confusing to me.

page_is_secretmem() also doesn't use a READ_ONCE() ...

--
Thanks,

David / dhildenb

mm/gup.c +21 -0

+static bool folio_longterm_write_pin_allowed(struct folio *folio)
+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*
+	 * If no mapping can be found, this implies an anonymous or otherwise

Quoted 26 lines

mm/gup.c +26 -0

+static bool folio_longterm_write_pin_allowed(struct folio *folio)
+{
+	struct address_space *mapping;
+
+	/*
+	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
+	 * to disappear from under us, as well as preventing RCU grace periods
+	 * from making progress (i.e. implying rcu_read_lock()).
+	 *
+	 * This means we can rely on the folio remaining stable for all
+	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
+	 * and those that do not.
+	 *
+	 * We get the added benefit that given inodes, and thus address_space,
+	 * objects are RCU freed, we can rely on the mapping remaining stable
+	 * here with no risk of a truncation or similar race.
+	 */
+	lockdep_assert_irqs_disabled();
+
+	/*
+	 * If no mapping can be found, this implies an anonymous or otherwise
+	 * non-file backed folio so in this instance we permit the pin.
+	 *
+	 * shmem and hugetlb mappings do not require dirty-tracking so we
+	 * explicitly whitelist these.
+	 *

mm/gup.c static inline struct folio *try_get_folio(struct page *page, int refs) · line 143

Quoted 5 lines

mm/gup.c +5 -0

+	 * Other non dirty-tracked folios will be picked up on the slow path.
+	 */
+	mapping = folio_mapping(folio);
+	return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);
+}

BTW, try_grab_folio() is also called from follow_hugetlb_page(), which is ordinary GUP and has interrupts enabled if I am not wrong.

--
Thanks,

David / dhildenb

Writing to file-backed mappings which require folio dirty tracking using GUP is a fundamentally broken operation, as kernel write access to GUP mappings do not adhere to the semantics expected by a file system.

A GUP caller uses the direct mapping to access the folio, which does not cause write notify to trigger, nor does it enforce that the caller marks the folio dirty.

The problem arises when, after an initial write to the folio, writeback results in the folio being cleaned and then the caller, via the GUP interface, writes to the folio again.

As a result of the use of this secondary, direct, mapping to the folio no write notify will occur, and if the caller does mark the folio dirty, this will be done so unexpectedly.

For example, consider the following scenario:-

1. A folio is written to via GUP which write-faults the memory, notifying the file system and dirtying the folio.
2. Later, writeback is triggered, resulting in the folio being cleaned and the PTE being marked read-only.
3. The GUP caller writes to the folio, as it is mapped read/write via the direct mapping.
4. The GUP caller, now done with the page, unpins it and sets it dirty (though it does not have to).

This change updates both the PUP FOLL_LONGTERM slow and fast APIs. As pin_user_pages_fast_only() does not exist, we can rely on a slightly imperfect whitelisting in the PUP-fast case and fall back to the slow case should this fail.

v7:
- Fixed very silly bug in writeable_file_mapping_allowed() inverting the logic.
- Removed unnecessary RCU lock code and replaced with adaptation of Peter's idea.
- Removed unnecessary open-coded folio_test_anon() in folio_longterm_write_pin_allowed() and restructured to generally permit NULL folio_mapping().

Quoted 32 lines · Lorenzo Stoakes

Writing to file-backed mappings which require folio dirty tracking using GUP is a fundamentally broken operation, as kernel write access to GUP mappings do not adhere to the semantics expected by a file system.

A GUP caller uses the direct mapping to access the folio, which does not cause write notify to trigger, nor does it enforce that the caller marks the folio dirty.

The problem arises when, after an initial write to the folio, writeback results in the folio being cleaned and then the caller, via the GUP interface, writes to the folio again.

As a result of the use of this secondary, direct, mapping to the folio no write notify will occur, and if the caller does mark the folio dirty, this will be done so unexpectedly.

For example, consider the following scenario:-

1. A folio is written to via GUP which write-faults the memory, notifying the file system and dirtying the folio.
2. Later, writeback is triggered, resulting in the folio being cleaned and the PTE being marked read-only.
3. The GUP caller writes to the folio, as it is mapped read/write via the direct mapping.
4. The GUP caller, now done with the page, unpins it and sets it dirty (though it does not have to).

This change updates both the PUP FOLL_LONGTERM slow and fast APIs. As pin_user_pages_fast_only() does not exist, we can rely on a slightly imperfect whitelisting in the PUP-fast case and fall back to the slow case should this fail.

Quoted 9 lines

v7:
- Fixed very silly bug in writeable_file_mapping_allowed() inverting the logic.
- Removed unnecessary RCU lock code and replaced with adaptation of Peter's idea.
- Removed unnecessary open-coded folio_test_anon() in folio_longterm_write_pin_allowed() and restructured to generally permit NULL folio_mapping().

FWIW, I realize you are planning another respin, but I went and tried this version out on s390 -- Now when using a memory backend file and vfio-pci on s390 I see vfio_pin_pages_remote failing consistently. However, the pin_user_pages_fast(FOLL_WRITE | FOLL_LONGTERM) in kvm_s390_pci_aif_enable will still return positive.