[v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

[PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Wei Wang 2 years, 6 months ago

Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from callers is
32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
will be converted to 0, which is not expected.

Improves the implementation by using bool in KVM_BUG and KVM_BUG_ON.
'bool' is preferred to 'int' as __ret is essentially used as a boolean
and coding-stytle.rst documents that use of bool is encouraged to improve
readability and is often a better option than 'int' for storing boolean
values.

Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 include/linux/kvm_host.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f06635b24bd0..0221a48b3e3d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -881,7 +881,7 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
 
 #define KVM_BUG(cond, kvm, fmt...)				\
 ({								\
-	int __ret = (cond);					\
+	bool __ret = !!(cond);					\
 								\
 	if (WARN_ONCE(__ret && !(kvm)->vm_bugged, fmt))		\
 		kvm_vm_bugged(kvm);				\
@@ -890,7 +890,7 @@ static inline void kvm_vm_bugged(struct kvm *kvm)
 
 #define KVM_BUG_ON(cond, kvm)					\
 ({								\
-	int __ret = (cond);					\
+	bool __ret = !!(cond);					\
 								\
 	if (WARN_ON_ONCE(__ret && !(kvm)->vm_bugged))		\
 		kvm_vm_bugged(kvm);				\
-- 
2.27.0

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Sean Christopherson 2 years, 3 months ago

On Tue, 07 Mar 2023 21:52:33 +0800, Wei Wang wrote:
> Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from callers is
> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
> will be converted to 0, which is not expected.
> 
> Improves the implementation by using bool in KVM_BUG and KVM_BUG_ON.
> 'bool' is preferred to 'int' as __ret is essentially used as a boolean
> and coding-stytle.rst documents that use of bool is encouraged to improve
> readability and is often a better option than 'int' for storing boolean
> values.
> 
> [...]

Applied to kvm-x86 generic, thanks!

[1/1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond
      https://github.com/kvm-x86/linux/commit/c9d601548603

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Michal Luczaj 2 years, 3 months ago

On 6/2/23 03:20, Sean Christopherson wrote:
> On Tue, 07 Mar 2023 21:52:33 +0800, Wei Wang wrote:
>> Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from callers is
>> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
>> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
>> will be converted to 0, which is not expected.
>>
>> Improves the implementation by using bool in KVM_BUG and KVM_BUG_ON.
>> 'bool' is preferred to 'int' as __ret is essentially used as a boolean
>> and coding-stytle.rst documents that use of bool is encouraged to improve
>> readability and is often a better option than 'int' for storing boolean
>> values.
>>
>> [...]
> 
> Applied to kvm-x86 generic, thanks!
> 
> [1/1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond
>       https://github.com/kvm-x86/linux/commit/c9d601548603

I guess this makes the !! in kvm_vm_ioctl_create_vcpu() unnecessary:

KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0)...

Is it worth a patch (perhaps along with chopping off !! in
kvm_msr_allowed() and few other places)?

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Sean Christopherson 2 years, 3 months ago

On Fri, Jun 02, 2023, Michal Luczaj wrote:
> On 6/2/23 03:20, Sean Christopherson wrote:
> > On Tue, 07 Mar 2023 21:52:33 +0800, Wei Wang wrote:
> >> Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from callers is
> >> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> >> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
> >> will be converted to 0, which is not expected.
> >>
> >> Improves the implementation by using bool in KVM_BUG and KVM_BUG_ON.
> >> 'bool' is preferred to 'int' as __ret is essentially used as a boolean
> >> and coding-stytle.rst documents that use of bool is encouraged to improve
> >> readability and is often a better option than 'int' for storing boolean
> >> values.
> >>
> >> [...]
> > 
> > Applied to kvm-x86 generic, thanks!
> > 
> > [1/1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond
> >       https://github.com/kvm-x86/linux/commit/c9d601548603
> 
> I guess this makes the !! in kvm_vm_ioctl_create_vcpu() unnecessary:
> 
> KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0)...

Ya, I saw that, which in addition to Wei's ping, is what reminded me that the
KVM_BUG_ON() fix hadn't been merged.

> Is it worth a patch (perhaps along with chopping off !! in
> kvm_msr_allowed() and few other places)?

Yes, I think so.

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Michal Luczaj 2 years, 3 months ago

On 6/2/23 18:56, Sean Christopherson wrote:
> On Fri, Jun 02, 2023, Michal Luczaj wrote:
>> I guess this makes the !! in kvm_vm_ioctl_create_vcpu() unnecessary:
>>
>> KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0)...
> 
> Ya, I saw that, which in addition to Wei's ping, is what reminded me that the
> KVM_BUG_ON() fix hadn't been merged.
> 
>> Is it worth a patch (perhaps along with chopping off !! in
>> kvm_msr_allowed() and few other places)?
> 
> Yes, I think so.

OK, so xa_store() aside[*], I see some bool-to-bools:

arch/x86/kvm/x86.c:
	kvm_msr_allowed():allowed = !!test_bit(index - start, bitmap);
arch/x86/kvm/hyperv.c:
	kvm_hv_hypercall():hc.rep = !!(hc.rep_cnt || hc.rep_idx);
arch/x86/kvm/mmu/mmu.c:
	update_pkru_bitmask():
		pkey_bits = !!check_pkey;
		pkey_bits |= (!!check_write) << 1;
arch/x86/kvm/svm/svm.c:
	msr_write_intercepted():return !!test_bit(bit_write,  &tmp);
	svm_vcpu_after_set_cpuid():
		2x set_msr_interception...
tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c:
	set_or_clear_invalid_guest_state():sregs.tr.unusable = !!set;

But perhaps this is a matter of style and those were meant to be this kind-of
explicit?

[*] https://lore.kernel.org/kvm/20230605114852.288964-1-mhal@rbox.co/

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Sean Christopherson 2 years, 3 months ago

On Mon, Jun 05, 2023, Michal Luczaj wrote:
> On 6/2/23 18:56, Sean Christopherson wrote:
> > On Fri, Jun 02, 2023, Michal Luczaj wrote:
> >> I guess this makes the !! in kvm_vm_ioctl_create_vcpu() unnecessary:
> >>
> >> KVM_BUG_ON(!!xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0)...
> > 
> > Ya, I saw that, which in addition to Wei's ping, is what reminded me that the
> > KVM_BUG_ON() fix hadn't been merged.
> > 
> >> Is it worth a patch (perhaps along with chopping off !! in
> >> kvm_msr_allowed() and few other places)?
> > 
> > Yes, I think so.
> 
> OK, so xa_store() aside[*], I see some bool-to-bools:
> 
> arch/x86/kvm/x86.c:
> 	kvm_msr_allowed():allowed = !!test_bit(index - start, bitmap);
> arch/x86/kvm/hyperv.c:
> 	kvm_hv_hypercall():hc.rep = !!(hc.rep_cnt || hc.rep_idx);
> arch/x86/kvm/mmu/mmu.c:
> 	update_pkru_bitmask():
> 		pkey_bits = !!check_pkey;
> 		pkey_bits |= (!!check_write) << 1;
> arch/x86/kvm/svm/svm.c:
> 	msr_write_intercepted():return !!test_bit(bit_write,  &tmp);
> 	svm_vcpu_after_set_cpuid():
> 		2x set_msr_interception...
> tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c:
> 	set_or_clear_invalid_guest_state():sregs.tr.unusable = !!set;
> 
> But perhaps this is a matter of style and those were meant to be this kind-of
> explicit?

I doubt it, I'm guessing most cases are due to the author being overzealous for
one reason or another, e.g. I suspect the test_bit() ones are due to the original
author incorrectly assuming test_bit() returned an unsigned long, i.e. the bit,
as opposed to the bool.

If you want to clean these up, I'd say "fix" the test_bit() cases, but leave the
others alone.  The test_bit() ones are clearly redundant, and IMO can be actively
due to implying test_bit() returns something other than a bool.

> [*] https://lore.kernel.org/kvm/20230605114852.288964-1-mhal@rbox.co/

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Michal Luczaj 2 years, 3 months ago

On 6/5/23 17:19, Sean Christopherson wrote:
> On Mon, Jun 05, 2023, Michal Luczaj wrote:
>> OK, so xa_store() aside[*], I see some bool-to-bools:
>>
>> arch/x86/kvm/x86.c:
>> 	kvm_msr_allowed():allowed = !!test_bit(index - start, bitmap);
>> arch/x86/kvm/hyperv.c:
>> 	kvm_hv_hypercall():hc.rep = !!(hc.rep_cnt || hc.rep_idx);
>> arch/x86/kvm/mmu/mmu.c:
>> 	update_pkru_bitmask():
>> 		pkey_bits = !!check_pkey;
>> 		pkey_bits |= (!!check_write) << 1;
>> arch/x86/kvm/svm/svm.c:
>> 	msr_write_intercepted():return !!test_bit(bit_write,  &tmp);
>> 	svm_vcpu_after_set_cpuid():
>> 		2x set_msr_interception...
>> tools/testing/selftests/kvm/x86_64/vmx_exception_with_invalid_guest_state.c:
>> 	set_or_clear_invalid_guest_state():sregs.tr.unusable = !!set;
>>
>> But perhaps this is a matter of style and those were meant to be this kind-of
>> explicit?
> 
> I doubt it, I'm guessing most cases are due to the author being overzealous for
> one reason or another, e.g. I suspect the test_bit() ones are due to the original
> author incorrectly assuming test_bit() returned an unsigned long, i.e. the bit,
> as opposed to the bool.
> 
> If you want to clean these up, I'd say "fix" the test_bit() cases, but leave the
> others alone.  The test_bit() ones are clearly redundant, and IMO can be actively
> due to implying test_bit() returns something other than a bool.

Done: https://lore.kernel.org/kvm/20230605200158.118109-1-mhal@rbox.co/

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Sean Christopherson 2 years, 6 months ago

On Tue, Mar 07, 2023, Wei Wang wrote:
> Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from callers is
> 32-bit as it casts 'cond' to the type of int.

You're very generous, I would have led with "Fix a badly done copy+paste ..." ;-)

> Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

RE: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Wang, Wei W 2 years, 3 months ago

On Thursday, March 9, 2023 7:26 AM, Sean Christopherson wrote:
> On Tue, Mar 07, 2023, Wei Wang wrote:
> > Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from
> callers
> > is 32-bit as it casts 'cond' to the type of int.
> 
> You're very generous, I would have led with "Fix a badly done
> copy+paste ..." ;-)
> 
> > Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as
> > bugged")
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > ---
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com>

Kind ping on this patch.
Seems it wasn't noticed for months. Just check if it would be good to be
merged or not proper for any reason?

Thanks,
Wei

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Sean Christopherson 2 years, 3 months ago

On Thu, Jun 01, 2023, Wei W Wang wrote:
> On Thursday, March 9, 2023 7:26 AM, Sean Christopherson wrote:
> > On Tue, Mar 07, 2023, Wei Wang wrote:
> > > Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from
> > callers
> > > is 32-bit as it casts 'cond' to the type of int.
> > 
> > You're very generous, I would have led with "Fix a badly done
> > copy+paste ..." ;-)
> > 
> > > Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as
> > > bugged")
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > ---
> > 
> > Reviewed-by: Sean Christopherson <seanjc@google.com>
> 
> Kind ping on this patch.
> Seems it wasn't noticed for months. Just check if it would be good to be
> merged or not proper for any reason?

I'll grab it for 6.5.

RE: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Wang, Wei W 2 years, 3 months ago

On Friday, June 2, 2023 12:21 AM, Sean Christopherson wrote:
> On Thu, Jun 01, 2023, Wei W Wang wrote:
> > On Thursday, March 9, 2023 7:26 AM, Sean Christopherson wrote:
> > > On Tue, Mar 07, 2023, Wei Wang wrote:
> > > > Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from
> > > callers
> > > > is 32-bit as it casts 'cond' to the type of int.
> > >
> > > You're very generous, I would have led with "Fix a badly done
> > > copy+paste ..." ;-)
> > >
> > > > Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM
> > > > as
> > > > bugged")
> > > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > > ---
> > >
> > > Reviewed-by: Sean Christopherson <seanjc@google.com>
> >
> > Kind ping on this patch.
> > Seems it wasn't noticed for months. Just check if it would be good to
> > be merged or not proper for any reason?
> 
> I'll grab it for 6.5.
	
OK, thanks. Please check if these two are ready to go into 6.5 if possible:
https://lore.kernel.org/kvm/20230315101606.10636-1-wei.w.wang@intel.com/
https://lore.kernel.org/kvm/20230207123713.3905-1-wei.w.wang@intel.com/

Re: [PATCH v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

Posted by Mingwei Zhang 2 years, 6 months ago

On Tue, Mar 7, 2023 at 5:52 AM Wei Wang <wei.w.wang@intel.com> wrote:
>
> Current KVM_BUG and KVM_BUG_ON assume that 'cond' passed from callers is
> 32-bit as it casts 'cond' to the type of int. This will be wrong if 'cond'
> provided by a caller is 64-bit, e.g. an error code of 0xc0000d0300000000
> will be converted to 0, which is not expected.
>
> Improves the implementation by using bool in KVM_BUG and KVM_BUG_ON.
> 'bool' is preferred to 'int' as __ret is essentially used as a boolean
> and coding-stytle.rst documents that use of bool is encouraged to improve
> readability and is often a better option than 'int' for storing boolean
> values.
>
> Fixes: 0b8f11737cff ("KVM: Add infrastructure and macro to mark VM as bugged")
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
Reviewed-by: Mingwei Zhang <mizhang@google.com>