linux-kernel - Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to pre-populate guest memory

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]

Message-ID: <20240307204149.GJ368614@ls.amr.corp.intel.com>
Date: Thu, 7 Mar 2024 12:41:49 -0800
From: Isaku Yamahata <isaku.yamahata@...ux.intel.com>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"federico.parola@...ito.it" <federico.parola@...ito.it>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>,
	"dmatlack@...gle.com" <dmatlack@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"michael.roth@....com" <michael.roth@....com>,
	"seanjc@...gle.com" <seanjc@...gle.com>,
	isaku.yamahata@...ux.intel.com
Subject: Re: [RFC PATCH 2/8] KVM: Add KVM_MAP_MEMORY vcpu ioctl to
 pre-populate guest memory

On Thu, Mar 07, 2024 at 12:45:16PM +0000,
"Huang, Kai" <kai.huang@...el.com> wrote:

> 
> >  
> > +int kvm_arch_vcpu_pre_map_memory(struct kvm_vcpu *vcpu);
> 
> No explanation of why this is needed, and why it only takes @vcpu as input w/o
> having the @mapping.
>  
> > +int kvm_arch_vcpu_map_memory(struct kvm_vcpu *vcpu,
> > +			     struct kvm_memory_mapping *mapping);
> > +
> > 
> 
> [...]
> 
> > +static int kvm_vcpu_map_memory(struct kvm_vcpu *vcpu,
> > +			       struct kvm_memory_mapping *mapping)
> > +{
> > +	bool added = false;
> > +	int idx, r = 0;
> > +
> > +	if (mapping->flags & ~(KVM_MEMORY_MAPPING_FLAG_WRITE |
> > +			       KVM_MEMORY_MAPPING_FLAG_EXEC |
> > +			       KVM_MEMORY_MAPPING_FLAG_USER |
> > +			       KVM_MEMORY_MAPPING_FLAG_PRIVATE))
> > +		return -EINVAL;
> > +	if ((mapping->flags & KVM_MEMORY_MAPPING_FLAG_PRIVATE) &&
> > +	    !kvm_arch_has_private_mem(vcpu->kvm))
> > +		return -EINVAL;
> > +
> > +	/* Sanity check */
> > +	if (!IS_ALIGNED(mapping->source, PAGE_SIZE) ||
> > +	    !mapping->nr_pages ||
> > +	    mapping->base_gfn + mapping->nr_pages <= mapping->base_gfn)
> > +		return -EINVAL;
> > +
> > +	vcpu_load(vcpu);
> > +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> > +	r = kvm_arch_vcpu_pre_map_memory(vcpu);
> > +	if (r)
> > +		return r;
> 
> Returning w/o unloading the vcpu and releasing the SRCU.

Oos, Will fix.


> > +
> > +	while (mapping->nr_pages) {
> > +		if (signal_pending(current)) {
> > +			r = -ERESTARTSYS;
> > +			break;
> > +		}
> > +
> > +		if (need_resched())
> > +			cond_resched();
> 
> need_resched() is not needed.
> 
> And normally I think we just put it at the end of the loop.

Ok, will move it.


> > +
> > +		r = kvm_arch_vcpu_map_memory(vcpu, mapping);
> > +		if (r)
> > +			break;
> > +
> > +		added = true;
> > +	}
> > +
> > +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> > +	vcpu_put(vcpu);
> > +
> > +	if (added && mapping->nr_pages > 0)
> > +		r = -EAGAIN;
> 
> Why do we need @added?
> 
> I assume the kvm_arch_vcpu_map_memory() can internally update the mapping-
> >nr_pages but still return -E<WHATEVER>.  So when that happens in the first call
> of kvm_arch_vcpu_map_memory(), @added won't get chance to turn to true.

I intend to tell the caller if the range is partially processed or not.
Anyway this seems moot. Let's drop this if clause.  Then it's caller's
responsibility to check error and partial conversion and to optionally loop
with the remaining region.
-- 
Isaku Yamahata <isaku.yamahata@...ux.intel.com>