Playing with virtio-mem and background snapshots (using uffd-wp) on
hugetlb in QEMU, I managed to trigger a VM_BUG_ON(). Looking into the
details, hugetlb_change_protection() seems to not handle uffd-wp correctly
in all cases.
Patch #1 fixes my test case. I don't have reproducers for patch #2, as
it requires running into migration entries.
I did not yet check in detail yet if !hugetlb code requires similar care.
Cc: Andrew Morton <[email protected]>
Cc: Mike Kravetz <[email protected]>
Cc: Peter Xu <[email protected]>
Cc: Muchun Song <[email protected]>
Cc: Miaohe Lin <[email protected]>
David Hildenbrand (2):
mm/hugetlb: fix PTE marker handling in hugetlb_change_protection()
mm/hugetlb: fix uffd-wp handling for migration entries in
hugetlb_change_protection()
mm/hugetlb.c | 38 ++++++++++++++++----------------------
1 file changed, 16 insertions(+), 22 deletions(-)
--
2.38.1
On Thu, Dec 22, 2022 at 09:55:09PM +0100, David Hildenbrand wrote:
> Playing with virtio-mem and background snapshots (using uffd-wp) on
> hugetlb in QEMU, I managed to trigger a VM_BUG_ON(). Looking into the
> details, hugetlb_change_protection() seems to not handle uffd-wp correctly
> in all cases.
>
> Patch #1 fixes my test case. I don't have reproducers for patch #2, as
> it requires running into migration entries.
>
> I did not yet check in detail yet if !hugetlb code requires similar care.
>
> Cc: Andrew Morton <[email protected]>
> Cc: Mike Kravetz <[email protected]>
> Cc: Peter Xu <[email protected]>
> Cc: Muchun Song <[email protected]>
> Cc: Miaohe Lin <[email protected]>
>
> David Hildenbrand (2):
> mm/hugetlb: fix PTE marker handling in hugetlb_change_protection()
> mm/hugetlb: fix uffd-wp handling for migration entries in
> hugetlb_change_protection()
Reviewed-by: Peter Xu <[email protected]>
Thanks, David.
--
Peter Xu
We have to update the uffd-wp SWP PTE bit independent of the type of
migration entry. Currently, if we're unlucky and we want to install/clear
the uffd-wp bit just while we're migrating a read-only mapped hugetlb page,
we would miss to set/clear the uffd-wp bit.
Further, if we're processing a readable-exclusive
migration entry and neither want to set or clear the uffd-wp bit, we
could currently end up losing the uffd-wp bit. Note that the same would
hold for writable migrating entries, however, having a writable
migration entry with the uffd-wp bit set would already mean that
something went wrong.
Note that the change from !is_readable_migration_entry ->
writable_migration_entry is harmless and actually cleaner, as raised by
Miaohe Lin and discussed in [1].
[1] https://lkml.kernel.org/r/[email protected]
Fixes: 60dfaad65aa9 ("mm/hugetlb: allow uffd wr-protect none ptes")
Cc: <[email protected]>
Signed-off-by: David Hildenbrand <[email protected]>
---
mm/hugetlb.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 3a94f519304f..9552a6d1a281 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6516,10 +6516,9 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
} else if (unlikely(is_hugetlb_entry_migration(pte))) {
swp_entry_t entry = pte_to_swp_entry(pte);
struct page *page = pfn_swap_entry_to_page(entry);
+ pte_t newpte = pte;
- if (!is_readable_migration_entry(entry)) {
- pte_t newpte;
-
+ if (is_writable_migration_entry(entry)) {
if (PageAnon(page))
entry = make_readable_exclusive_migration_entry(
swp_offset(entry));
@@ -6527,13 +6526,15 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
entry = make_readable_migration_entry(
swp_offset(entry));
newpte = swp_entry_to_pte(entry);
- if (uffd_wp)
- newpte = pte_swp_mkuffd_wp(newpte);
- else if (uffd_wp_resolve)
- newpte = pte_swp_clear_uffd_wp(newpte);
- set_huge_pte_at(mm, address, ptep, newpte);
pages++;
}
+
+ if (uffd_wp)
+ newpte = pte_swp_mkuffd_wp(newpte);
+ else if (uffd_wp_resolve)
+ newpte = pte_swp_clear_uffd_wp(newpte);
+ if (!pte_same(pte, newpte))
+ set_huge_pte_at(mm, address, ptep, newpte);
} else if (unlikely(is_pte_marker(pte))) {
/* No other markers apply for now. */
WARN_ON_ONCE(!pte_marker_uffd_wp(pte));
--
2.38.1
On Thu, Dec 22, 2022 at 04:17:22PM -0500, Peter Xu wrote:
> On Thu, Dec 22, 2022 at 09:55:09PM +0100, David Hildenbrand wrote:
> > Playing with virtio-mem and background snapshots (using uffd-wp) on
> > hugetlb in QEMU, I managed to trigger a VM_BUG_ON(). Looking into the
> > details, hugetlb_change_protection() seems to not handle uffd-wp correctly
> > in all cases.
> >
> > Patch #1 fixes my test case. I don't have reproducers for patch #2, as
> > it requires running into migration entries.
> >
> > I did not yet check in detail yet if !hugetlb code requires similar care.
Ah I should have left a message here but I forgot - afaict shmem is all fine.
> >
> > Cc: Andrew Morton <[email protected]>
> > Cc: Mike Kravetz <[email protected]>
> > Cc: Peter Xu <[email protected]>
> > Cc: Muchun Song <[email protected]>
> > Cc: Miaohe Lin <[email protected]>
> >
> > David Hildenbrand (2):
> > mm/hugetlb: fix PTE marker handling in hugetlb_change_protection()
> > mm/hugetlb: fix uffd-wp handling for migration entries in
> > hugetlb_change_protection()
>
> Reviewed-by: Peter Xu <[email protected]>
>
> Thanks, David.
>
> --
> Peter Xu
--
Peter Xu
On 12/22/22 21:55, David Hildenbrand wrote:
> We have to update the uffd-wp SWP PTE bit independent of the type of
> migration entry. Currently, if we're unlucky and we want to install/clear
> the uffd-wp bit just while we're migrating a read-only mapped hugetlb page,
> we would miss to set/clear the uffd-wp bit.
>
> Further, if we're processing a readable-exclusive
> migration entry and neither want to set or clear the uffd-wp bit, we
> could currently end up losing the uffd-wp bit. Note that the same would
> hold for writable migrating entries, however, having a writable
> migration entry with the uffd-wp bit set would already mean that
> something went wrong.
>
> Note that the change from !is_readable_migration_entry ->
> writable_migration_entry is harmless and actually cleaner, as raised by
> Miaohe Lin and discussed in [1].
>
> [1] https://lkml.kernel.org/r/[email protected]
>
> Fixes: 60dfaad65aa9 ("mm/hugetlb: allow uffd wr-protect none ptes")
> Cc: <[email protected]>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> mm/hugetlb.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
Thanks,
Reviewed-by: Mike Kravetz <[email protected]>
--
Mike Kravetz