Messages in this thread | | | From | Marco Elver <> | Date | Fri, 7 Aug 2020 14:08:11 +0200 | Subject | Re: [PATCH] x86/paravirt: Add missing noinstr to arch_local*() helpers |
| |
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. > > > Juergen
| |