[PATCH v5 0/7] Fix some cache flush bugs

[PATCH v5 0/7] Fix some cache flush bugs

[Muchun Song]

This series focus on fixing cache maintenance.

v5:
  - Collect Reviewed-by from Mike.
  - Fix mcopy_atomic_pte() and __mcopy_atomic() (Mike).
  - Fix shmem_mfill_atomic_pte().

v4:
  - Replace folio_copy() with copy_user_huge_page().
  - Update commit message for patch 2.

v3:
  - Collect Reviewed-by tag from Zi Yan.
  - Fix hugetlb cache maintenance.

v2:
  - Collect Reviewed-by tag from Zi Yan.
  - Using a for loop instead of the folio variant for backportability.

Muchun Song (7):
  mm: thp: fix wrong cache flush in remove_migration_pmd()
  mm: fix missing cache flush for all tail pages of compound page
  mm: hugetlb: fix missing cache flush in copy_huge_page_from_user()
  mm: hugetlb: fix missing cache flush in hugetlb_mcopy_atomic_pte()
  mm: shmem: fix missing cache flush in shmem_mfill_atomic_pte()
  mm: userfaultfd: fix missing cache flush in mcopy_atomic_pte() and
    __mcopy_atomic()
  mm: replace multiple dcache flush with flush_dcache_folio()

 mm/huge_memory.c | 3 ++-
 mm/hugetlb.c     | 3 ++-
 mm/memory.c      | 2 ++
 mm/migrate.c     | 3 +--
 mm/shmem.c       | 4 +++-
 mm/userfaultfd.c | 3 +++
 6 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH v5 1/7] mm: thp: fix wrong cache flush in remove_migration_pmd()

[Muchun Song]

The flush_cache_range() is supposed to be justified only if the page
is already placed in process page table, and that is done right after
flush_cache_range(). So using this interface is wrong. And there is
no need to invalite cache since it was non-present before in
remove_migration_pmd(). So just to remove it.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Zi Yan <ziy@nvidia.com>
---
 mm/huge_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index f58524394dc1..45ede45b11f5 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3207,7 +3207,6 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 	if (pmd_swp_uffd_wp(*pvmw->pmd))
 		pmde = pmd_wrprotect(pmd_mkuffd_wp(pmde));
 
-	flush_cache_range(vma, mmun_start, mmun_start + HPAGE_PMD_SIZE);
 	if (PageAnon(new))
 		page_add_anon_rmap(new, vma, mmun_start, true);
 	else
@@ -3215,6 +3214,8 @@ void remove_migration_pmd(struct page_vma_mapped_walk *pvmw, struct page *new)
 	set_pmd_at(mm, mmun_start, pvmw->pmd, pmde);
 	if ((vma->vm_flags & VM_LOCKED) && !PageDoubleMap(new))
 		mlock_vma_page(new);
+
+	/* No need to invalidate - it was non-present before */
 	update_mmu_cache_pmd(vma, address, pvmw->pmd);
 }
 #endif
-- 
2.11.0

[PATCH v5 2/7] mm: fix missing cache flush for all tail pages of compound page

[Muchun Song]

The D-cache maintenance inside move_to_new_page() only consider one page,
there is still D-cache maintenance issue for tail pages of compound page
(e.g. THP or HugeTLB).  THP migration is only enabled on x86_64, ARM64
and powerpc, while powerpc and arm64 need to maintain the consistency
between I-Cache and D-Cache, which depends on flush_dcache_page() to
maintain the consistency between I-Cache and D-Cache.  But there is no
issues on arm64 and powerpc since they already considers the compound
page cache flushing in their icache flush function.  HugeTLB migration
is enabled on arm, arm64, mips, parisc, powerpc, riscv, s390 and sh, while
arm has handled the compound page cache flush in flush_dcache_page(), but
most others do not.  In theory, the issue exists on many architectures.
Fix this by not using flush_dcache_folio() since it is not backportable.

Fixes: 290408d4a250 ("hugetlb: hugepage migration core")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Zi Yan <ziy@nvidia.com>
---
 mm/migrate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c9296d63878d..c418e8d92b9c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -933,9 +933,12 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 		if (!PageMappingFlags(page))
 			page->mapping = NULL;
 
-		if (likely(!is_zone_device_page(newpage)))
-			flush_dcache_page(newpage);
+		if (likely(!is_zone_device_page(newpage))) {
+			int i, nr = compound_nr(newpage);
 
+			for (i = 0; i < nr; i++)
+				flush_dcache_page(newpage + i);
+		}
 	}
 out:
 	return rc;
-- 
2.11.0

[PATCH v5 3/7] mm: hugetlb: fix missing cache flush in copy_huge_page_from_user()

