The following crash happens for me when running the -mm selftests
(below). Specifically, it happens while running the uffd-stress
subtests:
kernel BUG at mm/hugetlb.c:7249!
invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
CPU: 0 PID: 3238 Comm: uffd-stress Not tainted 6.4.0-hubbard-github+ #109
Hardware name: ASUS X299-A/PRIME X299-A, BIOS 1503 08/03/2018
RIP: 0010:huge_pte_alloc+0x12c/0x1a0
...
Call Trace:
<TASK>
? __die_body+0x63/0xb0
? die+0x9f/0xc0
? do_trap+0xab/0x180
? huge_pte_alloc+0x12c/0x1a0
? do_error_trap+0xc6/0x110
? huge_pte_alloc+0x12c/0x1a0
? handle_invalid_op+0x2c/0x40
? huge_pte_alloc+0x12c/0x1a0
? exc_invalid_op+0x33/0x50
? asm_exc_invalid_op+0x16/0x20
? __pfx_put_prev_task_idle+0x10/0x10
? huge_pte_alloc+0x12c/0x1a0
hugetlb_fault+0x1a3/0x1120
? finish_task_switch+0xb3/0x2a0
? lock_is_held_type+0xdb/0x150
handle_mm_fault+0xb8a/0xd40
? find_vma+0x5d/0xa0
do_user_addr_fault+0x257/0x5d0
exc_page_fault+0x7b/0x1f0
asm_exc_page_fault+0x22/0x30
That happens because a BUG() statement in huge_pte_alloc() attempts to
check that a pte, if present, is a hugetlb pte, but it does so in a
non-lockless-safe manner that leads to a false BUG() report.
We got here due to a couple of bugs, each of which by itself was not
quite enough to cause a problem:
First of all, before commit c33c794828f2("mm: ptep_get() conversion"),
the BUG() statement in huge_pte_alloc() was itself fragile: it relied
upon compiler behavior to only read the pte once, despite using it twice
in the same conditional.
Next, commit c33c794828f2 ("mm: ptep_get() conversion") broke that
delicate situation, by causing all direct pte reads to be done via
READ_ONCE(). And so READ_ONCE() got called twice within the same BUG()
conditional, leading to comparing (potentially, occasionally) different
versions of the pte, and thus to false BUG() reports.
Fix this by taking a single snapshot of the pte before using it in the
BUG conditional.
Now, that commit is only partially to blame here but, people doing
bisections will invariably land there, so this will help them find a fix
for a real crash. And also, the previous behavior was unlikely to ever
expose this bug--it was fragile, yet not actually broken.
So that's why I chose this commit for the Fixes tag, rather than the
commit that created the original BUG() statement.
Fixes: c33c794828f2 ("mm: ptep_get() conversion")
Cc: Adrian Hunter <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: Alexander Potapenko <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Dave Airlie <[email protected]>
Cc: Dimitri Sivanich <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Ian Rogers <[email protected]>
Cc: Jason Gunthorpe <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Kirill A. Shutemov <[email protected]>
Cc: Lorenzo Stoakes <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: Miaohe Lin <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Mike Kravetz <[email protected]>
Cc: Mike Rapoport (IBM) <[email protected]>
Cc: Muchun Song <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Naoya Horiguchi <[email protected]>
Cc: Oleksandr Tyshchenko <[email protected]>
Cc: Pavel Tatashin <[email protected]>
Cc: Roman Gushchin <[email protected]>
Cc: Ryan Roberts <[email protected]>
Cc: SeongJae Park <[email protected]>
Cc: Shakeel Butt <[email protected]>
Cc: Uladzislau Rezki (Sony) <[email protected]>
Cc: Vincenzo Frascino <[email protected]>
Cc: Yu Zhao <[email protected]>
Signed-off-by: John Hubbard <[email protected]>
---
mm/hugetlb.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index bce28cca73a1..73fbeb8f979f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -7246,7 +7246,12 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
pte = (pte_t *)pmd_alloc(mm, pud, addr);
}
}
- BUG_ON(pte && pte_present(ptep_get(pte)) && !pte_huge(ptep_get(pte)));
+
+ if (pte) {
+ pte_t pteval = ptep_get(pte);
+
+ BUG_ON(pte_present(pteval) && !pte_huge(pteval));
+ }
return pte;
}
base-commit: bf1fa6f15553df04f2bdd06190ccd5f388ab0777
--
2.41.0
> On Jun 30, 2023, at 09:32, John Hubbard <[email protected]> wrote:
>
> The following crash happens for me when running the -mm selftests
> (below). Specifically, it happens while running the uffd-stress
> subtests:
>
> kernel BUG at mm/hugetlb.c:7249!
> invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 0 PID: 3238 Comm: uffd-stress Not tainted 6.4.0-hubbard-github+ #109
> Hardware name: ASUS X299-A/PRIME X299-A, BIOS 1503 08/03/2018
> RIP: 0010:huge_pte_alloc+0x12c/0x1a0
> ...
> Call Trace:
> <TASK>
> ? __die_body+0x63/0xb0
> ? die+0x9f/0xc0
> ? do_trap+0xab/0x180
> ? huge_pte_alloc+0x12c/0x1a0
> ? do_error_trap+0xc6/0x110
> ? huge_pte_alloc+0x12c/0x1a0
> ? handle_invalid_op+0x2c/0x40
> ? huge_pte_alloc+0x12c/0x1a0
> ? exc_invalid_op+0x33/0x50
> ? asm_exc_invalid_op+0x16/0x20
> ? __pfx_put_prev_task_idle+0x10/0x10
> ? huge_pte_alloc+0x12c/0x1a0
> hugetlb_fault+0x1a3/0x1120
> ? finish_task_switch+0xb3/0x2a0
> ? lock_is_held_type+0xdb/0x150
> handle_mm_fault+0xb8a/0xd40
> ? find_vma+0x5d/0xa0
> do_user_addr_fault+0x257/0x5d0
> exc_page_fault+0x7b/0x1f0
> asm_exc_page_fault+0x22/0x30
>
> That happens because a BUG() statement in huge_pte_alloc() attempts to
> check that a pte, if present, is a hugetlb pte, but it does so in a
> non-lockless-safe manner that leads to a false BUG() report.
>
> We got here due to a couple of bugs, each of which by itself was not
> quite enough to cause a problem:
>
> First of all, before commit c33c794828f2("mm: ptep_get() conversion"),
> the BUG() statement in huge_pte_alloc() was itself fragile: it relied
> upon compiler behavior to only read the pte once, despite using it twice
> in the same conditional.
>
> Next, commit c33c794828f2 ("mm: ptep_get() conversion") broke that
> delicate situation, by causing all direct pte reads to be done via
> READ_ONCE(). And so READ_ONCE() got called twice within the same BUG()
> conditional, leading to comparing (potentially, occasionally) different
> versions of the pte, and thus to false BUG() reports.
>
> Fix this by taking a single snapshot of the pte before using it in the
> BUG conditional.
>
> Now, that commit is only partially to blame here but, people doing
> bisections will invariably land there, so this will help them find a fix
> for a real crash. And also, the previous behavior was unlikely to ever
> expose this bug--it was fragile, yet not actually broken.
>
> So that's why I chose this commit for the Fixes tag, rather than the
> commit that created the original BUG() statement.
>
> Fixes: c33c794828f2 ("mm: ptep_get() conversion")
> Cc: Adrian Hunter <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andrey Konovalov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Dimitri Sivanich <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Ian Rogers <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Kirill A. Shutemov <[email protected]>
> Cc: Lorenzo Stoakes <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Miaohe Lin <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Mike Kravetz <[email protected]>
> Cc: Mike Rapoport (IBM) <[email protected]>
> Cc: Muchun Song <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Naoya Horiguchi <[email protected]>
> Cc: Oleksandr Tyshchenko <[email protected]>
> Cc: Pavel Tatashin <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: Ryan Roberts <[email protected]>
> Cc: SeongJae Park <[email protected]>
> Cc: Shakeel Butt <[email protected]>
> Cc: Uladzislau Rezki (Sony) <[email protected]>
> Cc: Vincenzo Frascino <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
Acked-by: Muchun Song <[email protected]>
Thanks.
On 30/06/2023 02:32, John Hubbard wrote:
> The following crash happens for me when running the -mm selftests
> (below). Specifically, it happens while running the uffd-stress
> subtests:
>
> kernel BUG at mm/hugetlb.c:7249!
> invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 0 PID: 3238 Comm: uffd-stress Not tainted 6.4.0-hubbard-github+ #109
> Hardware name: ASUS X299-A/PRIME X299-A, BIOS 1503 08/03/2018
> RIP: 0010:huge_pte_alloc+0x12c/0x1a0
> ...
> Call Trace:
> <TASK>
> ? __die_body+0x63/0xb0
> ? die+0x9f/0xc0
> ? do_trap+0xab/0x180
> ? huge_pte_alloc+0x12c/0x1a0
> ? do_error_trap+0xc6/0x110
> ? huge_pte_alloc+0x12c/0x1a0
> ? handle_invalid_op+0x2c/0x40
> ? huge_pte_alloc+0x12c/0x1a0
> ? exc_invalid_op+0x33/0x50
> ? asm_exc_invalid_op+0x16/0x20
> ? __pfx_put_prev_task_idle+0x10/0x10
> ? huge_pte_alloc+0x12c/0x1a0
> hugetlb_fault+0x1a3/0x1120
> ? finish_task_switch+0xb3/0x2a0
> ? lock_is_held_type+0xdb/0x150
> handle_mm_fault+0xb8a/0xd40
> ? find_vma+0x5d/0xa0
> do_user_addr_fault+0x257/0x5d0
> exc_page_fault+0x7b/0x1f0
> asm_exc_page_fault+0x22/0x30
>
> That happens because a BUG() statement in huge_pte_alloc() attempts to
> check that a pte, if present, is a hugetlb pte, but it does so in a
> non-lockless-safe manner that leads to a false BUG() report.
>
> We got here due to a couple of bugs, each of which by itself was not
> quite enough to cause a problem:
>
> First of all, before commit c33c794828f2("mm: ptep_get() conversion"),
> the BUG() statement in huge_pte_alloc() was itself fragile: it relied
> upon compiler behavior to only read the pte once, despite using it twice
> in the same conditional.
>
> Next, commit c33c794828f2 ("mm: ptep_get() conversion") broke that
> delicate situation, by causing all direct pte reads to be done via
> READ_ONCE(). And so READ_ONCE() got called twice within the same BUG()
> conditional, leading to comparing (potentially, occasionally) different
> versions of the pte, and thus to false BUG() reports.
Thanks for finding and fixing this - sorry for the issue. FWIW, I've re-reviewed
the whole ptep_get conversion patch looking for other instances of this pattern
- I didn't spot any other issues.
>
> Fix this by taking a single snapshot of the pte before using it in the
> BUG conditional.
>
> Now, that commit is only partially to blame here but, people doing
> bisections will invariably land there, so this will help them find a fix
> for a real crash. And also, the previous behavior was unlikely to ever
> expose this bug--it was fragile, yet not actually broken.
>
> So that's why I chose this commit for the Fixes tag, rather than the
> commit that created the original BUG() statement.
>
> Fixes: c33c794828f2 ("mm: ptep_get() conversion")
> Cc: Adrian Hunter <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andrey Konovalov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Dimitri Sivanich <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Ian Rogers <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Kirill A. Shutemov <[email protected]>
> Cc: Lorenzo Stoakes <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Miaohe Lin <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Mike Kravetz <[email protected]>
> Cc: Mike Rapoport (IBM) <[email protected]>
> Cc: Muchun Song <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Naoya Horiguchi <[email protected]>
> Cc: Oleksandr Tyshchenko <[email protected]>
> Cc: Pavel Tatashin <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: Ryan Roberts <[email protected]>
> Cc: SeongJae Park <[email protected]>
> Cc: Shakeel Butt <[email protected]>
> Cc: Uladzislau Rezki (Sony) <[email protected]>
> Cc: Vincenzo Frascino <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> mm/hugetlb.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index bce28cca73a1..73fbeb8f979f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -7246,7 +7246,12 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
> pte = (pte_t *)pmd_alloc(mm, pud, addr);
> }
> }
> - BUG_ON(pte && pte_present(ptep_get(pte)) && !pte_huge(ptep_get(pte)));
> +
> + if (pte) {
> + pte_t pteval = ptep_get(pte);
Given the PTL is not held here, I think this should technically be
ptep_get_lockless()?
Thanks,
Ryan
> +
> + BUG_ON(pte_present(pteval) && !pte_huge(pteval));
> + }
>
> return pte;
> }
>
> base-commit: bf1fa6f15553df04f2bdd06190ccd5f388ab0777
On 6/30/23 03:07, Ryan Roberts wrote:
...
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index bce28cca73a1..73fbeb8f979f 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -7246,7 +7246,12 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
>> pte = (pte_t *)pmd_alloc(mm, pud, addr);
>> }
>> }
>> - BUG_ON(pte && pte_present(ptep_get(pte)) && !pte_huge(ptep_get(pte)));
>> +
>> + if (pte) {
>> + pte_t pteval = ptep_get(pte);
>
> Given the PTL is not held here, I think this should technically be
> ptep_get_lockless()?
>
Oh yes. I'll change it to that and send a v2, thanks!
thanks,
--
John Hubbard
NVIDIA
On 06/29/23 18:32, John Hubbard wrote:
> The following crash happens for me when running the -mm selftests
> (below). Specifically, it happens while running the uffd-stress
> subtests:
>
> kernel BUG at mm/hugetlb.c:7249!
> invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> CPU: 0 PID: 3238 Comm: uffd-stress Not tainted 6.4.0-hubbard-github+ #109
> Hardware name: ASUS X299-A/PRIME X299-A, BIOS 1503 08/03/2018
> RIP: 0010:huge_pte_alloc+0x12c/0x1a0
> ...
> Call Trace:
> <TASK>
> ? __die_body+0x63/0xb0
> ? die+0x9f/0xc0
> ? do_trap+0xab/0x180
> ? huge_pte_alloc+0x12c/0x1a0
> ? do_error_trap+0xc6/0x110
> ? huge_pte_alloc+0x12c/0x1a0
> ? handle_invalid_op+0x2c/0x40
> ? huge_pte_alloc+0x12c/0x1a0
> ? exc_invalid_op+0x33/0x50
> ? asm_exc_invalid_op+0x16/0x20
> ? __pfx_put_prev_task_idle+0x10/0x10
> ? huge_pte_alloc+0x12c/0x1a0
> hugetlb_fault+0x1a3/0x1120
> ? finish_task_switch+0xb3/0x2a0
> ? lock_is_held_type+0xdb/0x150
> handle_mm_fault+0xb8a/0xd40
> ? find_vma+0x5d/0xa0
> do_user_addr_fault+0x257/0x5d0
> exc_page_fault+0x7b/0x1f0
> asm_exc_page_fault+0x22/0x30
>
> That happens because a BUG() statement in huge_pte_alloc() attempts to
> check that a pte, if present, is a hugetlb pte, but it does so in a
> non-lockless-safe manner that leads to a false BUG() report.
>
> We got here due to a couple of bugs, each of which by itself was not
> quite enough to cause a problem:
>
> First of all, before commit c33c794828f2("mm: ptep_get() conversion"),
> the BUG() statement in huge_pte_alloc() was itself fragile: it relied
> upon compiler behavior to only read the pte once, despite using it twice
> in the same conditional.
>
> Next, commit c33c794828f2 ("mm: ptep_get() conversion") broke that
> delicate situation, by causing all direct pte reads to be done via
> READ_ONCE(). And so READ_ONCE() got called twice within the same BUG()
> conditional, leading to comparing (potentially, occasionally) different
> versions of the pte, and thus to false BUG() reports.
>
> Fix this by taking a single snapshot of the pte before using it in the
> BUG conditional.
>
> Now, that commit is only partially to blame here but, people doing
> bisections will invariably land there, so this will help them find a fix
> for a real crash. And also, the previous behavior was unlikely to ever
> expose this bug--it was fragile, yet not actually broken.
>
> So that's why I chose this commit for the Fixes tag, rather than the
> commit that created the original BUG() statement.
>
> Fixes: c33c794828f2 ("mm: ptep_get() conversion")
> Cc: Adrian Hunter <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: Alexander Potapenko <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Andrey Konovalov <[email protected]>
> Cc: Andrey Ryabinin <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Dave Airlie <[email protected]>
> Cc: Dimitri Sivanich <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Ian Rogers <[email protected]>
> Cc: Jason Gunthorpe <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Kirill A. Shutemov <[email protected]>
> Cc: Lorenzo Stoakes <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Miaohe Lin <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Mike Kravetz <[email protected]>
> Cc: Mike Rapoport (IBM) <[email protected]>
> Cc: Muchun Song <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Naoya Horiguchi <[email protected]>
> Cc: Oleksandr Tyshchenko <[email protected]>
> Cc: Pavel Tatashin <[email protected]>
> Cc: Roman Gushchin <[email protected]>
> Cc: Ryan Roberts <[email protected]>
> Cc: SeongJae Park <[email protected]>
> Cc: Shakeel Butt <[email protected]>
> Cc: Uladzislau Rezki (Sony) <[email protected]>
> Cc: Vincenzo Frascino <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> mm/hugetlb.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Thanks for catching this and the detailed explanation.
Acked-by: Mike Kravetz <[email protected]>
--
Mike Kravetz
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index bce28cca73a1..73fbeb8f979f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -7246,7 +7246,12 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
> pte = (pte_t *)pmd_alloc(mm, pud, addr);
> }
> }
> - BUG_ON(pte && pte_present(ptep_get(pte)) && !pte_huge(ptep_get(pte)));
> +
> + if (pte) {
> + pte_t pteval = ptep_get(pte);
> +
> + BUG_ON(pte_present(pteval) && !pte_huge(pteval));
> + }
>
> return pte;
> }
>
> base-commit: bf1fa6f15553df04f2bdd06190ccd5f388ab0777
> --
> 2.41.0
>