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: 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>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