The pmd_trans_huge() code in mfill_atomic() is wrong in two different ways depending on kernel version:
1. The pmd_trans_huge() check is racy and can lead to a BUG_ON() (if you hit the right two race windows) - I've tested this in a kernel build with some extra mdelay() calls. See the commit message for a description of the race scenario. On older kernels (before 6.5), I think the same bug can even theoretically lead to accessing transhuge page contents as a page table if you hit the right 5 narrow race windows (I haven't tested this case). 2. On newer kernels (>=6.5), for shmem mappings, khugepaged is allowed to yank page tables out from under us (though I haven't tested that), so I think the BUG_ON() checks in mfill_atomic() are just wrong.
I decided to write two separate fixes for these, so that the first fix can be backported to kernels affected by the first bug.
Signed-off-by: Jann Horn jannh@google.com --- Jann Horn (2): userfaultfd: Fix pmd_trans_huge() recheck race userfaultfd: Don't BUG_ON() if khugepaged yanks our page table
mm/userfaultfd.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) --- base-commit: d4560686726f7a357922f300fc81f5964be8df04 change-id: 20240812-uffd-thp-flip-fix-20f91f1151b9
The following race can occur:
mfill_atomic other thread ============ ============ <zap PMD> pmdp_get_lockless() [reads none pmd] <bail if trans_huge> <if none:> <pagefault creates transhuge zeropage> __pte_alloc [no-op] <zap PMD> <bail if pmd_trans_huge(*dst_pmd)> BUG_ON(pmd_none(*dst_pmd))
I have experimentally verified this in a kernel with extra mdelay() calls; the BUG_ON(pmd_none(*dst_pmd)) triggers.
On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow pte_offset_map[_lock]() to fail"), this can't lead to anything worse than a BUG_ON(), since the page table access helpers are actually designed to deal with page tables concurrently disappearing; but on older kernels (<=6.4), I think we could probably theoretically race past the two BUG_ON() checks and end up treating a hugepage as a page table.
Cc: stable@vger.kernel.org Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation") Signed-off-by: Jann Horn jannh@google.com --- mm/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index e54e5c8907fa..ec3750467aa5 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -801,7 +801,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx, break; } /* If an huge pmd materialized from under us fail */ - if (unlikely(pmd_trans_huge(*dst_pmd))) { + dst_pmdval = pmdp_get_lockless(dst_pmd); + if (unlikely(pmd_none(dst_pmdval) || pmd_trans_huge(dst_pmdval))) { err = -EFAULT; break; }
Hi Jann,
On 2024/8/13 00:42, Jann Horn wrote:
The following race can occur:
mfill_atomic other thread ============ ============ <zap PMD> pmdp_get_lockless() [reads none pmd]
<bail if trans_huge> <if none:> <pagefault creates transhuge zeropage> __pte_alloc [no-op] <zap PMD> <bail if pmd_trans_huge(*dst_pmd)> BUG_ON(pmd_none(*dst_pmd))
I have experimentally verified this in a kernel with extra mdelay() calls; the BUG_ON(pmd_none(*dst_pmd)) triggers.
On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow pte_offset_map[_lock]() to fail"), this can't lead to anything worse than a BUG_ON(), since the page table access helpers are actually designed to deal with page tables concurrently disappearing; but on older kernels (<=6.4), I think we could probably theoretically race past the two BUG_ON() checks and end up treating a hugepage as a page table.
Cc: stable@vger.kernel.org Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation") Signed-off-by: Jann Horn jannh@google.com
mm/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index e54e5c8907fa..ec3750467aa5 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -801,7 +801,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx, break; } /* If an huge pmd materialized from under us fail */
if (unlikely(pmd_trans_huge(*dst_pmd))) {
dst_pmdval = pmdp_get_lockless(dst_pmd);if (unlikely(pmd_none(dst_pmdval) || pmd_trans_huge(dst_pmdval))) {
Before commit 0d940a9b270b, should we also check for is_pmd_migration_entry(), pmd_devmap() and pmd_bad() here?
Thanks, Qi
err = -EFAULT; break; }
On Tue, Aug 13, 2024 at 8:19 AM Qi Zheng zhengqi.arch@bytedance.com wrote:
Hi Jann,
On 2024/8/13 00:42, Jann Horn wrote:
The following race can occur:
mfill_atomic other thread ============ ============ <zap PMD> pmdp_get_lockless() [reads none pmd]
<bail if trans_huge> <if none:> <pagefault creates transhuge zeropage> __pte_alloc [no-op] <zap PMD> <bail if pmd_trans_huge(*dst_pmd)> BUG_ON(pmd_none(*dst_pmd))
I have experimentally verified this in a kernel with extra mdelay() calls; the BUG_ON(pmd_none(*dst_pmd)) triggers.
On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow pte_offset_map[_lock]() to fail"), this can't lead to anything worse than a BUG_ON(), since the page table access helpers are actually designed to deal with page tables concurrently disappearing; but on older kernels (<=6.4), I think we could probably theoretically race past the two BUG_ON() checks and end up treating a hugepage as a page table.
Cc: stable@vger.kernel.org Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation") Signed-off-by: Jann Horn jannh@google.com
mm/userfaultfd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index e54e5c8907fa..ec3750467aa5 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -801,7 +801,8 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx, break; } /* If an huge pmd materialized from under us fail */
if (unlikely(pmd_trans_huge(*dst_pmd))) {
dst_pmdval = pmdp_get_lockless(dst_pmd);if (unlikely(pmd_none(dst_pmdval) || pmd_trans_huge(dst_pmdval))) {Before commit 0d940a9b270b, should we also check for is_pmd_migration_entry(), pmd_devmap() and pmd_bad() here?
Oooh. I think you're right that this check is insufficient, thanks for spotting that.
I think I should probably change the check to something like this?
if (unlikely(!pmd_present(dst_pmdval) || pmd_trans_huge(dst_pmdval) || pmd_devmap(dst_pmdval) || pmd_bad(dst_pmdval))) {
!pmd_present() implies !is_pmd_migration_entry(). And the pmd_bad() at the end shouldn't be necessary if everything is working right, I'm just tacking it on to be safe.
I'll send a v2 with this change soon.
(Alternatively, pmd_leaf() might be useful here, but then we'd have to figure an alternate way of doing this for the backport.)
On 12.08.24 18:42, Jann Horn wrote:
The following race can occur:
mfill_atomic other thread ============ ============ <zap PMD> pmdp_get_lockless() [reads none pmd]
<bail if trans_huge> <if none:> <pagefault creates transhuge zeropage> __pte_alloc [no-op] <zap PMD> <bail if pmd_trans_huge(*dst_pmd)> BUG_ON(pmd_none(*dst_pmd))
I have experimentally verified this in a kernel with extra mdelay() calls; the BUG_ON(pmd_none(*dst_pmd)) triggers.
On kernels newer than commit 0d940a9b270b ("mm/pgtable: allow pte_offset_map[_lock]() to fail"), this can't lead to anything worse than a BUG_ON(), since the page table access helpers are actually designed to deal with page tables concurrently disappearing; but on older kernels (<=6.4), I think we could probably theoretically race past the two BUG_ON() checks and end up treating a hugepage as a page table.
Cc: stable@vger.kernel.org Fixes: c1a4de99fada ("userfaultfd: mcopy_atomic|mfill_zeropage: UFFDIO_COPY|UFFDIO_ZEROPAGE preparation") Signed-off-by: Jann Horn jannh@google.com
Acked-by: David Hildenbrand david@redhat.com
Since khugepaged was changed to allow retracting page tables in file mappings without holding the mmap lock, these BUG_ON()s are wrong - get rid of them.
We could also remove the preceding "if (unlikely(...))" block, but then we could reach pte_offset_map_lock() with transhuge pages not just for file mappings but also for anonymous mappings - which would probably be fine but I think is not necessarily expected.
Cc: stable@vger.kernel.org Fixes: 1d65b771bc08 ("mm/khugepaged: retract_page_tables() without mmap or vma lock") Signed-off-by: Jann Horn jannh@google.com --- mm/userfaultfd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index ec3750467aa5..0dfa97db6feb 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -806,9 +806,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx, err = -EFAULT; break; } - - BUG_ON(pmd_none(*dst_pmd)); - BUG_ON(pmd_trans_huge(*dst_pmd)); + /* + * For shmem mappings, khugepaged is allowed to remove page + * tables under us; pte_offset_map_lock() will deal with that. + */
err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr, src_addr, flags, &folio);
On 2024/8/13 00:42, Jann Horn wrote:
Since khugepaged was changed to allow retracting page tables in file mappings without holding the mmap lock, these BUG_ON()s are wrong - get rid of them.
We could also remove the preceding "if (unlikely(...))" block, but then we could reach pte_offset_map_lock() with transhuge pages not just for file mappings but also for anonymous mappings - which would probably be fine but I think is not necessarily expected.
Cc: stable@vger.kernel.org Fixes: 1d65b771bc08 ("mm/khugepaged: retract_page_tables() without mmap or vma lock") Signed-off-by: Jann Horn jannh@google.com
mm/userfaultfd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Reviewed-by: Qi Zheng zhengqi.arch@bytedance.com
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index ec3750467aa5..0dfa97db6feb 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -806,9 +806,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx, err = -EFAULT; break; }
BUG_ON(pmd_none(*dst_pmd));BUG_ON(pmd_trans_huge(*dst_pmd));
/** For shmem mappings, khugepaged is allowed to remove page* tables under us; pte_offset_map_lock() will deal with that.*/err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr, src_addr, flags, &folio);
On 12.08.24 18:42, Jann Horn wrote:
Since khugepaged was changed to allow retracting page tables in file mappings without holding the mmap lock, these BUG_ON()s are wrong - get rid of them.
We could also remove the preceding "if (unlikely(...))" block, but then we could reach pte_offset_map_lock() with transhuge pages not just for file mappings but also for anonymous mappings - which would probably be fine but I think is not necessarily expected.
Cc: stable@vger.kernel.org Fixes: 1d65b771bc08 ("mm/khugepaged: retract_page_tables() without mmap or vma lock") Signed-off-by: Jann Horn jannh@google.com
mm/userfaultfd.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index ec3750467aa5..0dfa97db6feb 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -806,9 +806,10 @@ static __always_inline ssize_t mfill_atomic(struct userfaultfd_ctx *ctx, err = -EFAULT; break; }
BUG_ON(pmd_none(*dst_pmd));BUG_ON(pmd_trans_huge(*dst_pmd));
/** For shmem mappings, khugepaged is allowed to remove page* tables under us; pte_offset_map_lock() will deal with that.*/err = mfill_atomic_pte(dst_pmd, dst_vma, dst_addr, src_addr, flags, &folio);
Acked-by: David Hildenbrand david@redhat.com
linux-stable-mirror@lists.linaro.org
-
David Hildenbrand -
Jann Horn -
Qi Zheng