[PATCH] KVM: Veirfy memory slot only for readability

[PATCH] KVM: Veirfy memory slot only for readability

[Sasha Levin]

It's enough for memory slot to be readable, as the comment above the check
states.

A user should be able to create read-only memory slot.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 virt/kvm/kvm_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e289486..b92883f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -727,7 +727,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	/* We can read the guest memory with __xxx_user() later on. */
 	if (user_alloc &&
 	    ((mem->userspace_addr & (PAGE_SIZE - 1)) ||
-	     !access_ok(VERIFY_WRITE,
+	     !access_ok(VERIFY_READ,
 			(void __user *)(unsigned long)mem->userspace_addr,
 			mem->memory_size)))
 		goto out;
-- 
1.7.8.rc4

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Takuya Yoshikawa]

(2011/12/02 4:42), Sasha Levin wrote:
> It's enough for memory slot to be readable, as the comment above the check
> states.
> 
> A user should be able to create read-only memory slot.

I submitted the original patch like you to speed up page table walking,
a hot path in KVM, and Avi applied the patch with changing the VERIFY_READ
to VERIFY_WRITE:  on x86, both do the same check.  You can see that on the
commit.

After that, Xiao started to write with __xxx_user() based on this check IIRC.
So you should keep the code as is and change the comment if you like!

Thanks,
	Takuya

> 
> Cc: Avi Kivity<avi@redhat.com>
> Cc: Marcelo Tosatti<mtosatti@redhat.com>
> Cc: kvm@vger.kernel.org
> Signed-off-by: Sasha Levin<levinsasha928@gmail.com>
> ---
>   virt/kvm/kvm_main.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e289486..b92883f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -727,7 +727,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
>   	/* We can read the guest memory with __xxx_user() later on. */
>   	if (user_alloc&&
>   	((mem->userspace_addr&  (PAGE_SIZE - 1)) ||
> -	     !access_ok(VERIFY_WRITE,
> +	     !access_ok(VERIFY_READ,
>   			(void __user *)(unsigned long)mem->userspace_addr,
>   			mem->memory_size)))
>   		goto out;

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Takuya Yoshikawa]

(2011/12/02 10:15), Takuya Yoshikawa wrote:
> (2011/12/02 4:42), Sasha Levin wrote:
>> It's enough for memory slot to be readable, as the comment above the check
>> states.
>>
>> A user should be able to create read-only memory slot.

Note: at that time, it looked to me that the API did not allow me to know
which type was being registered.

Do you want to create read only memory slots for kvm tool?

	Takuya

> 
> I submitted the original patch like you to speed up page table walking,
> a hot path in KVM, and Avi applied the patch with changing the VERIFY_READ
> to VERIFY_WRITE:  on x86, both do the same check.  You can see that on the
> commit.
> 
> After that, Xiao started to write with __xxx_user() based on this check IIRC.
> So you should keep the code as is and change the comment if you like!
> 
> Thanks,
> 	Takuya
> 

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Sasha Levin]

On Fri, 2011-12-02 at 12:16 +0900, Takuya Yoshikawa wrote:
> (2011/12/02 10:15), Takuya Yoshikawa wrote:
> > (2011/12/02 4:42), Sasha Levin wrote:
> >> It's enough for memory slot to be readable, as the comment above the check
> >> states.
> >>
> >> A user should be able to create read-only memory slot.
> 
> Note: at that time, it looked to me that the API did not allow me to know
> which type was being registered.

Which code exactly assumes that all memory slots are writable?

> Do you want to create read only memory slots for kvm tool?

What KVM tool currently does is copy the kernel into guest memory and
run it from there. An idea raised recently was instead of copying it we
should mmap it into the memory to reduce footprint.

This is why I'm looking into adding a read only memory slot. The KVM
code doesn't have to know it's read only.

-- 

Sasha.

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Takuya Yoshikawa]

(2011/12/02 14:46), Sasha Levin wrote:
> On Fri, 2011-12-02 at 12:16 +0900, Takuya Yoshikawa wrote:
>> (2011/12/02 10:15), Takuya Yoshikawa wrote:
>>> (2011/12/02 4:42), Sasha Levin wrote:
>>>> It's enough for memory slot to be readable, as the comment above the check
>>>> states.
>>>>
>>>> A user should be able to create read-only memory slot.
>>
>> Note: at that time, it looked to me that the API did not allow me to know
>> which type was being registered.
>
> Which code exactly assumes that all memory slots are writable?

I agree with you, but as I wrote, my patch only used the check for
read with __xxx_user() because I had not thought changing the code
for write, not hot compared to read, would not worth it.

But access_ok() does the same for the two cases, so I did not mind
changing VERIFY_READ to VERIFY_WRITE: you should not have any problem
to do what you want, no?

I am not mentioning whether it is good as a programming style, here.

>
>> Do you want to create read only memory slots for kvm tool?
>
> What KVM tool currently does is copy the kernel into guest memory and
> run it from there. An idea raised recently was instead of copying it we
> should mmap it into the memory to reduce footprint.

