| Message ID | 20230307135233.54684-1-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond | expand |
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/
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); \
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(-)