[Muchun Song]

The userfaultfd calls copy_huge_page_from_user() which does not do
any cache flushing for the target page.  Then the target page will
be mapped to the user space with a different address (user address),
which might have an alias issue with the kernel address used to copy
the data from the user to.  Fix this issue by flushing dcache in
copy_huge_page_from_user().

Fixes: fa4d75c1de13 ("userfaultfd: hugetlbfs: add copy_huge_page_from_user for hugetlb userfaultfd support")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/memory.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memory.c b/mm/memory.c
index e8ce066be5f2..eb027da68aa7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5405,6 +5405,8 @@ long copy_huge_page_from_user(struct page *dst_page,
 		if (rc)
 			break;
 
+		flush_dcache_page(subpage);
+
 		cond_resched();
 	}
 	return ret_val;
-- 
2.11.0

[PATCH v5 4/7] mm: hugetlb: fix missing cache flush in hugetlb_mcopy_atomic_pte()

[Muchun Song]

folio_copy() will copy the data from one page to the target page, then
the target page will be mapped to the user space address, which might
have an alias issue with the kernel address used to copy the data from
the page to.  There are 2 ways to fix this issue.

 1) insert flush_dcache_page() after folio_copy().
 2) replace folio_copy() with copy_user_huge_page() which already
    considers the cache maintenance.

We chose 2) way to fix the issue since architectures can optimize this
situation.  It is also make backports easier.

Fixes: 8cc5fcbb5be8 ("mm, hugetlb: fix racy resv_huge_pages underflow on UFFDIO_COPY")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/hugetlb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index a1baa198519a..eba7681d15d0 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5818,7 +5818,8 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
 			*pagep = NULL;
 			goto out;
 		}
-		folio_copy(page_folio(page), page_folio(*pagep));
+		copy_user_huge_page(page, *pagep, dst_addr, dst_vma,
+				    pages_per_huge_page(h));
 		put_page(*pagep);
 		*pagep = NULL;
 	}
-- 
2.11.0

[PATCH v5 5/7] mm: shmem: fix missing cache flush in shmem_mfill_atomic_pte()

[Muchun Song]

The userfaultfd calls shmem_mfill_atomic_pte() which does not do any
cache flushing for the target page.  Then the target page will be mapped
to the user space with a different address (user address), which might
have an alias issue with the kernel address used to copy the data from the
user to.  Insert flush_dcache_page() in non-zero-page case.  And replace
clear_highpage() with clear_user_highpage() which already considers
the cache maintenance.

Fixes: 8d1039634206 ("userfaultfd: shmem: add shmem_mfill_zeropage_pte for userfaultfd support")
Fixes: 4c27fe4c4c84 ("userfaultfd: shmem: add shmem_mcopy_atomic_pte for userfaultfd support")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/shmem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index eb0fd9001130..2e17ec9231a2 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2371,8 +2371,10 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm,
 				/* don't free the page */
 				goto out_unacct_blocks;
 			}
