On Thu, Apr 10, 2025 at 01:04:39PM +0200, Radim Krčmář wrote:
2025-03-14T14:39:24-07:00, Deepak Gupta debug@rivosinc.com:
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h @@ -62,6 +62,9 @@ struct thread_info { long user_sp; /* User stack pointer */ int cpu; unsigned long syscall_work; /* SYSCALL_WORK_ flags */ +#ifdef CONFIG_RISCV_USER_CFI
- struct cfi_status user_cfi_state;
+#endif
I don't think it makes sense to put all the data in thread_info. kernel_ssp and user_ssp is more than enough and the rest can comfortably live elsewhere in task_struct.
thread_info is supposed to be as small as possible -- just spanning multiple cache-lines could be noticeable.
I can change it to only include only `user_ssp`, base and size.
But before we go there, see below:
$ pahole -C thread_info kbuild/vmlinux struct thread_info { long unsigned int flags; /* 0 8 */ int preempt_count; /* 8 4 */
/* XXX 4 bytes hole, try to pack */
long int kernel_sp; /* 16 8 */ long int user_sp; /* 24 8 */ int cpu; /* 32 4 */
/* XXX 4 bytes hole, try to pack */
long unsigned int syscall_work; /* 40 8 */ struct cfi_status user_cfi_state; /* 48 32 */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ long unsigned int a0; /* 80 8 */ long unsigned int a1; /* 88 8 */ long unsigned int a2; /* 96 8 */
/* size: 104, cachelines: 2, members: 10 */ /* sum members: 96, holes: 2, sum holes: 8 */ /* last cacheline: 40 bytes */ };
If we were to remove entire `cfi_status`, it would still be 72 bytes (88 bytes if shadow call stack were enabled) and already spans across two cachelines. I did see the comment above that it should fit inside a cacheline. Although I assumed its stale comment given that it already spans across cacheline and I didn't see any special mention in commit messages of changes which grew this structure above one cacheline. So I assumed this was a stale comment.
On the other hand, whenever enable/lock bits are checked, there is a high likelyhood that user_ssp and other fields are going to be accessed and thus it actually might be helpful to have it all in one cacheline during runtime.
So I am not sure if its helpful sticking to the comment which already is stale.
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S @@ -147,6 +147,20 @@ SYM_CODE_START(handle_exception)
REG_L s0, TASK_TI_USER_SP(tp) csrrc s1, CSR_STATUS, t0
- /*
* If previous mode was U, capture shadow stack pointer and save it away* Zero CSR_SSP at the same time for sanitization.*/- ALTERNATIVE("nop; nop; nop; nop",
__stringify( \andi s2, s1, SR_SPP; \bnez s2, skip_ssp_save; \csrrw s2, CSR_SSP, x0; \REG_S s2, TASK_TI_USER_SSP(tp); \skip_ssp_save:),0,RISCV_ISA_EXT_ZICFISS,CONFIG_RISCV_USER_CFI)(I'd prefer this closer to the user_sp and kernel_sp swap, it's breaking the flow here. We also already know if we've returned from userspace or not even without SR_SPP, but reusing the information might tangle the logic.)
csrr s2, CSR_EPC csrr s3, CSR_TVAL csrr s4, CSR_CAUSE @@ -236,6 +250,18 @@ SYM_CODE_START_NOALIGN(ret_from_exception) csrw CSR_SCRATCH, tp
- /*
* Going back to U mode, restore shadow stack pointer*/Are we? I think we can be just as well returning back to kernel-space. Similar to how we can enter the exception handler from kernel-space.
- ALTERNATIVE("nop; nop",
__stringify( \REG_L s3, TASK_TI_USER_SSP(tp); \csrw CSR_SSP, s3),0,RISCV_ISA_EXT_ZICFISS,CONFIG_RISCV_USER_CFI)Thanks.