linux-kernel - RE: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Mar 2023 01:49:53 +0000
From:   "Wang, Wei W" <wei.w.wang@...el.com>
To:     Mingwei Zhang <mizhang@...gle.com>
CC:     David Matlack <dmatlack@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1] KVM: allow KVM_BUG/KVM_BUG_ON to handle 64-bit cond

On Friday, March 3, 2023 2:12 AM, Mingwei Zhang wrote:
> > On Thursday, March 2, 2023 12:55 PM, Mingwei Zhang wrote:
> > > I don't get it. Why bothering the type if we just do this?
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index 4f26b244f6d0..10455253c6ea 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -848,7 +848,7 @@ static inline void kvm_vm_bugged(struct kvm
> > > *kvm)
> > >
> > >  #define KVM_BUG(cond, kvm, fmt...)                           \
> > >  ({                                                           \
> > > -     int __ret = (cond);                                     \
> > > +     int __ret = !!(cond);                                   \
> >
> > This is essentially "bool __ret". No biggie to change it this way.
> 
> !! will return an int, not a boolean, but it is used as a boolean.

What's the point of defining it as an int when actually being used as a Boolean?
Original returning of an 'int' is a bug in this sense. Either returning a Boolean or
the same type (length) as cond is good way to me.

> This is consistent with the original code which _is_ returning an integer.
> 
> > But I'm inclined to retain the original intention to have the macro
> > return the value that was passed in:
> > typeof(cond) __ret = (cond);
> 
> hmm, I think it is appropriate to retain the original type of 'cond'
> especially since it may also involve other arithmetic operations. But I doubt it
> will be very useful. For instance, who is going to write this code?
> 

Maybe there is, maybe not. But it doesn’t hurt anything to leave the
flexibility there using typeof(cond). As said, I'm also fine to use 'bool ret',
but probably not 'int' for no good reason.