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

  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(-)
  

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>
  

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>
  

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
  

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.
  

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/
  

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
  

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)?
  

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.
  

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/
  

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/
  

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/