Interesting.

>
> This is why I'm looking into adding a read only memory slot. The KVM
> code doesn't have to know it's read only.
>

What I wanted to say was that, it might be cleaner if I could put the
exactly needed flag only.

And if you want to change the VERIFY_WRITE to VERIFY_READ, please check
the following commit as well:

	commit ccddff20b9368cce9b774f6a22440a7c45367784
	Author: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>

	KVM: use __copy_to_user/__clear_user to write guest page

Ah, and the code is not x86 specific, so you may better check other
architectures as well.

Anyway, adding some more descriptions based on the history will be nice.

	Takuya

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Avi Kivity]

On 12/02/2011 07:46 AM, Sasha Levin wrote:
> > Do you want to create read only memory slots for kvm tool?
>
> What KVM tool currently does is copy the kernel into guest memory and
> run it from there. An idea raised recently was instead of copying it we
> should mmap it into the memory to reduce footprint.
>
> This is why I'm looking into adding a read only memory slot. The KVM
> code doesn't have to know it's read only.

The kernel will patch itself very early.  You need to use MAP_PRIVATE
(and thus have a read/write area).  It will be interesting to see what
fraction of the memory is modified.

Note that mapping will remove benefits like huge page support, and that
you can get page sharing by using ksm.  Still, it's interesting to see
where this goes.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Sasha Levin]

On Sun, 2011-12-04 at 18:53 +0200, Avi Kivity wrote:
> On 12/02/2011 07:46 AM, Sasha Levin wrote:
> > > Do you want to create read only memory slots for kvm tool?
> >
> > What KVM tool currently does is copy the kernel into guest memory and
> > run it from there. An idea raised recently was instead of copying it we
> > should mmap it into the memory to reduce footprint.
> >
> > This is why I'm looking into adding a read only memory slot. The KVM
> > code doesn't have to know it's read only.
> 
> The kernel will patch itself very early.  You need to use MAP_PRIVATE
> (and thus have a read/write area).  It will be interesting to see what
> fraction of the memory is modified.
> 
> Note that mapping will remove benefits like huge page support, and that
> you can get page sharing by using ksm.  Still, it's interesting to see
> where this goes.

Why would I lose hugepage if the kernel gets it's own memory slot?

-- 

Sasha.

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Avi Kivity]

On 12/04/2011 07:29 PM, Sasha Levin wrote:
> On Sun, 2011-12-04 at 18:53 +0200, Avi Kivity wrote:
> > On 12/02/2011 07:46 AM, Sasha Levin wrote:
> > > > Do you want to create read only memory slots for kvm tool?
> > >
> > > What KVM tool currently does is copy the kernel into guest memory and
> > > run it from there. An idea raised recently was instead of copying it we
> > > should mmap it into the memory to reduce footprint.
> > >
> > > This is why I'm looking into adding a read only memory slot. The KVM
> > > code doesn't have to know it's read only.
> > 
> > The kernel will patch itself very early.  You need to use MAP_PRIVATE
> > (and thus have a read/write area).  It will be interesting to see what
> > fraction of the memory is modified.
> > 
> > Note that mapping will remove benefits like huge page support, and that
> > you can get page sharing by using ksm.  Still, it's interesting to see
> > where this goes.
>
> Why would I lose hugepage if the kernel gets it's own memory slot?

(transparent) hugepages only work on anonymous memory.  Hopefully later
it will be extended to work on mapped memory as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Veirfy memory slot only for readability

[Sasha Levin]

On Sun, 2011-12-04 at 19:34 +0200, Avi Kivity wrote:
> On 12/04/2011 07:29 PM, Sasha Levin wrote:
> > On Sun, 2011-12-04 at 18:53 +0200, Avi Kivity wrote:
> > > On 12/02/2011 07:46 AM, Sasha Levin wrote:
> > > > > Do you want to create read only memory slots for kvm tool?
> > > >
> > > > What KVM tool currently does is copy the kernel into guest memory and
> > > > run it from there. An idea raised recently was instead of copying it we
> > > > should mmap it into the memory to reduce footprint.
> > > >
> > > > This is why I'm looking into adding a read only memory slot. The KVM
> > > > code doesn't have to know it's read only.
> > > 
> > > The kernel will patch itself very early.  You need to use MAP_PRIVATE
> > > (and thus have a read/write area).  It will be interesting to see what
> > > fraction of the memory is modified.
> > > 
> > > Note that mapping will remove benefits like huge page support, and that
> > > you can get page sharing by using ksm.  Still, it's interesting to see
> > > where this goes.
> >
> > Why would I lose hugepage if the kernel gets it's own memory slot?
> 
> (transparent) hugepages only work on anonymous memory.  Hopefully later
> it will be extended to work on mapped memory as well.

So I'll lose hugepages just for the kernel memory slot, I wonder how it
will matter performance-wise.

-- 

Sasha.