[External] [PATCH v5 1/5] riscv: Make get_insn public for instruction fault handling
yunhui cui
cuiyunhui at bytedance.com
Thu Oct 22 02:10:42 PDT 2026
Hi Yicong,
On Sat, Oct 17, 2026 at 4:31 PM Yicong Yang <yang.yicong at picoheart.com> wrote:
>
> Currently two places (vector and misaligned load/store) need
> to get fault instructions for handling and they implement the
> function separately. This is a common function so make
> get_insn() public in asm/insn.h for doing this job to make it
> less fragile and improve the maintainability.
>
> Also fix a bug that Bo Gan noticed: get_user() is insufficient
> in this case since it's possible for the instruction segment
> mapped as execute-only and get_user() doesn't have permissions
> to load it. The architecture provides sstatus.MXR for handling
> this so set MXR when reading userspace instruction.
>
> Cc: Bo Gan <ganboing at gmail.com>
> Signed-off-by: Yicong Yang <yang.yicong at picoheart.com>
> ---
> arch/riscv/include/asm/csr.h | 1 +
> arch/riscv/include/asm/insn.h | 62 ++++++++++++++++++++++++++++
> arch/riscv/include/asm/processor.h | 2 +-
> arch/riscv/kernel/asm-offsets.c | 6 +--
> arch/riscv/kernel/entry.S | 14 +++----
> arch/riscv/kernel/traps_misaligned.c | 52 -----------------------
> arch/riscv/kernel/vector.c | 6 +--
> 7 files changed, 77 insertions(+), 66 deletions(-)
>
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 621f84d1898f..e790cc2b8065 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -17,6 +17,7 @@
> #define SR_SPP _AC(0x00000100, UL) /* Previously Supervisor */
> #define SR_MPP _AC(0x00001800, UL) /* Previously Machine */
> #define SR_SUM _AC(0x00040000, UL) /* Supervisor User Memory Access */
> +#define SR_MXR _AC(0x00080000, UL) /* Make eXecutable Readable */
>
> /* zicfilp landing pad status bit */
> #define SR_SPELP _AC(0x00800000, UL)
> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index c3005573e8c9..88cea321ec3d 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
> @@ -600,4 +600,66 @@ static inline void riscv_insn_insert_utype_itype_imm(u32 *utype_insn, u32 *itype
> *utype_insn |= (imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1);
> *itype_insn |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF);
> }
> +
> +#define __read_insn(regs, insn, insn_addr, type) \
> +({ \
> + int __ret; \
> + \
> + if (user_mode(regs)) { \
> + const type __user *__p = (const type __user *)(insn_addr);\
> + might_fault(); \
> + if (access_ok(__p, sizeof(*__p))) { \
> + csr_set(CSR_STATUS, SR_MXR); \
> + __ret = __get_user((insn), __p); \
> + csr_clear(CSR_STATUS, SR_MXR); \
I took another look at patch 1/5.
For the __read_insn() part, I think keeping the v4 code would also be OK.
The issue I noticed is in the __switch_to() restore path. I sent a
separate fix for the existing SUM case here:
https://lore.kernel.org/all/20261022083212.84721-1-cuiyunhui@bytedance.com/
If this series keeps MXR as part of the saved task state, I think it
should be based on that fix and extend the mask from SR_SUM to
SR_SUM | SR_MXR in the csrrc path.
Thanks,
Yunhui
> + } else { \
> + (insn) = 0; \
> + __ret = -EFAULT; \
> + } \
> + } else { \
> + (insn) = *(type *)(insn_addr); \
> + __ret = 0; \
> + } \
> + \
> + __ret; \
> +})
> +
> +static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
> +{
> + ulong insn = 0;
> +
> + if (epc & 0x2) {
> + ulong tmp = 0;
> +
> + if (__read_insn(regs, insn, epc, u16))
> + return -EFAULT;
> + /* __get_user() uses regular "lw" which sign extend the loaded
> + * value make sure to clear higher order bits in case we "or" it
> + * below with the upper 16 bits half.
> + */
> + insn &= GENMASK(15, 0);
> + if ((insn & __INSN_LENGTH_MASK) != __INSN_LENGTH_32) {
> + *r_insn = insn;
> + return 0;
> + }
> + epc += sizeof(u16);
> + if (__read_insn(regs, tmp, epc, u16))
> + return -EFAULT;
> + *r_insn = (tmp << 16) | insn;
> +
> + return 0;
> + } else {
> + if (__read_insn(regs, insn, epc, u32))
> + return -EFAULT;
> + if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) {
> + *r_insn = insn;
> + return 0;
> + }
> + insn &= GENMASK(15, 0);
> + *r_insn = insn;
> +
> + return 0;
> + }
> +}
> +
> #endif /* _ASM_RISCV_INSN_H */
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index 815715c67f94..29af76aa486e 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -119,7 +119,7 @@ struct thread_struct {
> struct __riscv_d_ext_state fstate;
> unsigned long bad_cause;
> unsigned long envcfg;
> - unsigned long sum;
> + unsigned long sum_mxr;
> u32 riscv_v_flags;
> u32 vstate_ctrl;
> struct __riscv_v_ext_state vstate;
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index a75f0cfea1e9..b6de2179570b 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -35,7 +35,7 @@ void asm_offsets(void)
> OFFSET(TASK_THREAD_S9, task_struct, thread.s[9]);
> OFFSET(TASK_THREAD_S10, task_struct, thread.s[10]);
> OFFSET(TASK_THREAD_S11, task_struct, thread.s[11]);
> - OFFSET(TASK_THREAD_SUM, task_struct, thread.sum);
> + OFFSET(TASK_THREAD_SUM_MXR, task_struct, thread.sum_mxr);
>
> OFFSET(TASK_TI_CPU, task_struct, thread_info.cpu);
> OFFSET(TASK_TI_PREEMPT_COUNT, task_struct, thread_info.preempt_count);
> @@ -352,8 +352,8 @@ void asm_offsets(void)
> offsetof(struct task_struct, thread.s[11])
> - offsetof(struct task_struct, thread.ra)
> );
> - DEFINE(TASK_THREAD_SUM_RA,
> - offsetof(struct task_struct, thread.sum)
> + DEFINE(TASK_THREAD_SUM_MXR_RA,
> + offsetof(struct task_struct, thread.sum_mxr)
> - offsetof(struct task_struct, thread.ra)
> );
>
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index 61b7a7716697..bfd4bfa6e9fe 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -172,13 +172,13 @@ SYM_CODE_START(handle_exception)
> save_from_x6_to_x31
>
> /*
> - * Disable user-mode memory access as it should only be set in the
> - * actual user copy routines.
> + * Disable user-mode memory access and MXR as it should only be set in
> + * the actual user copy routines.
> *
> * Disable the FPU/Vector to detect illegal usage of floating point
> * or vector in kernel space.
> */
> - li t0, SR_SUM | SR_FS_VS
> + li t0, SR_SUM | SR_MXR | SR_FS_VS
> #ifdef CONFIG_64BIT
> li t1, SR_ELP
> or t0, t0, t1
> @@ -443,15 +443,15 @@ SYM_FUNC_START(__switch_to)
> REG_S s10, TASK_THREAD_S10_RA(a3)
> REG_S s11, TASK_THREAD_S11_RA(a3)
>
> - /* save the user space access flag */
> + /* save the user space access and MXR flag */
> csrr s0, CSR_STATUS
> - REG_S s0, TASK_THREAD_SUM_RA(a3)
> + REG_S s0, TASK_THREAD_SUM_MXR_RA(a3)
>
> /* Save the kernel shadow call stack pointer */
> scs_save_current
> /* Restore context from next->thread */
> - REG_L s0, TASK_THREAD_SUM_RA(a4)
> - li s1, SR_SUM
> + REG_L s0, TASK_THREAD_SUM_MXR_RA(a4)
> + li s1, SR_SUM | SR_MXR
> and s0, s0, s1
> csrs CSR_STATUS, s0
> REG_L ra, TASK_THREAD_RA_RA(a4)
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 6e8ae6c66322..f10f14001480 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -129,58 +129,6 @@ static unsigned long get_f32_rs(unsigned long insn, u8 fp_reg_offset,
> #define GET_F32_RS2C(insn, regs) (get_f32_rs(insn, 2, regs))
> #define GET_F32_RS2S(insn, regs) (get_f32_rs(RVC_RS2S(insn), 0, regs))
>
> -#define __read_insn(regs, insn, insn_addr, type) \
> -({ \
> - int __ret; \
> - \
> - if (user_mode(regs)) { \
> - __ret = get_user(insn, (type __user *) insn_addr); \
> - } else { \
> - insn = *(type *)insn_addr; \
> - __ret = 0; \
> - } \
> - \
> - __ret; \
> -})
> -
> -static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
> -{
> - ulong insn = 0;
> -
> - if (epc & 0x2) {
> - ulong tmp = 0;
> -
> - if (__read_insn(regs, insn, epc, u16))
> - return -EFAULT;
> - /* __get_user() uses regular "lw" which sign extend the loaded
> - * value make sure to clear higher order bits in case we "or" it
> - * below with the upper 16 bits half.
> - */
> - insn &= GENMASK(15, 0);
> - if ((insn & __INSN_LENGTH_MASK) != __INSN_LENGTH_32) {
> - *r_insn = insn;
> - return 0;
> - }
> - epc += sizeof(u16);
> - if (__read_insn(regs, tmp, epc, u16))
> - return -EFAULT;
> - *r_insn = (tmp << 16) | insn;
> -
> - return 0;
> - } else {
> - if (__read_insn(regs, insn, epc, u32))
> - return -EFAULT;
> - if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) {
> - *r_insn = insn;
> - return 0;
> - }
> - insn &= GENMASK(15, 0);
> - *r_insn = insn;
> -
> - return 0;
> - }
> -}
> -
> union reg_data {
> u8 data_bytes[8];
> ulong data_ulong;
> diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> index b112166d51e9..1e37810035a9 100644
> --- a/arch/riscv/kernel/vector.c
> +++ b/arch/riscv/kernel/vector.c
> @@ -184,8 +184,8 @@ EXPORT_SYMBOL_GPL(riscv_v_vstate_ctrl_user_allowed);
>
> bool riscv_v_first_use_handler(struct pt_regs *regs)
> {
> - u32 __user *epc = (u32 __user *)regs->epc;
> - u32 insn = (u32)regs->badaddr;
> + unsigned long epc = regs->epc;
> + unsigned long insn = regs->badaddr;
>
> if (!(has_vector() || has_xtheadvector()))
> return false;
> @@ -200,7 +200,7 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
>
> /* Get the instruction */
> if (!insn) {
> - if (__get_user(insn, epc))
> + if (get_insn(regs, epc, &insn))
> return false;
> }
>
> --
> 2.54.0 (Apple Git-157)
More information about the linux-riscv
mailing list