Re: [PATCH 2/2] KVM, Fix QEMU-KVM is killed by guest SRAO MCE

Re: [PATCH 2/2] KVM, Fix QEMU-KVM is killed by guest SRAO MCE

[Huang Ying]

On Tue, 2010-04-27 at 15:47 +0800, Avi Kivity wrote:
> (please copy kvm@vger.kernel.org on kvm patches)

Sorry, will do that for all future patches.

> On 04/27/2010 10:04 AM, Huang Ying wrote:
> >
> > +static void kvm_send_hwpoison_signal(struct kvm *kvm, gfn_t gfn)
> > +{
> > +	char buf[1];
> > +	void __user *hva;
> > +	int r;
> > +
> > +	/* Touch the page, so send SIGBUS */
> > +	hva = (void __user *)gfn_to_hva(kvm, gfn);
> > +	r = copy_from_user(buf, hva, 1);
> >    
> 
> No error check?  What will a copy_from_user() of poisoned page expected 
> to return?
> 
> Best to return -EFAULT on failure for consistency.

Just want to use the side effect of copy_from_user, SIGBUS will be sent
to current process because the page touched is marked as poisoned. That
is, failure is expected, so the return value is not checked.

> > +}
> > +
> >   static int nonpaging_map(struct kvm_vcpu *vcpu, gva_t v, int write, gfn_t gfn)
> >   {
> >   	int r;
> > @@ -1997,7 +2009,11 @@ static int nonpaging_map(struct kvm_vcpu
> >   	/* mmio */
> >   	if (is_error_pfn(pfn)) {
> >   		kvm_release_pfn_clean(pfn);
> > -		return 1;
> > +		if (is_hwpoison_pfn(pfn)) {
> > +			kvm_send_hwpoison_signal(vcpu->kvm, gfn);
> > +			return 0;
> > +		} else
> > +			return 1;
> >   	}
> >    
> 
> This is duplicated several times.  Please introduce a kvm_handle_bad_page():
> 
>      if (is_error_pfn(pfn))
>          return kvm_handle_bad_page(vcpu->kvm, gfn, pfn);

OK. Will do that.

Best Regards,
Huang Ying

Re: [PATCH 2/2] KVM, Fix QEMU-KVM is killed by guest SRAO MCE

[Avi Kivity]

On 04/27/2010 12:25 PM, Huang Ying wrote:
>
>
>> On 04/27/2010 10:04 AM, Huang Ying wrote:
>>      
>>> +static void kvm_send_hwpoison_signal(struct kvm *kvm, gfn_t gfn)
>>> +{
>>> +	char buf[1];
>>> +	void __user *hva;
>>> +	int r;
>>> +
>>> +	/* Touch the page, so send SIGBUS */
>>> +	hva = (void __user *)gfn_to_hva(kvm, gfn);
>>> +	r = copy_from_user(buf, hva, 1);
>>>
>>>        
>> No error check?  What will a copy_from_user() of poisoned page expected
>> to return?
>>
>> Best to return -EFAULT on failure for consistency.
>>      
> Just want to use the side effect of copy_from_user, SIGBUS will be sent
> to current process because the page touched is marked as poisoned. That
> is, failure is expected, so the return value is not checked.
>    

What if the failure doesn't happen?  Say, someone mmap()ed over the page.

btw, better to use (void)copy_from_user(...) instead to avoid the 
initialized but not used warning the compiler may generate.


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

Re: [PATCH 2/2] KVM, Fix QEMU-KVM is killed by guest SRAO MCE

[Huang Ying]

On Tue, 2010-04-27 at 17:30 +0800, Avi Kivity wrote:
> On 04/27/2010 12:25 PM, Huang Ying wrote:
> >
> >
> >> On 04/27/2010 10:04 AM, Huang Ying wrote:
> >>      
> >>> +static void kvm_send_hwpoison_signal(struct kvm *kvm, gfn_t gfn)
> >>> +{
> >>> +	char buf[1];
> >>> +	void __user *hva;
> >>> +	int r;
> >>> +
> >>> +	/* Touch the page, so send SIGBUS */
> >>> +	hva = (void __user *)gfn_to_hva(kvm, gfn);
> >>> +	r = copy_from_user(buf, hva, 1);
> >>>
> >>>        
> >> No error check?  What will a copy_from_user() of poisoned page expected
> >> to return?
> >>
> >> Best to return -EFAULT on failure for consistency.
> >>      
> > Just want to use the side effect of copy_from_user, SIGBUS will be sent
> > to current process because the page touched is marked as poisoned. That
> > is, failure is expected, so the return value is not checked.
> >    
> 
> What if the failure doesn't happen?  Say, someone mmap()ed over the page.

Sorry, not get your idea clearly. hva is re-mmap()ed? We just read the
hva, not write, so I think it should be OK here.

> btw, better to use (void)copy_from_user(...) instead to avoid the 
> initialized but not used warning the compiler may generate.

OK. Will do that.

Best Regards,
Huang Ying

Re: [PATCH 2/2] KVM, Fix QEMU-KVM is killed by guest SRAO MCE

[Avi Kivity]

On 04/28/2010 05:56 AM, Huang Ying wrote:
>    
>>>
>>> Just want to use the side effect of copy_from_user, SIGBUS will be sent
>>> to current process because the page touched is marked as poisoned. That
>>> is, failure is expected, so the return value is not checked.
>>>
>>>        
>> What if the failure doesn't happen?  Say, someone mmap()ed over the page.
>>      
> Sorry, not get your idea clearly. hva is re-mmap()ed? We just read the
> hva, not write, so I think it should be OK here.
>
>    

We don't generate a signal in this case.  Does the code continue to work 
correctly (not sure what correctly is in this case... should probably 
just continue).

There's also the possibility of -EFAULT.

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

Re: [PATCH 2/2] KVM, Fix QEMU-KVM is killed by guest SRAO MCE

[Huang Ying]

On Wed, 2010-04-28 at 17:47 +0800, Avi Kivity wrote:
> On 04/28/2010 05:56 AM, Huang Ying wrote:
> >    
> >>>
> >>> Just want to use the side effect of copy_from_user, SIGBUS will be sent
> >>> to current process because the page touched is marked as poisoned. That
> >>> is, failure is expected, so the return value is not checked.
> >>>
> >>>        
> >> What if the failure doesn't happen?  Say, someone mmap()ed over the page.
> >>      
> > Sorry, not get your idea clearly. hva is re-mmap()ed? We just read the
> > hva, not write, so I think it should be OK here.
> >
> >    
> 
> We don't generate a signal in this case.  Does the code continue to work 
> correctly (not sure what correctly is in this case... should probably 
> just continue).
> 
> There's also the possibility of -EFAULT.

I think signal should be generated for copy_from_user, because the hva
is poisoned now. The signal will not generated only if the hva is
re-mmap()ped to some other physical page, but this should be impossible
unless we have memory hotadd/hotremove in KVM.

If the signal is not generated, lost or overwritten, guest will
continue, and if the hva is still poisoned, the page fault will be
triggered again; if the hva is not poisoned, there will be no further
page fault.

Best Regards,
Huang Ying