2020-07-04 20:47:21

by David P. Reed

[permalink] [raw]
Subject: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

Fix: Mask undefined operation fault during emergency VMXOFF that must be
attempted to force cpu exit from VMX root operation.
Explanation: When a cpu may be in VMX root operation (only possible when
CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
using VMXOFF. This is necessary, because any INIT will be masked while cpu
is in VMX root operation, but that state cannot be reliably
discerned by the state of the cpu.
VMXOFF faults if the cpu is not actually in VMX root operation, signalling
undefined operation.
Discovered while debugging an out-of-tree x-visor with a race. Can happen
due to certain kinds of bugs in KVM.

Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
Reported-by: David P. Reed <[email protected]>
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: Sean Christopherson <[email protected]>
Suggested-by: Andy Lutomirski <[email protected]>
Signed-off-by: David P. Reed <[email protected]>
---
arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 0ede8d04535a..0e0900eacb9c 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
}


-/* Disable VMX on the current CPU
+/* Exit VMX root mode and isable VMX on the current CPU.
*
* vmxoff causes a undefined-opcode exception if vmxon was not run
- * on the CPU previously. Only call this function if you know VMX
- * is enabled.
+ * on the CPU previously. Only call this function if you know cpu
+ * is in VMX root mode.
*/
static inline void cpu_vmxoff(void)
{
@@ -47,14 +47,22 @@ static inline int cpu_vmx_enabled(void)
return __read_cr4() & X86_CR4_VMXE;
}

-/* Disable VMX if it is enabled on the current CPU
+/* Safely exit VMX root mode and disable VMX if VMX enabled
+ * on the current CPU. Handle undefined-opcode fault
+ * that can occur if cpu is not in VMX root mode, due
+ * to a race.
*
* You shouldn't call this if cpu_has_vmx() returns 0.
*/
static inline void __cpu_emergency_vmxoff(void)
{
- if (cpu_vmx_enabled())
- cpu_vmxoff();
+ if (!cpu_vmx_enabled())
+ return;
+ asm volatile ("1:vmxoff\n\t"
+ "2:\n\t"
+ _ASM_EXTABLE(1b, 2b)
+ ::: "cc", "memory");
+ cr4_clear_bits(X86_CR4_VMXE);
}

