linux-kernel - Re: [PATCH] KVM: x86: hyper-v: Don't auto-enable stimer during deserialization

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 16 Oct 2023 17:04:15 +0000
From:   Nicolas Saenz Julienne <nsaenz@...zon.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Vitaly Kuznetsov <vkuznets@...hat.com>, <kvm@...r.kernel.org>,
        <pbonzini@...hat.com>, <tglx@...utronix.de>, <mingo@...hat.com>,
        <bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
        <hpa@...or.com>, <graf@...zon.de>, <rkagan@...zon.de>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KVM: x86: hyper-v: Don't auto-enable stimer during
 deserialization

Hi Sean,
On Mon Oct 16, 2023 at 4:27 PM UTC, Sean Christopherson wrote:
> I'd prefer the shortlog be more explicit about the write coming from userspace, e.g.
>
>   KVM: x86: hyper-v: Don't auto-enable stimer on write from userspace
>
> A non-zero number of KVM's "deserialization" ioctls are used to stuff state
> without a paired "serialization".  I doubt anyone is doing that with the Hyper-V
> ioctls, but keeping things consistent is helpful for readers.
>
> On Mon, Oct 16, 2023, Nicolas Saenz Julienne wrote:
> > Hi Vitaly,
> >
> > On Mon Oct 16, 2023 at 12:14 PM UTC, Vitaly Kuznetsov wrote:
> > > Nicolas Saenz Julienne <nsaenz@...zon.com> writes:
> > >
> > > > By not honoring the 'stimer->config.enable' state during stimer
> > > > deserialization we might introduce spurious timer interrupts. For
>
> Avoid pronouns please.
>
> > > > example through the following events:
> > > >  - The stimer is configured in auto-enable mode.
> > > >  - The stimer's count is set and the timer enabled.
> > > >  - The stimer expires, an interrupt is injected.
> > > >  - We live migrate the VM.
>
> Same here.  "We" is already ambiguous, because the first usage is largely about
> KVM, and the second usage here is much more about userspace and/or the actual
> user.
>
> > > >  - The stimer config and count are deserialized, auto-enable is ON, the
> > > >    stimer is re-enabled.
> > > >  - The stimer expires right away, and injects an unwarranted interrupt.
> > > >
> > > > So let's not change the stimer's enable state if the MSR write comes
> > > > from user-space.
>
> Don't hedge, firmly state what the patch does and why the change is necessary
> and correct.  If it turns out the change is wrong, then the follow-up patch can
> explain the situation.  But in the happy case where the change is correct, using
> language that isn't assertive can result in
>
> > > > Fixes: 1f4b34f825e8 ("kvm/x86: Hyper-V SynIC timers")
>
> Does this need a?
>
>   Cc: stable@...r.kernel


Your reply raced with my v2. I'll rework the commit message, and send a
third revision.

Nicolas