Re: [PATCH v5] mm/gup: disallow GUP writing to file-backed mappings by default
From: David Hildenbrand
Date: Fri Apr 28 2023 - 10:21:38 EST
Sorry for jumping in late, I'm on vacation :)
On 28.04.23 01:42, Lorenzo Stoakes wrote:
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.
How should we enforce it? It would be a BUG in the GUP user.
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.
Right, in mprotect() code we only allow upgrading write permissions in
this case if the pte is dirty, so we always go via the pagefault path.
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.
How would that be triggered? Would that writeback triggered by e.g.,
fsync that Jan tried to tackle recently?
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.
As part of this change we separate out vma_needs_dirty_tracking() as a
helper function to determine this which is distinct from
vma_wants_writenotify() which is specific to determining which PTE flags to
set.
[1]:https://lore.kernel.org/linux-mm/20180103100430.GE4911@xxxxxxxxxxxxxx/
This change has the potential to break existing setups. Simple example:
libvirt domains configured for file-backed VM memory that also has a
vfio device configured. It can easily be configured by users (evolving
VM configuration, copy-paste etc.). And it works from a VM perspective,
because the guest memory is essentially stale once the VM is shutdown
and the pages were unpinned. At least we're not concerned about stale
data on disk.
With your changes, such VMs would no longer start, breaking existing
user setups with a kernel update.
I don't really see a lot of reasons to perform this change now. It's
been known to be problematic for a long time. People are working on a
fix (I see Jan is already CCed, CCing Dave and Christop). FOLL_LONGTERM
check is only handling some of the problematic cases, so it's not even a
complete blocker.
I know, Jason und John will disagree, but I don't think we want to be
very careful with changing the default.
Sure, we could warn, or convert individual users using a flag
(io_uring). But maybe we should invest more energy on a fix?
Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
---
include/linux/mm.h | 1 +
mm/gup.c | 41 ++++++++++++++++++++++++++++++++++++++++-
mm/mmap.c | 36 +++++++++++++++++++++++++++---------
3 files changed, 68 insertions(+), 10 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 37554b08bb28..f7da02fc89c6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2433,6 +2433,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)
{
diff --git a/mm/gup.c b/mm/gup.c
index 1f72a717232b..d36a5db9feb1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -959,16 +959,51 @@ 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).
+ */
+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. */
+ if (!(gup_flags & (FOLL_GET | FOLL_PIN)))
+ return true;
FOLL_LONGTERM only applies to FOLL_PIN. This check can be dropped.
+
+ /* We limit this check to the most egregious case - a long term pin. */
+ if (!(gup_flags & FOLL_LONGTERM))
+ return true;
+
+ /* If the VMA requires dirty tracking then GUP will be problematic. */
+ return vma_needs_dirty_tracking(vma);
+}
+
static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
{
vm_flags_t vm_flags = vma->vm_flags;
int write = (gup_flags & FOLL_WRITE);
int foreign = (gup_flags & FOLL_REMOTE);
+ bool vma_anon = vma_is_anonymous(vma);
if (vm_flags & (VM_IO | VM_PFNMAP))
return -EFAULT;
- if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma))
+ if ((gup_flags & FOLL_ANON) && !vma_anon)
return -EFAULT;
if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
@@ -978,6 +1013,10 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
return -EFAULT;
if (write) {
+ if (!vma_anon &&
+ !writeable_file_mapping_allowed(vma, gup_flags))
+ return -EFAULT;
+
if (!(vm_flags & VM_WRITE)) {
if (!(gup_flags & FOLL_FORCE))
return -EFAULT;
diff --git a/mm/mmap.c b/mm/mmap.c
index 536bbb8fa0ae..7b6344d1832a 100644
--- a/mm/mmap.c
I'm probably missing something, why don't we have to handle GUP-fast
(having said that, it's hard to handle ;) )? The sequence you describe
above should apply to GUP-fast as well, no?
1) Pin writable mapped page using GUP-fast
2) Trigger writeback
3) Write to page via pin
4) Unpin and set dirty
--
Thanks,
David / dhildenb