linux-kernel - Re: [PATCH 0/4] KVM: add KVM_CREATE_VM2 to allow dynamic kvm->vcpus array

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Apr 2017 22:03:46 +0200
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     Cornelia Huck <cornelia.huck@...ibm.com>
Cc:     David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, Christoffer Dall <cdall@...aro.org>,
        Marc Zyngier <marc.zyngier@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        James Hogan <james.hogan@...tec.com>,
        Paul Mackerras <paulus@...abs.org>,
        Alexander Graf <agraf@...e.com>
Subject: Re: [PATCH 0/4] KVM: add KVM_CREATE_VM2 to allow dynamic kvm->vcpus
 array

2017-04-18 14:29+0200, Cornelia Huck:
> On Tue, 18 Apr 2017 13:11:55 +0200
> David Hildenbrand <david@...hat.com> wrote:
>> On 13.04.2017 22:19, Radim Krčmář wrote:
>> > new KVM_MAX_CONFIGURABLE_VCPUS, probably directly to INT_MAX/KVM_VCPU_ID, so we
>> > don't have to worry about it for a while.
>> > 
>> > PPC should be interested in this as they set KVM_MAX_VCPUS to NR_CPUS
>> > and probably waste few pages for every guest this way.
>> 
>> As we just store pointers, this should be a maximum of 4 pages for ppc
>> (4k pages). Is this really worth yet another VM creation ioctl? Is there
>> not a nicer way to handle this internally?
>> 
>> An alternative might be to simply realloc the array when it reaches a
>> certain size (on VCPU creation, maybe protecting the pointer via rcu).
>> But not sure if something like that could work.
> 
> I like that idea better, if it does work (I think it should be doable).
> If we just double the array size every time we run out of space, we
> should be able to do this with few reallocations. That has also the
> advantage of being transparent to user space (other than increased
> number of vcpus).

Yes, relocating would work with protection against use-after-free and
RCU fits well.  (Readers don't have any lock we could piggyback on.)

I didn't go for it because of C: the kvm_for_each_vcpu macro would be
less robust if it included the locking around its body -- nested fors are
susceptible to return/goto errors inside the loop body + we'd need to
obfuscate several existing users of that pattern.  And open-coding the
protection everywhere is polluting the code too, IMO.

Lock-less list would solve those problems, but we are accessing the
VCPUs by index, which makes it suboptimal in other direction ... using
the list for kvm_for_each_vcpu and adding RCU protected array for
kvm_get_vcpu and kvm_get_vcpu_by_id looks like over-engineering as we
wouldn't save memory, performance, nor lines of code by doing that.

I didn't see a way to untangle kvm->vcpu that would allow a nice
runtime-dynamic variant.

We currently don't need to pass more information at VM creation time
either, so I was also thinking of hijacking the parameter to
KVM_CREATE_VM for factor-of-2 VCPU count (20 bits would last a while),
but that is already a new interface and new IOCTL to do a superset of
another one seemed much better.

I agree that the idea is questionable.   I'll redo the series and bump
KVM_MAX_VCPUS unless you think that the dynamic could be done nicely.

(The memory saving is a miniscule fraction of a VM size and if we do big
 increments in KVM_MAX_VCPUS, then the motivation is gone.)

Thanks.