Re: [PATCH V3 4/7] riscv: convert to generic entry

[Peter Zijlstra]

On Mon, Sep 05, 2022 at 11:54:20PM -0400, guoren@xxxxxxxxxx wrote:

> +asmlinkage void noinstr do_riscv_irq(struct pt_regs *regs)
> +{
> +	struct pt_regs *old_regs;
> +	irqentry_state_t state = irqentry_enter(regs);
> +
> +	irq_enter_rcu();
> +	old_regs = set_irq_regs(regs);
> +	handle_arch_irq(regs);
> +	set_irq_regs(old_regs);
> +	irq_exit_rcu();
> +
> +	irqentry_exit(regs, state);
> +}

The above is right in that everything that calls irqentry_enter() should
be noinstr; however all the below instances get it wrong:

>  #define DO_ERROR_INFO(name, signo, code, str)				\
>  asmlinkage __visible __trap_section void name(struct pt_regs *regs)	\
>  {									\
> +	irqentry_state_t state = irqentry_enter(regs);			\
>  	do_trap_error(regs, signo, code, regs->epc, "Oops - " str);	\
> +	irqentry_exit(regs, state);					\
>  }
>  
>  DO_ERROR_INFO(do_trap_unknown,
> @@ -123,18 +126,22 @@ int handle_misaligned_store(struct pt_regs *regs);
>  
>  asmlinkage void __trap_section do_trap_load_misaligned(struct pt_regs *regs)
>  {
> +	irqentry_state_t state = irqentry_enter(regs);
>  	if (!handle_misaligned_load(regs))
>  		return;
>  	do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
>  		      "Oops - load address misaligned");
> +	irqentry_exit(regs, state);
>  }
>  
>  asmlinkage void __trap_section do_trap_store_misaligned(struct pt_regs *regs)
>  {
> +	irqentry_state_t state = irqentry_enter(regs);
>  	if (!handle_misaligned_store(regs))
>  		return;
>  	do_trap_error(regs, SIGBUS, BUS_ADRALN, regs->epc,
>  		      "Oops - store (or AMO) address misaligned");
> +	irqentry_exit(regs, state);
>  }
>  #endif
>  DO_ERROR_INFO(do_trap_store_fault,
> @@ -158,6 +165,8 @@ static inline unsigned long get_break_insn_length(unsigned long pc)
>  
>  asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
>  {
> +	irqentry_state_t state = irqentry_enter(regs);
> +
>  #ifdef CONFIG_KPROBES
>  	if (kprobe_single_step_handler(regs))
>  		return;
> @@ -185,6 +194,8 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
>  		regs->epc += get_break_insn_length(regs->epc);
>  	else
>  		die(regs, "Kernel BUG");
> +
> +	irqentry_exit(regs, state);
>  }
>  NOKPROBE_SYMBOL(do_trap_break);

> +asmlinkage void do_page_fault(struct pt_regs *regs)
> +{
> +	irqentry_state_t state = irqentry_enter(regs);
> +
> +	__do_page_fault(regs);
> +
> +	irqentry_exit(regs, state);
> +}
>  NOKPROBE_SYMBOL(do_page_fault);

Without noinstr the compiler is free to insert instrumentation (think
all the k*SAN, KCov, GCov, ftrace etc..) which can call code we're not
yet ready to run this early in the entry path, for instance it could
rely on RCU which isn't on yet, or expect lockdep state.