On 2025-05-06 11:31 AM, Alexandre Ghiti wrote:
On 05/05/2025 21:27, Alexandre Ghiti wrote:
On 05/05/2025 18:07, Nam Cao wrote:
Hi Alex,
On Mon, May 05, 2025 at 06:02:26PM +0200, Alexandre Ghiti wrote:
On 04/05/2025 12:19, Nam Cao wrote:
When userspace does PR_SET_TAGGED_ADDR_CTRL, but Supm extension is not available, the kernel crashes:
Oops - illegal instruction [#1] [snip] epc : set_tagged_addr_ctrl+0x112/0x15a ra : set_tagged_addr_ctrl+0x74/0x15a epc : ffffffff80011ace ra : ffffffff80011a30 sp : ffffffc60039be10 [snip] status: 0000000200000120 badaddr: 0000000010a79073 cause: 0000000000000002 set_tagged_addr_ctrl+0x112/0x15a __riscv_sys_prctl+0x352/0x73c do_trap_ecall_u+0x17c/0x20c andle_exception+0x150/0x15c
It seems like the csr write is triggering this illegal instruction, can you confirm it is?
Yes, it is the "csr_write(CSR_ENVCFG, envcfg);" in envcfg_update_bits().
If so, I can't find in the specification that an implementation should do that when writing envcfg and I can't reproduce it on qemu. Where did you see this oops?
I can't find it in the spec either. I think it is up to the implementation.
The reserved fields of senvcfg are WPRI and contrary to WLRL, it does not explicitly "permit" to raise an illegal instruction so I'd say it is not up to the implementation, I'll ask around.
So I had confirmation that WPRI should not raise an illegal instruction so that's an issue with the platform. Your patch is not wrong but I'd rather have an explicit errata, what do you think?
There is no erratum here. Allwinner D1 / T-HEAD C906 implements Ss1p11, which was before senvcfg was added to the privileged architecture, and does not implement any of the extensions which would imply senvcfg's existence, so the CSR is reserved. We could check for Xlinuxenvcfg to determine if the CSR access will raise an exception, but that does not gain anything over checking for Supm specifically. So the fix is correct.
Reviewed-by: Samuel Holland samuel.holland@sifive.com
That said, I wonder if set_tagged_addr_ctrl(task, 0) should succeed when Supm is not implemented, matching get_tagged_addr_ctrl(). Without Supm, we know that have_user_pmlen_7 and have_user_pmlen_16 will both be false, so pmlen == 0 is the only case where we would call envcfg_update_bits(). And we know it would be a no-op. So an alternative fix would be to return 0 below the pmlen checks:
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 7c244de77180..536da9aa690e 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -309,6 +309,9 @@ long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg) if (!(arg & PR_TAGGED_ADDR_ENABLE)) pmlen = PMLEN_0;
+ if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) + return 0; + if (mmap_write_lock_killable(mm)) return -EINTR;
But I don't know if this better matches what userspace would expect.
Regards, Samuel
I got this crash on the MangoPI board: https://mangopi.org/mqpro
Best regards, Nam