On Wed, Aug 05, 2020 at 03:42PM +0200, [email protected] 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/[email protected]
>
> > 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
On Wed, Aug 05, 2020 at 03:59:40PM +0200, Marco Elver wrote:
> On Wed, Aug 05, 2020 at 03:42PM +0200, [email protected] wrote:
> > 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
> 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
Blergh, that's abysmall. In part I suspect because you have
CONFIG_PARAVIRT_DEBUG, let me try and untangle that PV macro maze.
On Wed, Aug 05, 2020 at 04:12:37PM +0200, [email protected] wrote:
> On Wed, Aug 05, 2020 at 03:59:40PM +0200, Marco Elver wrote:
> > On Wed, Aug 05, 2020 at 03:42PM +0200, [email protected] wrote:
>
> > > 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
>
> > 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
>
>
> Blergh, that's abysmall. In part I suspect because you have
> CONFIG_PARAVIRT_DEBUG, let me try and untangle that PV macro maze.
Yeah, look here:
0000 0000000000462149 <arch_local_save_flags>:
0000 462149: ff 14 25 00 00 00 00 callq *0x0
0003 46214c: R_X86_64_32S pv_ops+0x120
0007 462150: c3 retq
That's exactly what I was expecting.
On Wed, 5 Aug 2020 at 16:17, <[email protected]> wrote:
>
> On Wed, Aug 05, 2020 at 04:12:37PM +0200, [email protected] wrote:
> > On Wed, Aug 05, 2020 at 03:59:40PM +0200, Marco Elver wrote:
> > > On Wed, Aug 05, 2020 at 03:42PM +0200, [email protected] wrote:
> >
> > > > 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
> >
> > > 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
> >
> >
> > Blergh, that's abysmall. In part I suspect because you have
> > CONFIG_PARAVIRT_DEBUG, let me try and untangle that PV macro maze.
>
> Yeah, look here:
>
> 0000 0000000000462149 <arch_local_save_flags>:
> 0000 462149: ff 14 25 00 00 00 00 callq *0x0
> 0003 46214c: R_X86_64_32S pv_ops+0x120
> 0007 462150: c3 retq
>
>
> That's exactly what I was expecting.
Ah, for some reason the __always_inline version does *not* work with
KCSAN -- I'm getting various warnings, including the same lockdep
warning. I think there is some weirdness when this stuff gets inlined
into instrumented functions. At least with KCSAN, when any accesses
here are instrumented, and then KCSAN disable/enables interrupts,
things break. So, these functions should never be instrumented,
noinstr or not. Marking them 'inline noinstr' seems like the safest
option. Without CONFIG_PARAVIRT_DEBUG, any compiler should hopefully
inline them?
Thanks,
-- Marco