linux-kernel - Re: [PATCH 1/1] KVM: s390: index kvm->arch.idle_mask by vcpu

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, 27 Aug 2021 23:19:21 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Claudio Imbrenda <imbrenda@...ux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@...ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>, kvm@...r.kernel.org,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Michael Mueller <mimu@...ux.ibm.com>,
        Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [PATCH 1/1] KVM: s390: index kvm->arch.idle_mask by vcpu_idx

On Fri, 27 Aug 2021 16:06:16 +0200
Claudio Imbrenda <imbrenda@...ux.ibm.com> wrote:

> On Fri, 27 Aug 2021 14:54:29 +0200
> Halil Pasic <pasic@...ux.ibm.com> wrote:
> 
> > While in practice vcpu->vcpu_idx ==  vcpu->vcp_id is often true,

s/vcp_id/vcpu_id/

> > it may not always be, and we must not rely on this.  
> 
> why?
> 
> maybe add a simple explanation of why vcpu_idx and vcpu_id can be
> different, namely:
> KVM decides the vcpu_idx, userspace decides the vcpu_id, thus the two
> might not match 

Not sure that is a good explanation. A quote from
Documentation/virt/kvm/api.rst:

"""
4.7 KVM_CREATE_VCPU
-------------------

:Capability: basic
:Architectures: all
:Type: vm ioctl
:Parameters: vcpu id (apic id on x86)
:Returns: vcpu fd on success, -1 on error

This API adds a vcpu to a virtual machine. No more than max_vcpus may be added.
The vcpu id is an integer in the range [0, max_vcpu_id).

The recommended max_vcpus value can be retrieved using the KVM_CAP_NR_VCPUS of
the KVM_CHECK_EXTENSION ioctl() at run-time.
The maximum possible value for max_vcpus can be retrieved using the
KVM_CAP_MAX_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time.
"""

Based on that and a quick look at the code, it looks to me like the
set of valid vcpu_id values are a subset of the range of vcpu_idx-es,
i.e. that kvm could decide to choose vcpu_id for the value of vcpu_idx.
I don't think it should, but it could. Were the set of valid vcpu_id
values not a subset of the set of supported vcpu_idx values, then one
could argue that this is why.

I didn't want to get into explaining the why, I just wanted to state the
fact.

> 
> > 
> > Currently kvm->arch.idle_mask is indexed by vcpu_id, which implies
> > that code like
> > for_each_set_bit(vcpu_id, kvm->arch.idle_mask, online_vcpus) {
> >                 vcpu = kvm_get_vcpu(kvm, vcpu_id);  
> 
> you can also add a sentence to clarify that kvm_get_vcpu expects an
> vcpu_idx, not an vcpu_id.

maybe ...

> 
> > 		do_stuff(vcpu);
> > }
> > is not legit. The trouble is, we do actually use kvm->arch.idle_mask

... s/legit\./legit, because kvm_get_vcpu() expects a vcpu_idx and not a
vcpu_id.

But I agree kvm_get_vcpu(kvm, vcpu_id); does not scream BUG at me while
kvm_get_vcpu_by_idx(kvm, vcpu_id) would look much more suspicious.

[..]
> 
> otherwise looks good to me.
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@...ux.ibm.com>

Thanks for your reveiew!

Halil

[..]