+
+			flush_dcache_page(page);
 		} else {		/* ZEROPAGE */
-			clear_highpage(page);
+			clear_user_highpage(page, dst_addr);
 		}
 	} else {
 		page = *pagep;
-- 
2.11.0

Re: [PATCH v5 5/7] mm: shmem: fix missing cache flush in shmem_mfill_atomic_pte()

[Mike Kravetz]

On 2/10/22 04:30, Muchun Song wrote:
> The userfaultfd calls shmem_mfill_atomic_pte() which does not do any
> cache flushing for the target page.  Then the target page will be mapped
> to the user space with a different address (user address), which might
> have an alias issue with the kernel address used to copy the data from the
> user to.  Insert flush_dcache_page() in non-zero-page case.  And replace
> clear_highpage() with clear_user_highpage() which already considers
> the cache maintenance.
> 
> Fixes: 8d1039634206 ("userfaultfd: shmem: add shmem_mfill_zeropage_pte for userfaultfd support")
> Fixes: 4c27fe4c4c84 ("userfaultfd: shmem: add shmem_mcopy_atomic_pte for userfaultfd support")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/shmem.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks,

It might have been better to combine this and the next patch.  When looking
at this, I noted the 'fallback to copy_from_user outside mmap_lock' case needs
to be addressed as well.  It is in the next patch.  No need to change.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index eb0fd9001130..2e17ec9231a2 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2371,8 +2371,10 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm,
>  				/* don't free the page */
>  				goto out_unacct_blocks;
>  			}
> +
> +			flush_dcache_page(page);
>  		} else {		/* ZEROPAGE */
> -			clear_highpage(page);
> +			clear_user_highpage(page, dst_addr);
>  		}
>  	} else {
>  		page = *pagep;


-- 
Mike Kravetz

Re: [PATCH v5 5/7] mm: shmem: fix missing cache flush in shmem_mfill_atomic_pte()

[Muchun Song]

On Wed, Feb 16, 2022 at 3:12 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 2/10/22 04:30, Muchun Song wrote:
> > The userfaultfd calls shmem_mfill_atomic_pte() which does not do any
> > cache flushing for the target page.  Then the target page will be mapped
> > to the user space with a different address (user address), which might
> > have an alias issue with the kernel address used to copy the data from the
> > user to.  Insert flush_dcache_page() in non-zero-page case.  And replace
> > clear_highpage() with clear_user_highpage() which already considers
> > the cache maintenance.
> >
> > Fixes: 8d1039634206 ("userfaultfd: shmem: add shmem_mfill_zeropage_pte for userfaultfd support")
> > Fixes: 4c27fe4c4c84 ("userfaultfd: shmem: add shmem_mcopy_atomic_pte for userfaultfd support")
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/shmem.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
>
> Thanks,
>
> It might have been better to combine this and the next patch.  When looking
> at this, I noted the 'fallback to copy_from_user outside mmap_lock' case needs
> to be addressed as well.  It is in the next patch.  No need to change.

I separate those changes into 2 patches since the fixed patch
is different. This patch is fixing linux 4.13 and later, while next
patch is fixing linux 4.2 and later. Maybe it is hard to backport
if combining those two patches.

>
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Thanks Mike.

[PATCH v5 6/7] mm: userfaultfd: fix missing cache flush in mcopy_atomic_pte() and __mcopy_atomic()

[Muchun Song]

The userfaultfd calls mcopy_atomic_pte() and __mcopy_atomic() which do not
do any cache flushing for the target page.  Then the target page will be
mapped to the user space with a different address (user address), which might
have an alias issue with the kernel address used to copy the data from the
user to.  Fix this by insert flush_dcache_page() after copy_from_user()
succeeds.

Fixes: b6ebaedb4cb1 ("userfaultfd: avoid mmap_sem read recursion in mcopy_atomic")
Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/userfaultfd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 0780c2a57ff1..6ccc534d1c1c 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -150,6 +150,8 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
 			/* don't free the page */
 			goto out;
 		}
+
+		flush_dcache_page(page);
 	} else {
 		page = *pagep;
 		*pagep = NULL;
@@ -625,6 +627,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
 				err = -EFAULT;
 				goto out;
 			}
+			flush_dcache_page(page);
 			goto retry;
 		} else
 			BUG_ON(page);
-- 
2.11.0

Re: [PATCH v5 6/7] mm: userfaultfd: fix missing cache flush in mcopy_atomic_pte() and __mcopy_atomic()

[Mike Kravetz]

On 2/10/22 04:30, Muchun Song wrote:
> The userfaultfd calls mcopy_atomic_pte() and __mcopy_atomic() which do not
> do any cache flushing for the target page.  Then the target page will be
> mapped to the user space with a different address (user address), which might
> have an alias issue with the kernel address used to copy the data from the
> user to.  Fix this by insert flush_dcache_page() after copy_from_user()
> succeeds.
> 
> Fixes: b6ebaedb4cb1 ("userfaultfd: avoid mmap_sem read recursion in mcopy_atomic")
> Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/userfaultfd.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

> 
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 0780c2a57ff1..6ccc534d1c1c 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -150,6 +150,8 @@ static int mcopy_atomic_pte(struct mm_struct *dst_mm,
>  			/* don't free the page */
>  			goto out;
>  		}
> +
> +		flush_dcache_page(page);
>  	} else {
>  		page = *pagep;
>  		*pagep = NULL;
> @@ -625,6 +627,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
>  				err = -EFAULT;
>  				goto out;
>  			}
> +			flush_dcache_page(page);
>  			goto retry;
>  		} else
>  			BUG_ON(page);

[PATCH v5 7/7] mm: replace multiple dcache flush with flush_dcache_folio()

[Muchun Song]

Simplify the code by using flush_dcache_folio().

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/migrate.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c418e8d92b9c..daf2b3508670 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -933,12 +933,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 		if (!PageMappingFlags(page))
 			page->mapping = NULL;
 
-		if (likely(!is_zone_device_page(newpage))) {
-			int i, nr = compound_nr(newpage);
-
-			for (i = 0; i < nr; i++)
-				flush_dcache_page(newpage + i);
-		}
+		if (likely(!is_zone_device_page(newpage)))
+			flush_dcache_folio(page_folio(newpage));
 	}
 out:
 	return rc;
-- 
2.11.0