Messages in this thread Patch in this message | | | Date | Fri, 7 Aug 2020 17:19:03 +0200 | From | Marco Elver <> | Subject | Re: [PATCH] x86/paravirt: Add missing noinstr to arch_local*() helpers |
| |
On Fri, Aug 07, 2020 at 02:08PM +0200, Marco Elver wrote: > On Fri, 7 Aug 2020 at 14:04, Jürgen Groß <jgross@suse.com> wrote: > > > > On 07.08.20 13:38, Marco Elver wrote: > > > On Fri, Aug 07, 2020 at 12:35PM +0200, Jürgen Groß wrote: > > >> On 07.08.20 11:50, Marco Elver wrote: > > >>> On Fri, Aug 07, 2020 at 11:24AM +0200, Jürgen Groß wrote: > > >>>> On 07.08.20 11:01, Marco Elver wrote: > > >>>>> On Thu, 6 Aug 2020 at 18:06, Marco Elver <elver@google.com> wrote: > > >>>>>> On Thu, 6 Aug 2020 at 15:17, Marco Elver <elver@google.com> wrote: > > >>>>>>> On Thu, Aug 06, 2020 at 01:32PM +0200, peterz@infradead.org wrote: > > >>>>>>>> On Thu, Aug 06, 2020 at 09:47:23AM +0200, Marco Elver wrote: > > >>>>>>>>> Testing my hypothesis that raw then nested non-raw > > >>>>>>>>> local_irq_save/restore() breaks IRQ state tracking -- see the reproducer > > >>>>>>>>> below. This is at least 1 case I can think of that we're bound to hit. > > >>>>>>> ... > > >>>>>>>> > > >>>>>>>> /me goes ponder things... > > >>>>>>>> > > >>>>>>>> How's something like this then? > > >>>>>>>> > > >>>>>>>> --- > > >>>>>>>> include/linux/sched.h | 3 --- > > >>>>>>>> kernel/kcsan/core.c | 62 ++++++++++++++++++++++++++++++++++++--------------- > > >>>>>>>> 2 files changed, 44 insertions(+), 21 deletions(-) > > >>>>>>> > > >>>>>>> Thank you! That approach seems to pass syzbot (also with > > >>>>>>> CONFIG_PARAVIRT) and kcsan-test tests. > > >>>>>>> > > >>>>>>> I had to modify it some, so that report.c's use of the restore logic > > >>>>>>> works and not mess up the IRQ trace printed on KCSAN reports (with > > >>>>>>> CONFIG_KCSAN_VERBOSE). > > >>>>>>> > > >>>>>>> I still need to fully convince myself all is well now and we don't end > > >>>>>>> up with more fixes. :-) If it passes further testing, I'll send it as a > > >>>>>>> real patch (I want to add you as Co-developed-by, but would need your > > >>>>>>> Signed-off-by for the code you pasted, I think.) > > >>>>> > > >>>>> I let it run on syzbot through the night, and it's fine without > > >>>>> PARAVIRT (see below). I have sent the patch (need your Signed-off-by > > >>>>> as it's based on your code, thank you!): > > >>>>> https://lkml.kernel.org/r/20200807090031.3506555-1-elver@google.com > > >>>>> > > >>>>>> With CONFIG_PARAVIRT=y (without the notrace->noinstr patch), I still > > >>>>>> get lockdep DEBUG_LOCKS_WARN_ON(!lockdep_hardirqs_enabled()), although > > >>>>>> it takes longer for syzbot to hit them. But I think that's expected > > >>>>>> because we can still get the recursion that I pointed out, and will > > >>>>>> need that patch. > > >>>>> > > >>>>> Never mind, I get these warnings even if I don't turn on KCSAN > > >>>>> (CONFIG_KCSAN=n). Something else is going on with PARAVIRT=y that > > >>>>> throws off IRQ state tracking. :-/ > > >>>> > > >>>> What are the settings of CONFIG_PARAVIRT_XXL and > > >>>> CONFIG_PARAVIRT_SPINLOCKS in this case? > > >>> > > >>> I attached a config. > > >>> > > >>> $> grep PARAVIRT .config > > >>> CONFIG_PARAVIRT=y > > >>> CONFIG_PARAVIRT_XXL=y > > >>> # CONFIG_PARAVIRT_DEBUG is not set > > >>> CONFIG_PARAVIRT_SPINLOCKS=y > > >>> # CONFIG_PARAVIRT_TIME_ACCOUNTING is not set > > >>> CONFIG_PARAVIRT_CLOCK=y > > >> > > >> Anything special I need to do to reproduce the problem? Or would you be > > >> willing to do some more rounds with different config settings? > > > > > > I can only test it with syzkaller, but that probably doesn't help if you > > > don't already have it set up. It can't seem to find a C reproducer. > > > > > > I did some more rounds with different configs. > > > > > >> I think CONFIG_PARAVIRT_XXL shouldn't matter, but I'm not completely > > >> sure about that. CONFIG_PARAVIRT_SPINLOCKS would be my primary suspect. > > > > > > Yes, PARAVIRT_XXL doesn't make a different. When disabling > > > PARAVIRT_SPINLOCKS, however, the warnings go away. > > > > Thanks for testing! > > > > I take it you are doing the tests in a KVM guest? > > Yes, correct. > > > If so I have a gut feeling that the use of local_irq_save() and > > local_irq_restore() in kvm_wait() might be fishy. I might be completely > > wrong here, though. > > Happy to help debug more, although I might need patches or pointers > what to play with. > > > BTW, I think Xen's variant of pv spinlocks is fine (no playing with IRQ > > on/off). > > > > Hyper-V seems to do the same as KVM, and kicking another vcpu could be > > problematic as well, as it is just using IPI.
I experimented a bit more, and the below patch seems to solve the warnings. However, that was based on your pointer about kvm_wait(), and I can't quite tell if it is the right solution.
My hypothesis here is simply that kvm_wait() may be called in a place where we get the same case I mentioned to Peter,
raw_local_irq_save(); /* or other IRQs off without tracing */ ... kvm_wait() /* IRQ state tracing gets confused */ ... raw_local_irq_restore();
and therefore, using raw variants in kvm_wait() works. It's also safe because it doesn't call any other libraries that would result in corrupt IRQ state AFAIK.
Thanks, -- Marco
------ >8 ------
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index 233c77d056c9..1d412d1466f0 100644 --- a/arch/x86/kernel/kvm.c +++ b/arch/x86/kernel/kvm.c @@ -797,7 +797,7 @@ static void kvm_wait(u8 *ptr, u8 val) if (in_nmi()) return; - local_irq_save(flags); + raw_local_irq_save(flags); if (READ_ONCE(*ptr) != val) goto out; @@ -810,10 +810,10 @@ static void kvm_wait(u8 *ptr, u8 val) if (arch_irqs_disabled_flags(flags)) halt(); else - safe_halt(); + raw_safe_halt(); out: - local_irq_restore(flags); + raw_local_irq_restore(flags); } #ifdef CONFIG_X86_32
| |