[PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()

[PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()

[Christophe Leroy]

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

Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()

[Michael Ellerman]

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

Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()

[Christophe Leroy]

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.

Re: [PATCH] powerpc/64e: Fix wrong test in __ptep_test_and_clear_young()

[Christophe Leroy]

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