Re: -EINTR return in domain_relinquish_resources

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Jan 23, 2015 at 03:34:51PM +0000, Jan Beulich wrote:
> >>> On 23.01.15 at 15:46, <andrew.cooper3@citrix.com> wrote:
> > On 23/01/15 14:32, Konrad Rzeszutek Wilk wrote:
> >> On Fri, Jan 23, 2015 at 09:11:25AM +0000, Jan Beulich wrote:
> >>>>>> On 22.01.15 at 17:38, <konrad.wilk@oracle.com> wrote:
> >>>> On Thu, Jan 22, 2015 at 10:00:35AM +0000, Jan Beulich wrote:
> >>>>>>>> On 21.01.15 at 22:27, <konrad.wilk@oracle.com> wrote:
> >>>>>> As I was looking at some of the XSA I realized that the
> >>>>>> call-chain of:
> >>>>>>
> >>>>>>  domain_relinquish_resources
> >>>>>>    ->vcpu_destroy_pagetables
> >>>>>>      -> put_page_and_type_preemptible
> >>>>>>         -> __put_page_type
> >>>>>> 		returns -EINTR
> >>>>>> [...]
> >>>>>>  b). Or should the hypervisor convert the -EINTR to -ERESTART
> >>>>>>      (or -EAGAIN) - which most of the code (see users of
> >>>>>>      get_page_type_preemptible) do right now?
> >>>>> This one, in vcpu_destroy_pagetables(). I'm afraid I overlooked
> >>>>> this when adding the preemption capability here.
> >>>> OK. Would this RFC patch be OK? (I can send it off as normal - just
> >>>> want to make sure you are OK with this being put there)
> >>> Conceptually yes, but there are issues:
> >>>
> >>>> From 1558a9870f438df949a8ad09a27825bd35a9f4ea Mon Sep 17 00:00:00 2001
> >>>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>>> Date: Thu, 22 Jan 2015 11:34:21 -0500
> >>>> Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -EINTR
> >>>>  instead of -EAGAIN
> >>> You should stop sending such conversions to EAGAIN. We switched
> >>> to ERESTART, and you (just guessing) taking the patch from an
> >>> older Xen version shouldn't result in this recurring mistake.
> >> Nah, Andrew said in his email EAGAIN so that is what I picked.
> > 
> > EAGAIN was correct for the domain_destroy hypercall, at this hypercall
> > explicitly has continuation built into its API via EAGAIN.
> 
> Ouch - I based the comment on code resulting from a patch I
> didn't send out yet (largely because Konrad indicated issues with
> XEN_DOMCTL_destroydomain that I'd need to look into in more
> detail before doing so), doing away with the tool stack based
> continuations. Yet based on what domain_kill() does with
> domain_relinquish_resources()'s return value, it should
> nevertheless be -ERESTART here to ease future changes.

OK :-)


>From ac642805a96261f518fbba7d47f3ca38c950b3c8 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 22 Jan 2015 11:34:21 -0500
Subject: [PATCH] domain: In vcpu_destroy_pagetables we can return -ERESTART
 instead of -EINTR

which has the side effect that domain_relinquish_resources will stop
and return to user-space -EINTR - which it is not equipped to deal with.
The preemption mechanism we have to domain destruction is to return
-EAGAIN and as such we need to catch the case of:

domain_relinquish_resources
  ->vcpu_destroy_pagetables
    -> put_page_and_type_preemptible
       -> __put_page_type
           returns -EINTR

and convert it to the proper type.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Add comment and s/ERESTART/EAGAIN/
---
 xen/arch/x86/mm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6e9c2c0..5452c01 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2677,6 +2677,16 @@ int vcpu_destroy_pagetables(struct vcpu *v)
 
     v->arch.cr3 = 0;
 
+    /*
+     * The put_page_and_type_preemptible is liable to return -EINTR. Other
+     * callers of it filter the -EINTR to whatever they deem applicable - in
+     * this case we MUST do it as the caller of this function will return the
+     * error code to userspace. And userspace for domain destruction expects
+     * -EAGAIN (domain_relinquish_resources converts ERESTART to -EAGAIN).
+     */
+    if ( rc == -EINTR )
+        rc = -ERESTART;
+
     return rc;
 }
 
-- 
2.1.0