/* Disable VMX if it is supported and enabled on the current CPU
--
2.26.2


2020-07-05 18:23:39

by Andy Lutomirski

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> wrote:
>
> Fix: Mask undefined operation fault during emergency VMXOFF that must be
> attempted to force cpu exit from VMX root operation.
> Explanation: When a cpu may be in VMX root operation (only possible when
> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
> using VMXOFF. This is necessary, because any INIT will be masked while cpu
> is in VMX root operation, but that state cannot be reliably
> discerned by the state of the cpu.
> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
> undefined operation.
> Discovered while debugging an out-of-tree x-visor with a race. Can happen
> due to certain kinds of bugs in KVM.

Can you re-wrap lines to 68 characters? Also, the Fix: and
Explanation: is probably unnecessary. You could say:

Ignore a potential #UD failut during emergency VMXOFF ...

When a cpu may be in VMX ...

>
> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
> Reported-by: David P. Reed <[email protected]>

It's not really necessary to say that you, the author, reported the
problem, but I guess it's harmless.

> Suggested-by: Thomas Gleixner <[email protected]>
> Suggested-by: Sean Christopherson <[email protected]>
> Suggested-by: Andy Lutomirski <[email protected]>
> Signed-off-by: David P. Reed <[email protected]>
> ---
> arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 0ede8d04535a..0e0900eacb9c 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
> }
>
>
> -/* Disable VMX on the current CPU
> +/* Exit VMX root mode and isable VMX on the current CPU.

s/isable/disable/


> /* Disable VMX if it is supported and enabled on the current CPU
> --
> 2.26.2
>

Other than that:

Reviewed-by: Andy Lutomirski <[email protected]>

--Andy

2020-07-05 19:53:39

by David P. Reed

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

Thanks, will handle these. 2 questions below.

On Sunday, July 5, 2020 2:22pm, "Andy Lutomirski" <[email protected]> said:

> On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> wrote:
>>
>> Fix: Mask undefined operation fault during emergency VMXOFF that must be
>> attempted to force cpu exit from VMX root operation.
>> Explanation: When a cpu may be in VMX root operation (only possible when
>> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
>> using VMXOFF. This is necessary, because any INIT will be masked while cpu
>> is in VMX root operation, but that state cannot be reliably
>> discerned by the state of the cpu.
>> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
>> undefined operation.
>> Discovered while debugging an out-of-tree x-visor with a race. Can happen
>> due to certain kinds of bugs in KVM.
>
> Can you re-wrap lines to 68 characters? Also, the Fix: and

I used 'scripts/checkpatch.pl' and it had me wrap to 75 chars:
"WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)"

Should I submit a fix to checkpatch.pl to say 68?

> Explanation: is probably unnecessary. You could say:
>
> Ignore a potential #UD failut during emergency VMXOFF ...
>
> When a cpu may be in VMX ...
>
>>
>> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
>> Reported-by: David P. Reed <[email protected]>
>
> It's not really necessary to say that you, the author, reported the
> problem, but I guess it's harmless.
>
>> Suggested-by: Thomas Gleixner <[email protected]>
>> Suggested-by: Sean Christopherson <[email protected]>
>> Suggested-by: Andy Lutomirski <[email protected]>
>> Signed-off-by: David P. Reed <[email protected]>
>> ---
>> arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>> index 0ede8d04535a..0e0900eacb9c 100644
>> --- a/arch/x86/include/asm/virtext.h
>> +++ b/arch/x86/include/asm/virtext.h
>> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>> }
>>
>>
>> -/* Disable VMX on the current CPU
>> +/* Exit VMX root mode and isable VMX on the current CPU.
>
> s/isable/disable/
>
>
>> /* Disable VMX if it is supported and enabled on the current CPU
>> --
>> 2.26.2
>>
>
> Other than that:
>
> Reviewed-by: Andy Lutomirski <[email protected]>

As a newbie, I have a process question - should I resend the patch with the 'Reviewed-by' line, as well as correcting the other wording? Thanks!

>
> --Andy
>


2020-07-05 20:56:39

by Andy Lutomirski

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

On Sun, Jul 5, 2020 at 12:52 PM David P. Reed <[email protected]> wrote:
>
> Thanks, will handle these. 2 questions below.
>
> On Sunday, July 5, 2020 2:22pm, "Andy Lutomirski" <[email protected]> said:
>
> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> wrote:
> >>
> >> Fix: Mask undefined operation fault during emergency VMXOFF that must be
> >> attempted to force cpu exit from VMX root operation.
> >> Explanation: When a cpu may be in VMX root operation (only possible when
> >> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
> >> using VMXOFF. This is necessary, because any INIT will be masked while cpu
> >> is in VMX root operation, but that state cannot be reliably
> >> discerned by the state of the cpu.
> >> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
> >> undefined operation.
> >> Discovered while debugging an out-of-tree x-visor with a race. Can happen
> >> due to certain kinds of bugs in KVM.
> >
> > Can you re-wrap lines to 68 characters? Also, the Fix: and
>
> I used 'scripts/checkpatch.pl' and it had me wrap to 75 chars:
> "WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)"
>
> Should I submit a fix to checkpatch.pl to say 68?

75 is probably fine too, but something is odd about your wrapping.
You have long lines mostly alternating with short lines. It's as if
you wrote 120-ish character lines and then wrapped to 75 without
reflowing.

>
> > Explanation: is probably unnecessary. You could say:
> >
> > Ignore a potential #UD failut during emergency VMXOFF ...
> >
> > When a cpu may be in VMX ...
> >
> >>
> >> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
> >> Reported-by: David P. Reed <[email protected]>
> >
> > It's not really necessary to say that you, the author, reported the
> > problem, but I guess it's harmless.
> >
> >> Suggested-by: Thomas Gleixner <[email protected]>
> >> Suggested-by: Sean Christopherson <[email protected]>
> >> Suggested-by: Andy Lutomirski <[email protected]>
> >> Signed-off-by: David P. Reed <[email protected]>
> >> ---
> >> arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
> >> 1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> >> index 0ede8d04535a..0e0900eacb9c 100644
> >> --- a/arch/x86/include/asm/virtext.h
> >> +++ b/arch/x86/include/asm/virtext.h
> >> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
> >> }
> >>
> >>
> >> -/* Disable VMX on the current CPU
> >> +/* Exit VMX root mode and isable VMX on the current CPU.
> >
> > s/isable/disable/
> >
> >
> >> /* Disable VMX if it is supported and enabled on the current CPU
> >> --
> >> 2.26.2
> >>
> >
> > Other than that:
> >
> > Reviewed-by: Andy Lutomirski <[email protected]>
>
> As a newbie, I have a process question - should I resend the patch with the 'Reviewed-by' line, as well as correcting the other wording? Thanks!

Probably. Sometimes a maintainer will apply the patch and make these
types of cosmetic changes, but it's easier if you resubmit. That
being said, for non-urgent patches, it's usually considered polite to
wait a day or two to give other people a chance to comment.

--Andy

2020-07-05 22:19:13

by David P. Reed

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic



On Sunday, July 5, 2020 4:55pm, "Andy Lutomirski" <[email protected]> said:

> On Sun, Jul 5, 2020 at 12:52 PM David P. Reed <[email protected]> wrote:
>>
>> Thanks, will handle these. 2 questions below.
>>
>> On Sunday, July 5, 2020 2:22pm, "Andy Lutomirski" <[email protected]> said:
>>
>> > On Sat, Jul 4, 2020 at 1:38 PM David P. Reed <[email protected]> wrote:
>> >>
>> >> Fix: Mask undefined operation fault during emergency VMXOFF that must be
>> >> attempted to force cpu exit from VMX root operation.
>> >> Explanation: When a cpu may be in VMX root operation (only possible when
>> >> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
>> >> using VMXOFF. This is necessary, because any INIT will be masked while cpu
>> >> is in VMX root operation, but that state cannot be reliably
>> >> discerned by the state of the cpu.
>> >> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
>> >> undefined operation.
>> >> Discovered while debugging an out-of-tree x-visor with a race. Can happen
>> >> due to certain kinds of bugs in KVM.
>> >
>> > Can you re-wrap lines to 68 characters? Also, the Fix: and
>>
>> I used 'scripts/checkpatch.pl' and it had me wrap to 75 chars:
>> "WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per
>> line)"
>>
>> Should I submit a fix to checkpatch.pl to say 68?
>
> 75 is probably fine too, but something is odd about your wrapping.
> You have long lines mostly alternating with short lines. It's as if
> you wrote 120-ish character lines and then wrapped to 75 without
> reflowing.
My emacs settings tend to wrap at about 85 depending on file type (big screens). I did the shortening manually, aimed at breaking at meaningful points, not worrying too much about
line-length uniformity.

>
>>
>> > Explanation: is probably unnecessary. You could say:
>> >
>> > Ignore a potential #UD failut during emergency VMXOFF ...
>> >
>> > When a cpu may be in VMX ...
>> >
>> >>
>> >> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
>> >> Reported-by: David P. Reed <[email protected]>
>> >
>> > It's not really necessary to say that you, the author, reported the
>> > problem, but I guess it's harmless.
>> >
>> >> Suggested-by: Thomas Gleixner <[email protected]>
>> >> Suggested-by: Sean Christopherson <[email protected]>
>> >> Suggested-by: Andy Lutomirski <[email protected]>
>> >> Signed-off-by: David P. Reed <[email protected]>
>> >> ---
>> >> arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>> >> 1 file changed, 14 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>> >> index 0ede8d04535a..0e0900eacb9c 100644
>> >> --- a/arch/x86/include/asm/virtext.h
>> >> +++ b/arch/x86/include/asm/virtext.h
>> >> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>> >> }
>> >>
>> >>
>> >> -/* Disable VMX on the current CPU
>> >> +/* Exit VMX root mode and isable VMX on the current CPU.
>> >
>> > s/isable/disable/
>> >
>> >
>> >> /* Disable VMX if it is supported and enabled on the current CPU
>> >> --
>> >> 2.26.2
>> >>
>> >
>> > Other than that:
>> >
>> > Reviewed-by: Andy Lutomirski <[email protected]>
>>
>> As a newbie, I have a process question - should I resend the patch with the
>> 'Reviewed-by' line, as well as correcting the other wording? Thanks!
>
> Probably. Sometimes a maintainer will apply the patch and make these
> types of cosmetic changes, but it's easier if you resubmit. That
> being said, for non-urgent patches, it's usually considered polite to
> wait a day or two to give other people a chance to comment.

I'm not sure which maintainer will move the patches along. I am waiting for additional input, but will resubmit in a day or two.

>
> --Andy
>


2020-07-07 05:12:28

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

On Sat, Jul 04, 2020 at 04:38:08PM -0400, David P. Reed wrote:
> Fix: Mask undefined operation fault during emergency VMXOFF that must be
> attempted to force cpu exit from VMX root operation.
> Explanation: When a cpu may be in VMX root operation (only possible when
> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
> using VMXOFF. This is necessary, because any INIT will be masked while cpu
> is in VMX root operation, but that state cannot be reliably
> discerned by the state of the cpu.
> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
> undefined operation.
> Discovered while debugging an out-of-tree x-visor with a race. Can happen
> due to certain kinds of bugs in KVM.
>
> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
> Reported-by: David P. Reed <[email protected]>
> Suggested-by: Thomas Gleixner <[email protected]>
> Suggested-by: Sean Christopherson <[email protected]>
> Suggested-by: Andy Lutomirski <[email protected]>
> Signed-off-by: David P. Reed <[email protected]>
> ---
> arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 0ede8d04535a..0e0900eacb9c 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
> }
>
>
> -/* Disable VMX on the current CPU
> +/* Exit VMX root mode and isable VMX on the current CPU.
> *
> * vmxoff causes a undefined-opcode exception if vmxon was not run
> - * on the CPU previously. Only call this function if you know VMX
> - * is enabled.
> + * on the CPU previously. Only call this function if you know cpu
> + * is in VMX root mode.
> */
> static inline void cpu_vmxoff(void)
> {
> @@ -47,14 +47,22 @@ static inline int cpu_vmx_enabled(void)
> return __read_cr4() & X86_CR4_VMXE;
> }
>
> -/* Disable VMX if it is enabled on the current CPU
> +/* Safely exit VMX root mode and disable VMX if VMX enabled
> + * on the current CPU. Handle undefined-opcode fault
> + * that can occur if cpu is not in VMX root mode, due
> + * to a race.
> *
> * You shouldn't call this if cpu_has_vmx() returns 0.
> */
> static inline void __cpu_emergency_vmxoff(void)
> {
> - if (cpu_vmx_enabled())
> - cpu_vmxoff();
> + if (!cpu_vmx_enabled())
> + return;
> + asm volatile ("1:vmxoff\n\t"
> + "2:\n\t"
> + _ASM_EXTABLE(1b, 2b)
> + ::: "cc", "memory");
> + cr4_clear_bits(X86_CR4_VMXE);

Open coding vmxoff doesn't make sense, and IMO is flat out wrong as it fixes
flows that use __cpu_emergency_vmxoff() but leaves the same bug hanging
around in emergency_vmx_disable_all() until the next patch.

The reason I say it doesn't make sense is that there is no sane scenario
where the generic vmxoff helper should _not_ eat the fault. All other VMXOFF
faults are mode related, i.e. any fault is guaranteed to be due to the
!post-VMXON check unless we're magically in RM, VM86, compat mode, or at
CPL>0. Given that the whole point of this series is that it's impossible to
determine whether or not the CPU if post-VMXON if CR4.VMXE=1 without taking a
fault of some form, there's simply no way that anything except the hypervisor
(in normal operation) can know the state of VMX. And given that the only
in-tree hypervisor (KVM) has its own version of vmxoff, that means there is
no scenario in which cpu_vmxoff() can safely be used. Case in point, after
the next patch there are no users of cpu_vmxoff().

TL;DR: Just do fixup on cpu_vmxoff().

> }
>
> /* Disable VMX if it is supported and enabled on the current CPU
> --
> 2.26.2
>

2020-07-07 19:12:01

by David P. Reed

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic



On Tuesday, July 7, 2020 1:09am, "Sean Christopherson" <[email protected]> said:

> On Sat, Jul 04, 2020 at 04:38:08PM -0400, David P. Reed wrote:
>> Fix: Mask undefined operation fault during emergency VMXOFF that must be
>> attempted to force cpu exit from VMX root operation.
>> Explanation: When a cpu may be in VMX root operation (only possible when
>> CR4.VMXE is set), crash or panic reboot tries to exit VMX root operation
>> using VMXOFF. This is necessary, because any INIT will be masked while cpu
>> is in VMX root operation, but that state cannot be reliably
>> discerned by the state of the cpu.
>> VMXOFF faults if the cpu is not actually in VMX root operation, signalling
>> undefined operation.
>> Discovered while debugging an out-of-tree x-visor with a race. Can happen
>> due to certain kinds of bugs in KVM.
>>
>> Fixes: 208067 <https://bugzilla.kernel.org/show_bug.cgi?id=208067>
>> Reported-by: David P. Reed <[email protected]>
>> Suggested-by: Thomas Gleixner <[email protected]>
>> Suggested-by: Sean Christopherson <[email protected]>
>> Suggested-by: Andy Lutomirski <[email protected]>
>> Signed-off-by: David P. Reed <[email protected]>
>> ---
>> arch/x86/include/asm/virtext.h | 20 ++++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>> index 0ede8d04535a..0e0900eacb9c 100644
>> --- a/arch/x86/include/asm/virtext.h
>> +++ b/arch/x86/include/asm/virtext.h
>> @@ -30,11 +30,11 @@ static inline int cpu_has_vmx(void)
>> }
>>
>>
>> -/* Disable VMX on the current CPU
>> +/* Exit VMX root mode and isable VMX on the current CPU.
>> *
>> * vmxoff causes a undefined-opcode exception if vmxon was not run
>> - * on the CPU previously. Only call this function if you know VMX
>> - * is enabled.
>> + * on the CPU previously. Only call this function if you know cpu
>> + * is in VMX root mode.
>> */
>> static inline void cpu_vmxoff(void)
>> {
>> @@ -47,14 +47,22 @@ static inline int cpu_vmx_enabled(void)
>> return __read_cr4() & X86_CR4_VMXE;
>> }
>>
>> -/* Disable VMX if it is enabled on the current CPU
>> +/* Safely exit VMX root mode and disable VMX if VMX enabled
>> + * on the current CPU. Handle undefined-opcode fault
>> + * that can occur if cpu is not in VMX root mode, due
>> + * to a race.
>> *
>> * You shouldn't call this if cpu_has_vmx() returns 0.
>> */
>> static inline void __cpu_emergency_vmxoff(void)
>> {
>> - if (cpu_vmx_enabled())
>> - cpu_vmxoff();
>> + if (!cpu_vmx_enabled())
>> + return;
>> + asm volatile ("1:vmxoff\n\t"
>> + "2:\n\t"
>> + _ASM_EXTABLE(1b, 2b)
>> + ::: "cc", "memory");
>> + cr4_clear_bits(X86_CR4_VMXE);
>
> Open coding vmxoff doesn't make sense, and IMO is flat out wrong as it fixes
> flows that use __cpu_emergency_vmxoff() but leaves the same bug hanging
> around in emergency_vmx_disable_all() until the next patch.
>
> The reason I say it doesn't make sense is that there is no sane scenario
> where the generic vmxoff helper should _not_ eat the fault. All other VMXOFF
> faults are mode related, i.e. any fault is guaranteed to be due to the
> !post-VMXON check unless we're magically in RM, VM86, compat mode, or at
> CPL>0. Given that the whole point of this series is that it's impossible to
> determine whether or not the CPU if post-VMXON if CR4.VMXE=1 without taking a
> fault of some form, there's simply no way that anything except the hypervisor
> (in normal operation) can know the state of VMX. And given that the only
> in-tree hypervisor (KVM) has its own version of vmxoff, that means there is
> no scenario in which cpu_vmxoff() can safely be used. Case in point, after
> the next patch there are no users of cpu_vmxoff().
>
> TL;DR: Just do fixup on cpu_vmxoff().

Personally, I don't care either way, since it fixes the bug either way (and it's inlined, so either way no additional code is generated. I was just being conservative since the cpu_vmxoff() is exported throughout the kernel source, so it might be expected to stay the same (when not in an "emergency"). I'll wait a day or two for any objections to just doing the fix in cpu_vmxoff() by other commenters. WIth no objection, I'll just do it that way.

Sean, are you the one who would get this particular fix pushed into Linus's tree, by the way? The "maintainership" is not clear to me. If you are, happy to take direction from you as the primary input.

>
>> }
>>
>> /* Disable VMX if it is supported and enabled on the current CPU
>> --
>> 2.26.2
>>
>


2020-07-07 19:25:58

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic

On Tue, Jul 07, 2020 at 03:09:38PM -0400, David P. Reed wrote:
>
> On Tuesday, July 7, 2020 1:09am, "Sean Christopherson" <[email protected]> said:
> Sean, are you the one who would get this particular fix pushed into Linus's
> tree, by the way? The "maintainership" is not clear to me.

Nope, I'm just here to complain and nitpick :-) There's no direct maintainer
for virtext.h so it falls under the higher level arch/x86 umbrella, i.e. I
expect Boris/Thomas/Ingo will pick this up.

2020-07-07 19:52:57

by David P. Reed

[permalink] [raw]
Subject: Re: [PATCH v3 2/3] Fix undefined operation fault that can hang a cpu on crash or panic



On Tuesday, July 7, 2020 3:24pm, "Sean Christopherson" <[email protected]> said:

> On Tue, Jul 07, 2020 at 03:09:38PM -0400, David P. Reed wrote:
>>
>> On Tuesday, July 7, 2020 1:09am, "Sean Christopherson"
>> <[email protected]> said:
>> Sean, are you the one who would get this particular fix pushed into Linus's
>> tree, by the way? The "maintainership" is not clear to me.
>
> Nope, I'm just here to complain and nitpick :-) There's no direct maintainer
> for virtext.h so it falls under the higher level arch/x86 umbrella, i.e. I
> expect Boris/Thomas/Ingo will pick this up.
>
Thanks for your time and effort in helping.