| Message ID | a690186fc37e1ea92556a7dbd0887fe201fcc709.1683067198.git.lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | mm/gup: disallow GUP writing to file-backed mappings by default | expand |
On Tue, 2 May 2023 23:51:35 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote: > Writing to file-backed dirty-tracked mappings via GUP is inherently broken > as we cannot rule out folios being cleaned and then a GUP user writing to > them again and possibly marking them dirty unexpectedly. > > This is especially egregious for long-term mappings (as indicated by the > use of the FOLL_LONGTERM flag), so we disallow this case in GUP-fast as > we have already done in the slow path. > > We have access to less information in the fast path as we cannot examine > the VMA containing the mapping, however we can determine whether the folio > is anonymous or belonging to a whitelisted filesystem - specifically > hugetlb and shmem mappings. > > We take special care to ensure that both the folio and mapping are safe to > access when performing these checks and document folio_fast_pin_allowed() > accordingly. > > It's important to note that there are no APIs allowing users to specify > FOLL_FAST_ONLY for a PUP-fast let alone with FOLL_LONGTERM, so we can > always rely on the fact that if we fail to pin on the fast path, the code > will fall back to the slow path which can perform the more thorough check. arm allnoconfig said mm/gup.c:115:13: warning: 'folio_fast_pin_allowed' defined but not used [-Wunused-function] 115 | static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) | ^~~~~~~~~~~~~~~~~~~~~~ so I moved the definition inside CONFIG_ARCH_HAS_PTE_SPECIAL. mm/gup.c | 154 ++++++++++++++++++++++++++--------------------------- 1 file changed, 77 insertions(+), 77 deletions(-) --- a/mm/gup.c~mm-gup-disallow-foll_longterm-gup-fast-writing-to-file-backed-mappings-fix +++ a/mm/gup.c @@ -96,83 +96,6 @@ retry: return folio; } -/* - * Used in the GUP-fast path to determine whether a pin is permitted for a - * specific folio. - * - * This call assumes the caller has pinned the folio, that the lowest page table - * level still points to this folio, and that interrupts have been disabled. - * - * Writing to pinned file-backed dirty tracked folios is inherently problematic - * (see comment describing the writable_file_mapping_allowed() function). We - * therefore try to avoid the most egregious case of a long-term mapping doing - * so. - * - * This function cannot be as thorough as that one as the VMA is not available - * in the fast path, so instead we whitelist known good cases and if in doubt, - * fall back to the slow path. - */ -static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) -{ - struct address_space *mapping; - unsigned long mapping_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 one we disallow. - */ - if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != - (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) - return true; - - /* The folio is pinned, so we can safely access folio fields. */ - - /* Neither of these should be possible, but check to be sure. */ - if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) - return false; - - /* hugetlb mappings do not require dirty-tracking. */ - if (folio_test_hugetlb(folio)) - return true; - - /* - * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods - * cannot proceed, which means no actions performed under RCU can - * proceed either. - * - * inodes and thus their mappings are freed under RCU, which means the - * mapping cannot be freed beneath us and thus we can safely dereference - * it. - */ - lockdep_assert_irqs_disabled(); - - /* - * However, there may be operations which _alter_ the mapping, so ensure - * we read it once and only once. - */ - mapping = READ_ONCE(folio->mapping); - - /* - * The mapping may have been truncated, in any case we cannot determine - * if this mapping is safe - fall back to slow path to determine how to - * proceed. - */ - if (!mapping) - return false; - - /* Anonymous folios are fine, other non-file backed cases are not. */ - mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; - if (mapping_flags) - return mapping_flags == PAGE_MAPPING_ANON; - - /* - * At this point, we know the mapping is non-null and points to an - * address_space object. The only remaining whitelisted file system is - * shmem. - */ - return shmem_mapping(mapping); -} - /** * try_grab_folio() - Attempt to get or pin a folio. * @page: pointer to page to be grabbed @@ -2474,6 +2397,83 @@ static void __maybe_unused undo_dev_page #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL /* + * Used in the GUP-fast path to determine whether a pin is permitted for a + * specific folio. + * + * This call assumes the caller has pinned the folio, that the lowest page table + * level still points to this folio, and that interrupts have been disabled. + * + * Writing to pinned file-backed dirty tracked folios is inherently problematic + * (see comment describing the writable_file_mapping_allowed() function). We + * therefore try to avoid the most egregious case of a long-term mapping doing + * so. + * + * This function cannot be as thorough as that one as the VMA is not available + * in the fast path, so instead we whitelist known good cases and if in doubt, + * fall back to the slow path. + */ +static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) +{ + struct address_space *mapping; + unsigned long mapping_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 one we disallow. + */ + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != + (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) + return true; + + /* The folio is pinned, so we can safely access folio fields. */ + + /* Neither of these should be possible, but check to be sure. */ + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) + return false; + + /* hugetlb mappings do not require dirty-tracking. */ + if (folio_test_hugetlb(folio)) + return true; + + /* + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods + * cannot proceed, which means no actions performed under RCU can + * proceed either. + * + * inodes and thus their mappings are freed under RCU, which means the + * mapping cannot be freed beneath us and thus we can safely dereference + * it. + */ + lockdep_assert_irqs_disabled(); + + /* + * However, there may be operations which _alter_ the mapping, so ensure + * we read it once and only once. + */ + mapping = READ_ONCE(folio->mapping); + + /* + * The mapping may have been truncated, in any case we cannot determine + * if this mapping is safe - fall back to slow path to determine how to + * proceed. + */ + if (!mapping) + return false; + + /* Anonymous folios are fine, other non-file backed cases are not. */ + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; + if (mapping_flags) + return mapping_flags == PAGE_MAPPING_ANON; + + /* + * At this point, we know the mapping is non-null and points to an + * address_space object. The only remaining whitelisted file system is + * shmem. + */ + return shmem_mapping(mapping); +} + +/* * Fast-gup relies on pte change detection to avoid concurrent pgtable * operations. *
On Tue 02-05-23 23:51:35, Lorenzo Stoakes wrote: > Writing to file-backed dirty-tracked mappings via GUP is inherently broken > as we cannot rule out folios being cleaned and then a GUP user writing to > them again and possibly marking them dirty unexpectedly. > > This is especially egregious for long-term mappings (as indicated by the > use of the FOLL_LONGTERM flag), so we disallow this case in GUP-fast as > we have already done in the slow path. > > We have access to less information in the fast path as we cannot examine > the VMA containing the mapping, however we can determine whether the folio > is anonymous or belonging to a whitelisted filesystem - specifically > hugetlb and shmem mappings. > > We take special care to ensure that both the folio and mapping are safe to > access when performing these checks and document folio_fast_pin_allowed() > accordingly. > > It's important to note that there are no APIs allowing users to specify > FOLL_FAST_ONLY for a PUP-fast let alone with FOLL_LONGTERM, so we can > always rely on the fact that if we fail to pin on the fast path, the code > will fall back to the slow path which can perform the more thorough check. > > Suggested-by: David Hildenbrand <david@redhat.com> > Suggested-by: Kirill A . Shutemov <kirill@shutemov.name> > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> The patch looks good to me now. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > mm/gup.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 102 insertions(+) > > diff --git a/mm/gup.c b/mm/gup.c > index 0ea9ebec9547..1ab369b5d889 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -18,6 +18,7 @@ > #include <linux/migrate.h> > #include <linux/mm_inline.h> > #include <linux/sched/mm.h> > +#include <linux/shmem_fs.h> > > #include <asm/mmu_context.h> > #include <asm/tlbflush.h> > @@ -95,6 +96,83 @@ static inline struct folio *try_get_folio(struct page *page, int refs) > return folio; > } > > +/* > + * Used in the GUP-fast path to determine whether a pin is permitted for a > + * specific folio. > + * > + * This call assumes the caller has pinned the folio, that the lowest page table > + * level still points to this folio, and that interrupts have been disabled. > + * > + * Writing to pinned file-backed dirty tracked folios is inherently problematic > + * (see comment describing the writable_file_mapping_allowed() function). We > + * therefore try to avoid the most egregious case of a long-term mapping doing > + * so. > + * > + * This function cannot be as thorough as that one as the VMA is not available > + * in the fast path, so instead we whitelist known good cases and if in doubt, > + * fall back to the slow path. > + */ > +static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > +{ > + struct address_space *mapping; > + unsigned long mapping_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 one we disallow. > + */ > + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != > + (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) > + return true; > + > + /* The folio is pinned, so we can safely access folio fields. */ > + > + /* Neither of these should be possible, but check to be sure. */ > + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) > + return false; > + > + /* hugetlb mappings do not require dirty-tracking. */ > + if (folio_test_hugetlb(folio)) > + return true; > + > + /* > + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods > + * cannot proceed, which means no actions performed under RCU can > + * proceed either. > + * > + * inodes and thus their mappings are freed under RCU, which means the > + * mapping cannot be freed beneath us and thus we can safely dereference > + * it. > + */ > + lockdep_assert_irqs_disabled(); > + > + /* > + * However, there may be operations which _alter_ the mapping, so ensure > + * we read it once and only once. > + */ > + mapping = READ_ONCE(folio->mapping); > + > + /* > + * The mapping may have been truncated, in any case we cannot determine > + * if this mapping is safe - fall back to slow path to determine how to > + * proceed. > + */ > + if (!mapping) > + return false; > + > + /* Anonymous folios are fine, other non-file backed cases are not. */ > + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; > + if (mapping_flags) > + return mapping_flags == PAGE_MAPPING_ANON; > + > + /* > + * At this point, we know the mapping is non-null and points to an > + * address_space object. The only remaining whitelisted file system is > + * shmem. > + */ > + return shmem_mapping(mapping); > +} > + > /** > * try_grab_folio() - Attempt to get or pin a folio. > * @page: pointer to page to be grabbed > @@ -2464,6 +2542,11 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > goto pte_unmap; > } > > + if (!folio_fast_pin_allowed(folio, flags)) { > + gup_put_folio(folio, 1, flags); > + goto pte_unmap; > + } > + > if (!pte_write(pte) && gup_must_unshare(NULL, flags, page)) { > gup_put_folio(folio, 1, flags); > goto pte_unmap; > @@ -2656,6 +2739,11 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, > return 0; > } > > + if (!folio_fast_pin_allowed(folio, flags)) { > + gup_put_folio(folio, refs, flags); > + return 0; > + } > + > if (!pte_write(pte) && gup_must_unshare(NULL, flags, &folio->page)) { > gup_put_folio(folio, refs, flags); > return 0; > @@ -2722,6 +2810,10 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr, > return 0; > } > > + if (!folio_fast_pin_allowed(folio, flags)) { > + gup_put_folio(folio, refs, flags); > + return 0; > + } > if (!pmd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) { > gup_put_folio(folio, refs, flags); > return 0; > @@ -2762,6 +2854,11 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr, > return 0; > } > > + if (!folio_fast_pin_allowed(folio, flags)) { > + gup_put_folio(folio, refs, flags); > + return 0; > + } > + > if (!pud_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) { > gup_put_folio(folio, refs, flags); > return 0; > @@ -2797,6 +2894,11 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr, > return 0; > } > > + if (!folio_fast_pin_allowed(folio, flags)) { > + gup_put_folio(folio, refs, flags); > + return 0; > + } > + > *nr += refs; > folio_set_referenced(folio); > return 1; > -- > 2.40.1 >
[...] > +static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > +{ > + struct address_space *mapping; > + unsigned long mapping_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 one we disallow. > + */ > + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != > + (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) > + return true; > + > + /* The folio is pinned, so we can safely access folio fields. */ > + > + /* Neither of these should be possible, but check to be sure. */ You can easily have anon pages that are at the swapcache at this point (especially, because this function is called before our unsharing checks), the comment is misleading. And there is nothing wrong about pinning an anon page that's still in the swapcache. The following folio_test_anon() check will allow them. The check made sense in page_mapping(), but here it's not required. I do agree regarding folio_test_slab(), though. Should we WARN in case we would have one? if (WARN_ON_ONCE(folio_test_slab(folio))) return false; > + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) > + return false; > + > + /* hugetlb mappings do not require dirty-tracking. */ > + if (folio_test_hugetlb(folio)) > + return true; > + > + /* > + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods > + * cannot proceed, which means no actions performed under RCU can > + * proceed either. > + * > + * inodes and thus their mappings are freed under RCU, which means the > + * mapping cannot be freed beneath us and thus we can safely dereference > + * it. > + */ > + lockdep_assert_irqs_disabled(); > + > + /* > + * However, there may be operations which _alter_ the mapping, so ensure > + * we read it once and only once. > + */ > + mapping = READ_ONCE(folio->mapping); > + > + /* > + * The mapping may have been truncated, in any case we cannot determine > + * if this mapping is safe - fall back to slow path to determine how to > + * proceed. > + */ > + if (!mapping) > + return false; > + > + /* Anonymous folios are fine, other non-file backed cases are not. */ > + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; > + if (mapping_flags) > + return mapping_flags == PAGE_MAPPING_ANON; KSM pages are also (shared) anonymous folios, and that check would fail -- which is ok (the following unsharing checks rejects long-term pinning them), but a bit inconstent with your comment and folio_test_anon(). It would be more consistent (with your comment and also the folio_test_anon implementation) to have here: return mapping_flags & PAGE_MAPPING_ANON; > + > + /* > + * At this point, we know the mapping is non-null and points to an > + * address_space object. The only remaining whitelisted file system is > + * shmem. > + */ > + return shmem_mapping(mapping); > +} > + In general, LGTM Acked-by: David Hildenbrand <david@redhat.com>
On Tue, May 02, 2023 at 07:18:21PM -0700, Andrew Morton wrote: > On Tue, 2 May 2023 23:51:35 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote: > > > Writing to file-backed dirty-tracked mappings via GUP is inherently broken > > as we cannot rule out folios being cleaned and then a GUP user writing to > > them again and possibly marking them dirty unexpectedly. > > > > This is especially egregious for long-term mappings (as indicated by the > > use of the FOLL_LONGTERM flag), so we disallow this case in GUP-fast as > > we have already done in the slow path. > > > > We have access to less information in the fast path as we cannot examine > > the VMA containing the mapping, however we can determine whether the folio > > is anonymous or belonging to a whitelisted filesystem - specifically > > hugetlb and shmem mappings. > > > > We take special care to ensure that both the folio and mapping are safe to > > access when performing these checks and document folio_fast_pin_allowed() > > accordingly. > > > > It's important to note that there are no APIs allowing users to specify > > FOLL_FAST_ONLY for a PUP-fast let alone with FOLL_LONGTERM, so we can > > always rely on the fact that if we fail to pin on the fast path, the code > > will fall back to the slow path which can perform the more thorough check. > > arm allnoconfig said > > mm/gup.c:115:13: warning: 'folio_fast_pin_allowed' defined but not used [-Wunused-function] > 115 | static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > | ^~~~~~~~~~~~~~~~~~~~~~ > > so I moved the definition inside CONFIG_ARCH_HAS_PTE_SPECIAL. > > > > mm/gup.c | 154 ++++++++++++++++++++++++++--------------------------- > 1 file changed, 77 insertions(+), 77 deletions(-) > > --- a/mm/gup.c~mm-gup-disallow-foll_longterm-gup-fast-writing-to-file-backed-mappings-fix > +++ a/mm/gup.c > @@ -96,83 +96,6 @@ retry: > return folio; > } > > -/* > - * Used in the GUP-fast path to determine whether a pin is permitted for a > - * specific folio. > - * > - * This call assumes the caller has pinned the folio, that the lowest page table > - * level still points to this folio, and that interrupts have been disabled. > - * > - * Writing to pinned file-backed dirty tracked folios is inherently problematic > - * (see comment describing the writable_file_mapping_allowed() function). We > - * therefore try to avoid the most egregious case of a long-term mapping doing > - * so. > - * > - * This function cannot be as thorough as that one as the VMA is not available > - * in the fast path, so instead we whitelist known good cases and if in doubt, > - * fall back to the slow path. > - */ > -static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > -{ > - struct address_space *mapping; > - unsigned long mapping_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 one we disallow. > - */ > - if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != > - (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) > - return true; > - > - /* The folio is pinned, so we can safely access folio fields. */ > - > - /* Neither of these should be possible, but check to be sure. */ > - if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) > - return false; > - > - /* hugetlb mappings do not require dirty-tracking. */ > - if (folio_test_hugetlb(folio)) > - return true; > - > - /* > - * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods > - * cannot proceed, which means no actions performed under RCU can > - * proceed either. > - * > - * inodes and thus their mappings are freed under RCU, which means the > - * mapping cannot be freed beneath us and thus we can safely dereference > - * it. > - */ > - lockdep_assert_irqs_disabled(); > - > - /* > - * However, there may be operations which _alter_ the mapping, so ensure > - * we read it once and only once. > - */ > - mapping = READ_ONCE(folio->mapping); > - > - /* > - * The mapping may have been truncated, in any case we cannot determine > - * if this mapping is safe - fall back to slow path to determine how to > - * proceed. > - */ > - if (!mapping) > - return false; > - > - /* Anonymous folios are fine, other non-file backed cases are not. */ > - mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; > - if (mapping_flags) > - return mapping_flags == PAGE_MAPPING_ANON; > - > - /* > - * At this point, we know the mapping is non-null and points to an > - * address_space object. The only remaining whitelisted file system is > - * shmem. > - */ > - return shmem_mapping(mapping); > -} > - > /** > * try_grab_folio() - Attempt to get or pin a folio. > * @page: pointer to page to be grabbed > @@ -2474,6 +2397,83 @@ static void __maybe_unused undo_dev_page > > #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL > /* > + * Used in the GUP-fast path to determine whether a pin is permitted for a > + * specific folio. > + * > + * This call assumes the caller has pinned the folio, that the lowest page table > + * level still points to this folio, and that interrupts have been disabled. > + * > + * Writing to pinned file-backed dirty tracked folios is inherently problematic > + * (see comment describing the writable_file_mapping_allowed() function). We > + * therefore try to avoid the most egregious case of a long-term mapping doing > + * so. > + * > + * This function cannot be as thorough as that one as the VMA is not available > + * in the fast path, so instead we whitelist known good cases and if in doubt, > + * fall back to the slow path. > + */ > +static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > +{ > + struct address_space *mapping; > + unsigned long mapping_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 one we disallow. > + */ > + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != > + (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) > + return true; > + > + /* The folio is pinned, so we can safely access folio fields. */ > + > + /* Neither of these should be possible, but check to be sure. */ > + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) > + return false; > + > + /* hugetlb mappings do not require dirty-tracking. */ > + if (folio_test_hugetlb(folio)) > + return true; > + > + /* > + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods > + * cannot proceed, which means no actions performed under RCU can > + * proceed either. > + * > + * inodes and thus their mappings are freed under RCU, which means the > + * mapping cannot be freed beneath us and thus we can safely dereference > + * it. > + */ > + lockdep_assert_irqs_disabled(); > + > + /* > + * However, there may be operations which _alter_ the mapping, so ensure > + * we read it once and only once. > + */ > + mapping = READ_ONCE(folio->mapping); > + > + /* > + * The mapping may have been truncated, in any case we cannot determine > + * if this mapping is safe - fall back to slow path to determine how to > + * proceed. > + */ > + if (!mapping) > + return false; > + > + /* Anonymous folios are fine, other non-file backed cases are not. */ > + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; > + if (mapping_flags) > + return mapping_flags == PAGE_MAPPING_ANON; > + > + /* > + * At this point, we know the mapping is non-null and points to an > + * address_space object. The only remaining whitelisted file system is > + * shmem. > + */ > + return shmem_mapping(mapping); > +} > + > +/* > * Fast-gup relies on pte change detection to avoid concurrent pgtable > * operations. > * > _ > Ack thanks for this, I think it doesn't quite cover all cases (kernel bot moaning on -next for mips), needs some more fiddly #ifdef stuff, I will spin a v9 shortly anyway to fix this up and address a few other minor things.
On Thu, May 04, 2023 at 05:04:34PM +0200, David Hildenbrand wrote: > [...] > > > +static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > > +{ > > + struct address_space *mapping; > > + unsigned long mapping_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 one we disallow. > > + */ > > + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != > > + (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) > > + return true; > > + > > + /* The folio is pinned, so we can safely access folio fields. */ > > + > > + /* Neither of these should be possible, but check to be sure. */ > > You can easily have anon pages that are at the swapcache at this point > (especially, because this function is called before our unsharing checks), > the comment is misleading. Ack will update. > > And there is nothing wrong about pinning an anon page that's still in the > swapcache. The following folio_test_anon() check will allow them. > > The check made sense in page_mapping(), but here it's not required. Waaaaaaaaaait a second, you were saying before:- "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 are we allowing or disallowing pinning anon swap cache pages? :P I mean slow path would allow them if they are just marked anon so I'm inclined to allow them. > > I do agree regarding folio_test_slab(), though. Should we WARN in case we > would have one? > > if (WARN_ON_ONCE(folio_test_slab(folio))) > return false; > God help us if we have a slab page at this point, so agreed worth doing, it would surely have to arise from some dreadful bug/memory corruption. > > + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) > > + return false; > > + > > + /* hugetlb mappings do not require dirty-tracking. */ > > + if (folio_test_hugetlb(folio)) > > + return true; > > + > > + /* > > + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods > > + * cannot proceed, which means no actions performed under RCU can > > + * proceed either. > > + * > > + * inodes and thus their mappings are freed under RCU, which means the > > + * mapping cannot be freed beneath us and thus we can safely dereference > > + * it. > > + */ > > + lockdep_assert_irqs_disabled(); > > + > > + /* > > + * However, there may be operations which _alter_ the mapping, so ensure > > + * we read it once and only once. > > + */ > > + mapping = READ_ONCE(folio->mapping); > > + > > + /* > > + * The mapping may have been truncated, in any case we cannot determine > > + * if this mapping is safe - fall back to slow path to determine how to > > + * proceed. > > + */ > > + if (!mapping) > > + return false; > > + > > + /* Anonymous folios are fine, other non-file backed cases are not. */ > > + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; > > + if (mapping_flags) > > + return mapping_flags == PAGE_MAPPING_ANON; > > KSM pages are also (shared) anonymous folios, and that check would fail -- > which is ok (the following unsharing checks rejects long-term pinning them), > but a bit inconstent with your comment and folio_test_anon(). > > It would be more consistent (with your comment and also the folio_test_anon > implementation) to have here: > > return mapping_flags & PAGE_MAPPING_ANON; > I explicitly excluded KSM out of fear that could be some breakage given they're wrprotect'd + expected to CoW though? But I guess you mean they'd get picked up by the unshare and so it doesn't matter + we wouldn't want to exclude an PG_anon_exclusive case? I'll make the change in any case given the unshare check! I notice the gup_huge_pgd() doesn't do an unshare but I mean, a PGD-sized huge page probably isn't going to be CoW'd :P > > + > > + /* > > + * At this point, we know the mapping is non-null and points to an > > + * address_space object. The only remaining whitelisted file system is > > + * shmem. > > + */ > > + return shmem_mapping(mapping); > > +} > > + > > In general, LGTM > > Acked-by: David Hildenbrand <david@redhat.com> > Thanks! Will respin, addressing your comments and addressing the issue the kernel bot picked up with placement in the appropriate #ifdef's and send out a v9 shortly. > -- > Thanks, > > David / dhildenb >
>> And there is nothing wrong about pinning an anon page that's still in the >> swapcache. The following folio_test_anon() check will allow them. >> >> The check made sense in page_mapping(), but here it's not required. > > Waaaaaaaaaait a second, you were saying before:- > > "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 are we allowing or disallowing pinning anon swap cache pages? :P If we have an exclusive anon page that's still in the swap cache, sure! :) I think there are ways that can be done, and nothing would actually break. (I even wrote selftests in the cow selftests for that to amke sure it works as expected) > > I mean slow path would allow them if they are just marked anon so I'm inclined > to allow them. Exactly my reasoning. The less checks the better (especially if ordinary GUP just allows for pinning it) :) > >> >> I do agree regarding folio_test_slab(), though. Should we WARN in case we >> would have one? >> >> if (WARN_ON_ONCE(folio_test_slab(folio))) >> return false; >> > > God help us if we have a slab page at this point, so agreed worth doing, it > would surely have to arise from some dreadful bug/memory corruption. > Or some nasty race condition that we managed to ignore with rechecking if the PTEs/PMDs changed :) >>> + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) >>> + return false; >>> + >>> + /* hugetlb mappings do not require dirty-tracking. */ >>> + if (folio_test_hugetlb(folio)) >>> + return true; >>> + >>> + /* >>> + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods >>> + * cannot proceed, which means no actions performed under RCU can >>> + * proceed either. >>> + * >>> + * inodes and thus their mappings are freed under RCU, which means the >>> + * mapping cannot be freed beneath us and thus we can safely dereference >>> + * it. >>> + */ >>> + lockdep_assert_irqs_disabled(); >>> + >>> + /* >>> + * However, there may be operations which _alter_ the mapping, so ensure >>> + * we read it once and only once. >>> + */ >>> + mapping = READ_ONCE(folio->mapping); >>> + >>> + /* >>> + * The mapping may have been truncated, in any case we cannot determine >>> + * if this mapping is safe - fall back to slow path to determine how to >>> + * proceed. >>> + */ >>> + if (!mapping) >>> + return false; >>> + >>> + /* Anonymous folios are fine, other non-file backed cases are not. */ >>> + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; >>> + if (mapping_flags) >>> + return mapping_flags == PAGE_MAPPING_ANON; >> >> KSM pages are also (shared) anonymous folios, and that check would fail -- >> which is ok (the following unsharing checks rejects long-term pinning them), >> but a bit inconstent with your comment and folio_test_anon(). >> >> It would be more consistent (with your comment and also the folio_test_anon >> implementation) to have here: >> >> return mapping_flags & PAGE_MAPPING_ANON; >> > > I explicitly excluded KSM out of fear that could be some breakage given they're > wrprotect'd + expected to CoW though? But I guess you mean they'd get picked up > by the unshare and so it doesn't matter + we wouldn't want to exclude an > PG_anon_exclusive case? Yes, unsharing handles that in the ordinary GUP and GUP-fast case. And unsharing is neither GUP-fast nor even longterm specific (for anon pages). Reason I'm brining this up is that I think it's best if we let folio_fast_pin_allowed() just check for what's absolutely GUP-fast specific. > > I'll make the change in any case given the unshare check! > > I notice the gup_huge_pgd() doesn't do an unshare but I mean, a PGD-sized huge > page probably isn't going to be CoW'd :P I spotted exactly the same thing and wondered about that (after all I added all that unsharing logic ... so I should know). I'm sure there must be a reason I didn't add it ;) ... probably we should just add it even though it might essentially be dead code for now (at least the cow selftests would try with each and every hugetlb size and eventually reveal the problem on whatever arch ends up using that code ... ). Do you want to send a patch to add unsharing to gup_huge_pgd() as well?
On Fri, May 05, 2023 at 04:17:38PM +0200, David Hildenbrand wrote: > > > And there is nothing wrong about pinning an anon page that's still in the > > > swapcache. The following folio_test_anon() check will allow them. > > > > > > The check made sense in page_mapping(), but here it's not required. > > > > Waaaaaaaaaait a second, you were saying before:- > > > > "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 are we allowing or disallowing pinning anon swap cache pages? :P > > If we have an exclusive anon page that's still in the swap cache, sure! :) > > I think there are ways that can be done, and nothing would actually break. > (I even wrote selftests in the cow selftests for that to amke sure it works > as expected) > > > > > I mean slow path would allow them if they are just marked anon so I'm inclined > > to allow them. > > Exactly my reasoning. > > The less checks the better (especially if ordinary GUP just allows for > pinning it) :) Yeah a lot of my decision making on this has been trying to be conservative about what we filter for but you get this weird inversion whereby if you're too conservative about what you allow, you are actually being more outlandish about what you disallow and vice-versa. > > > > > > > > > I do agree regarding folio_test_slab(), though. Should we WARN in case we > > > would have one? > > > > > > if (WARN_ON_ONCE(folio_test_slab(folio))) > > > return false; > > > > > > > God help us if we have a slab page at this point, so agreed worth doing, it > > would surely have to arise from some dreadful bug/memory corruption. > > > > Or some nasty race condition that we managed to ignore with rechecking if > the PTEs/PMDs changed :) Well that should be sorted now :) > > > > > + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) > > > > + return false; > > > > + > > > > + /* hugetlb mappings do not require dirty-tracking. */ > > > > + if (folio_test_hugetlb(folio)) > > > > + return true; > > > > + > > > > + /* > > > > + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods > > > > + * cannot proceed, which means no actions performed under RCU can > > > > + * proceed either. > > > > + * > > > > + * inodes and thus their mappings are freed under RCU, which means the > > > > + * mapping cannot be freed beneath us and thus we can safely dereference > > > > + * it. > > > > + */ > > > > + lockdep_assert_irqs_disabled(); > > > > + > > > > + /* > > > > + * However, there may be operations which _alter_ the mapping, so ensure > > > > + * we read it once and only once. > > > > + */ > > > > + mapping = READ_ONCE(folio->mapping); > > > > + > > > > + /* > > > > + * The mapping may have been truncated, in any case we cannot determine > > > > + * if this mapping is safe - fall back to slow path to determine how to > > > > + * proceed. > > > > + */ > > > > + if (!mapping) > > > > + return false; > > > > + > > > > + /* Anonymous folios are fine, other non-file backed cases are not. */ > > > > + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; > > > > + if (mapping_flags) > > > > + return mapping_flags == PAGE_MAPPING_ANON; > > > > > > KSM pages are also (shared) anonymous folios, and that check would fail -- > > > which is ok (the following unsharing checks rejects long-term pinning them), > > > but a bit inconstent with your comment and folio_test_anon(). > > > > > > It would be more consistent (with your comment and also the folio_test_anon > > > implementation) to have here: > > > > > > return mapping_flags & PAGE_MAPPING_ANON; > > > > > > > I explicitly excluded KSM out of fear that could be some breakage given they're > > wrprotect'd + expected to CoW though? But I guess you mean they'd get picked up > > by the unshare and so it doesn't matter + we wouldn't want to exclude an > > PG_anon_exclusive case? > > Yes, unsharing handles that in the ordinary GUP and GUP-fast case. And > unsharing is neither GUP-fast nor even longterm specific (for anon pages). > > Reason I'm brining this up is that I think it's best if we let > folio_fast_pin_allowed() just check for what's absolutely GUP-fast specific. Ack, indeed it's a separate thing, see above for the contradictory nature of wanting to be cautious but then accidentally making your change _more radical_ than you intended...! > > > > > I'll make the change in any case given the unshare check! > > > > I notice the gup_huge_pgd() doesn't do an unshare but I mean, a PGD-sized huge > > page probably isn't going to be CoW'd :P > > I spotted exactly the same thing and wondered about that (after all I added > all that unsharing logic ... so I should know). I'm sure there must be a > reason I didn't add it ;) > > ... probably we should just add it even though it might essentially be dead > code for now (at least the cow selftests would try with each and every > hugetlb size and eventually reveal the problem on whatever arch ends up > using that code ... ). > > Do you want to send a patch to add unsharing to gup_huge_pgd() as well? > Sure will do! > -- > Thanks, > > David / dhildenb >
diff --git a/mm/gup.c b/mm/gup.c index 0ea9ebec9547..1ab369b5d889 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -18,6 +18,7 @@ #include <linux/migrate.h> #include <linux/mm_inline.h> #include <linux/sched/mm.h> +#include <linux/shmem_fs.h> #include <asm/mmu_context.h> #include <asm/tlbflush.h> @@ -95,6 +96,83 @@ static inline struct folio *try_get_folio(struct page *page, int refs) return folio; } +/* + * Used in the GUP-fast path to determine whether a pin is permitted for a + * specific folio. + * + * This call assumes the caller has pinned the folio, that the lowest page table + * level still points to this folio, and that interrupts have been disabled. + * + * Writing to pinned file-backed dirty tracked folios is inherently problematic + * (see comment describing the writable_file_mapping_allowed() function). We + * therefore try to avoid the most egregious case of a long-term mapping doing + * so. + * + * This function cannot be as thorough as that one as the VMA is not available + * in the fast path, so instead we whitelist known good cases and if in doubt, + * fall back to the slow path. + */ +static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) +{ + struct address_space *mapping; + unsigned long mapping_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 one we disallow. + */ + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != + (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) + return true; + + /* The folio is pinned, so we can safely access folio fields. */ + + /* Neither of these should be possible, but check to be sure. */ + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) + return false; + + /* hugetlb mappings do not require dirty-tracking. */ + if (folio_test_hugetlb(folio)) + return true; + + /* + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods + * cannot proceed, which means no actions performed under RCU can + * proceed either. + * + * inodes and thus their mappings are freed under RCU, which means the + * mapping cannot be freed beneath us and thus we can safely dereference + * it. + */ + lockdep_assert_irqs_disabled(); + + /* + * However, there may be operations which _alter_ the mapping, so ensure + * we read it once and only once. + */ + mapping = READ_ONCE(folio->mapping); + + /* + * The mapping may have been truncated, in any case we cannot determine + * if this mapping is safe - fall back to slow path to determine how to + * proceed. + */ + if (!mapping) + return false; + + /* Anonymous folios are fine, other non-file backed cases are not. */ + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; + if (mapping_flags) + return mapping_flags == PAGE_MAPPING_ANON; + + /* + * At this point, we know the mapping is non-null and points to an + * address_space object. The only remaining whitelisted file system is + * shmem. + */ + return shmem_mapping(mapping); +} + /** * try_grab_folio() - Attempt to get or pin a folio. * @page: pointer to page to be grabbed @@ -2464,6 +2542,11 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, goto pte_unmap; } + if (!folio_fast_pin_allowed(folio, flags)) { + gup_put_folio(folio, 1, flags); + goto pte_unmap; + } + if (!pte_write(pte) && gup_must_unshare(NULL, flags, page)) { gup_put_folio(folio, 1, flags); goto pte_unmap; @@ -2656,6 +2739,11 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, return 0; } + if (!folio_fast_pin_allowed(folio, flags)) { + gup_put_folio(folio, refs, flags); + return 0; + } + if (!pte_write(pte) && gup_must_unshare(NULL, flags, &folio->page)) { gup_put_folio(folio, refs, flags); return 0; @@ -2722,6 +2810,10 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr, return 0; } + if (!folio_fast_pin_allowed(folio, flags)) { + gup_put_folio(folio, refs, flags); + return 0; + } if (!pmd_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) { gup_put_folio(folio, refs, flags); return 0; @@ -2762,6 +2854,11 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr, return 0; } + if (!folio_fast_pin_allowed(folio, flags)) { + gup_put_folio(folio, refs, flags); + return 0; + } + if (!pud_write(orig) && gup_must_unshare(NULL, flags, &folio->page)) { gup_put_folio(folio, refs, flags); return 0; @@ -2797,6 +2894,11 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr, return 0; } + if (!folio_fast_pin_allowed(folio, flags)) { + gup_put_folio(folio, refs, flags); + return 0; + } + *nr += refs; folio_set_referenced(folio); return 1;
Writing to file-backed dirty-tracked mappings via GUP is inherently broken as we cannot rule out folios being cleaned and then a GUP user writing to them again and possibly marking them dirty unexpectedly. This is especially egregious for long-term mappings (as indicated by the use of the FOLL_LONGTERM flag), so we disallow this case in GUP-fast as we have already done in the slow path. We have access to less information in the fast path as we cannot examine the VMA containing the mapping, however we can determine whether the folio is anonymous or belonging to a whitelisted filesystem - specifically hugetlb and shmem mappings. We take special care to ensure that both the folio and mapping are safe to access when performing these checks and document folio_fast_pin_allowed() accordingly. It's important to note that there are no APIs allowing users to specify FOLL_FAST_ONLY for a PUP-fast let alone with FOLL_LONGTERM, so we can always rely on the fact that if we fail to pin on the fast path, the code will fall back to the slow path which can perform the more thorough check. Suggested-by: David Hildenbrand <david@redhat.com> Suggested-by: Kirill A . Shutemov <kirill@shutemov.name> Suggested-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- mm/gup.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+)