* [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()
@ 2023-09-02 8:36 Christophe Leroy
2023-09-05 2:36 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Christophe Leroy @ 2023-09-02 8:36 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin; +Cc: linuxppc-dev, linux-kernel
Commit 45201c879469 ("powerpc/nohash: Remove hash related code from
nohash headers.") replaced:
if ((pte_val(*ptep) & (_PAGE_ACCESSED | _PAGE_HASHPTE)) == 0)
return 0;
By:
if (pte_young(*ptep))
return 0;
But it should be:
if (!pte_young(*ptep))
return 0;
Fix it.
Fixes: 45201c879469 ("powerpc/nohash: Remove hash related code from nohash headers.")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/nohash/64/pgtable.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index 4ff6aa66b29a..6114a17ed234 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -154,7 +154,7 @@ static inline int __ptep_test_and_clear_young(struct mm_struct *mm,
{
unsigned long old;
- if (pte_young(*ptep))
+ if (!pte_young(*ptep))
return 0;
old = pte_update(mm, addr, ptep, _PAGE_ACCESSED, 0, 0);
return (old & _PAGE_ACCESSED) != 0;
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()
2023-09-02 8:36 [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young() Christophe Leroy
@ 2023-09-05 2:36 ` Michael Ellerman
2023-09-05 4:46 ` Christophe Leroy
0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2023-09-05 2:36 UTC (permalink / raw)
To: Christophe Leroy, Nicholas Piggin; +Cc: linuxppc-dev, linux-kernel
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Commit 45201c879469 ("powerpc/nohash: Remove hash related code from
> nohash headers.") replaced:
>
> if ((pte_val(*ptep) & (_PAGE_ACCESSED | _PAGE_HASHPTE)) == 0)
> return 0;
>
> By:
>
> if (pte_young(*ptep))
> return 0;
>
> But it should be:
>
> if (!pte_young(*ptep))
> return 0;
That seems bad :)
But I don't know off the top of my head where
__ptep_test_and_clear_young() is used, and so what the symptoms could
be. Presumably nothing too bad or someone would have noticed?
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()
2023-09-05 2:36 ` Michael Ellerman
@ 2023-09-05 4:46 ` Christophe Leroy
2023-09-05 7:03 ` Christophe Leroy
0 siblings, 1 reply; 4+ messages in thread
From: Christophe Leroy @ 2023-09-05 4:46 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Le 05/09/2023 à 04:36, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Commit 45201c879469 ("powerpc/nohash: Remove hash related code from
>> nohash headers.") replaced:
>>
>> if ((pte_val(*ptep) & (_PAGE_ACCESSED | _PAGE_HASHPTE)) == 0)
>> return 0;
>>
>> By:
>>
>> if (pte_young(*ptep))
>> return 0;
>>
>> But it should be:
>>
>> if (!pte_young(*ptep))
>> return 0;
>
>
> That seems bad :)
>
> But I don't know off the top of my head where
> __ptep_test_and_clear_young() is used, and so what the symptoms could
> be. Presumably nothing too bad or someone would have noticed?
>
The two uses in mm/vmscan.c are as follows:
if (!ptep_test_and_clear_young(args->vma, addr, pte + i))
VM_WARN_ON_ONCE(true);
So it seems to be expected to never happen.
The only useful place it is used seems to be folio_check_references()
which is part of the reclaim process. So probably it messes up swap, but
to what extent ? ppc64e is for embedded systems. Do embedded systems
have swap at all ?
Also surprising that it is also called from mm/debug_vm_pgtable.c so
shouldn't we have noticed earlier ? I'll check if it works.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()
2023-09-05 4:46 ` Christophe Leroy
@ 2023-09-05 7:03 ` Christophe Leroy
0 siblings, 0 replies; 4+ messages in thread
From: Christophe Leroy @ 2023-09-05 7:03 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Le 05/09/2023 à 06:46, Christophe Leroy a écrit :
>
>
> Le 05/09/2023 à 04:36, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>> Commit 45201c879469 ("powerpc/nohash: Remove hash related code from
>>> nohash headers.") replaced:
>>>
>>> if ((pte_val(*ptep) & (_PAGE_ACCESSED | _PAGE_HASHPTE)) == 0)
>>> return 0;
>>>
>>> By:
>>>
>>> if (pte_young(*ptep))
>>> return 0;
>>>
>>> But it should be:
>>>
>>> if (!pte_young(*ptep))
>>> return 0;
>>
>>
>> That seems bad :)
>>
>> But I don't know off the top of my head where
>> __ptep_test_and_clear_young() is used, and so what the symptoms could
>> be. Presumably nothing too bad or someone would have noticed?
>>
>
> The two uses in mm/vmscan.c are as follows:
>
> if (!ptep_test_and_clear_young(args->vma, addr, pte + i))
> VM_WARN_ON_ONCE(true);
>
> So it seems to be expected to never happen.
>
> The only useful place it is used seems to be folio_check_references()
> which is part of the reclaim process. So probably it messes up swap, but
> to what extent ? ppc64e is for embedded systems. Do embedded systems
> have swap at all ?
>
> Also surprising that it is also called from mm/debug_vm_pgtable.c so
> shouldn't we have noticed earlier ? I'll check if it works.
I confirm CONFIG_DEBUG_VM_PGTABLE on QEMU detects the problem:
debug_vm_pgtable: [debug_vm_pgtable ]: Validating architecture
page table helpers
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at mm/debug_vm_pgtable.c:174
debug_vm_pgtable+0x82c/0xa78
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.5.0-11584-g0ad6bbfc7dab #457
Hardware name: QEMU ppce500 e6500 0x80400020 QEMU e500
NIP: c0000000010fbe20 LR: c0000000010fbdf8 CTR: 0000000000000000
REGS: c0000000030cb860 TRAP: 0700 Not tainted (6.5.0-11584-g0ad6bbfc7dab)
MSR: 0000000080029002 <CE,EE,ME> CR: 44000484 XER: 00000000
IRQMASK: 0
GPR00: c0000000010fbde0 c0000000030cbb00 c0000000010e1200 c00000003f2a9eb8
GPR04: 000033cbee9df000 c0000000039b0ef8 00000039b124028d 0000000000000001
GPR08: 0000000000000000 0000000000000001 c0000000039b0ef8 0000000000000001
GPR12: 0000000024000248 c00000000132e000 c000000000001e38 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: c0000000010da0c4 c00000000112d068 c000000003015920 0000000000200000
GPR28: 0000000010000000 0000000000010000 c00000003f2a9eb8 00000039b124028d
NIP [c0000000010fbe20] debug_vm_pgtable+0x82c/0xa78
LR [c0000000010fbdf8] debug_vm_pgtable+0x804/0xa78
Call Trace:
[c0000000030cbb00] [c0000000010fbde0] debug_vm_pgtable+0x7ec/0xa78
(unreliable)
[c0000000030cbc70] [c000000000001a38] do_one_initcall+0x68/0x2b8
[c0000000030cbd40] [c0000000010db498] kernel_init_freeable+0x2d0/0x348
[c0000000030cbde0] [c000000000001e64] kernel_init+0x34/0x170
[c0000000030cbe50] [c000000000000594] ret_from_kernel_user_thread+0x14/0x1c
--- interrupt: 0 at 0x0
NIP: 0000000000000000 LR: 0000000000000000 CTR: 0000000000000000
REGS: c0000000030cbe80 TRAP: 0000 Not tainted (6.5.0-11584-g0ad6bbfc7dab)
MSR: 0000000000000000 <> CR: 00000000 XER: 00000000
IRQMASK: 0
GPR00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR04: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR12: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR24: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR28: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
NIP [0000000000000000] 0x0
LR [0000000000000000] 0x0
--- interrupt: 0
Code: 7c7e189e 4b16a3e9 e9410090 e92a0000 792b77e3 40c20010 79296842
79299800 f92a0000 e9410090 e92a0000 792977e2 <0b090000> 39200000
f92a0000 e9210090
---[ end trace 0000000000000000 ]---
Christophe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-05 7:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-02 8:36 [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young() Christophe Leroy
2023-09-05 2:36 ` Michael Ellerman
2023-09-05 4:46 ` Christophe Leroy
2023-09-05 7:03 ` Christophe Leroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).