Re: [PATCH] x86/paravirt: Add missing noinstr to arch_local*() helpers

[Marco Elver]

On Wed, Aug 05, 2020 at 03:42PM +0200, peterz@xxxxxxxxxxxxx wrote:
> On Wed, Aug 05, 2020 at 03:26:29PM +0200, Marco Elver wrote:
> > Add missing noinstr to arch_local*() helpers, as they may be called from
> > noinstr code.
> > 
> > On a KCSAN config with CONFIG_PARAVIRT=y, syzbot stumbled across corrupt
> 
> Cute, so I've been working on adding objtool support for this a little:
> 
>   https://lkml.kernel.org/r/20200803143231.GE2674@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> > diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
> > index 3d2afecde50c..a606f2ba2b5e 100644
> > --- a/arch/x86/include/asm/paravirt.h
> > +++ b/arch/x86/include/asm/paravirt.h
> > @@ -760,27 +760,27 @@ bool __raw_callee_save___native_vcpu_is_preempted(long cpu);
> >  	((struct paravirt_callee_save) { func })
> >  
> >  #ifdef CONFIG_PARAVIRT_XXL
> > -static inline notrace unsigned long arch_local_save_flags(void)
> > +static inline noinstr unsigned long arch_local_save_flags(void)
> >  {
> >  	return PVOP_CALLEE0(unsigned long, irq.save_fl);
> >  }
> >  
> > -static inline notrace void arch_local_irq_restore(unsigned long f)
> > +static inline noinstr void arch_local_irq_restore(unsigned long f)
> >  {
> >  	PVOP_VCALLEE1(irq.restore_fl, f);
> >  }
> >  
> > -static inline notrace void arch_local_irq_disable(void)
> > +static inline noinstr void arch_local_irq_disable(void)
> >  {
> >  	PVOP_VCALLEE0(irq.irq_disable);
> >  }
> >  
> > -static inline notrace void arch_local_irq_enable(void)
> > +static inline noinstr void arch_local_irq_enable(void)
> >  {
> >  	PVOP_VCALLEE0(irq.irq_enable);
> >  }
> >  
> > -static inline notrace unsigned long arch_local_irq_save(void)
> > +static inline noinstr unsigned long arch_local_irq_save(void)
> >  {
> >  	unsigned long f;
> >  
> 
> Shouldn't we __always_inline those? They're going to be really small.

I can send a v2, and you can choose. For reference, though:

	ffffffff86271ee0 <arch_local_save_flags>:
	ffffffff86271ee0:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff86271ee5:       48 83 3d 43 87 e4 01    cmpq   $0x0,0x1e48743(%rip)        # ffffffff880ba630 <pv_ops+0x120>
	ffffffff86271eec:       00
	ffffffff86271eed:       74 0d                   je     ffffffff86271efc <arch_local_save_flags+0x1c>
	ffffffff86271eef:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff86271ef4:       ff 14 25 30 a6 0b 88    callq  *0xffffffff880ba630
	ffffffff86271efb:       c3                      retq
	ffffffff86271efc:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff86271f01:       0f 0b                   ud2
	ffffffff86271f03:       66 66 2e 0f 1f 84 00    data16 nopw %cs:0x0(%rax,%rax,1)
	ffffffff86271f0a:       00 00 00 00
	ffffffff86271f0e:       66 90                   xchg   %ax,%ax

	[...]

	ffffffff86271a90 <arch_local_irq_restore>:
	ffffffff86271a90:       53                      push   %rbx
	ffffffff86271a91:       48 89 fb                mov    %rdi,%rbx
	ffffffff86271a94:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff86271a99:       48 83 3d 97 8b e4 01    cmpq   $0x0,0x1e48b97(%rip)        # ffffffff880ba638 <pv_ops+0x128>
	ffffffff86271aa0:       00
	ffffffff86271aa1:       74 11                   je     ffffffff86271ab4 <arch_local_irq_restore+0x24>
	ffffffff86271aa3:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff86271aa8:       48 89 df                mov    %rbx,%rdi
	ffffffff86271aab:       ff 14 25 38 a6 0b 88    callq  *0xffffffff880ba638
	ffffffff86271ab2:       5b                      pop    %rbx
	ffffffff86271ab3:       c3                      retq
	ffffffff86271ab4:       0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)
	ffffffff86271ab9:       0f 0b                   ud2
	ffffffff86271abb:       cc                      int3
	ffffffff86271abc:       cc                      int3
	ffffffff86271abd:       cc                      int3
	ffffffff86271abe:       cc                      int3
	ffffffff86271abf:       cc                      int3

	[... and the rest looking of similar size.]

Thanks,
-- Marco