[3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
diff mbox series

Message ID 20191205223008.8623-4-andrew.cooper3@citrix.com
State New
Headers show
Series
  • xen: Support continuations from tasklets
Related show

Commit Message

Andrew Cooper Dec. 5, 2019, 10:30 p.m. UTC
More paths are going start using hypercall continuations.  We could add extra
calls to hypercall_create_continuation() but it is much easier to handle
-ERESTART once at the top level.

One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
compatibility in a security fix, turn a DOMCTL continuation into
__HYPERVISOR_arch_1.  This remains as it was, gaining a comment explaining
what is going on.

With -ERESTART handling in place, the !domctl_lock_acquire() path can use the
normal exit path, instead of opencoding a subset of it locally.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/arch/x86/domctl.c           |  5 ++++-
 xen/arch/x86/mm/hap/hap.c       |  3 +--
 xen/arch/x86/mm/shadow/common.c |  3 +--
 xen/common/domctl.c             | 19 +++++--------------
 4 files changed, 11 insertions(+), 19 deletions(-)

Comments

Julien Grall Dec. 8, 2019, 12:18 p.m. UTC | #1
Hi,

On 05/12/2019 22:30, Andrew Cooper wrote:
> More paths are going start using hypercall continuations.  We could add extra
> calls to hypercall_create_continuation() but it is much easier to handle
> -ERESTART once at the top level.
> 
> One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
> compatibility in a security fix, turn a DOMCTL continuation into
> __HYPERVISOR_arch_1.  This remains as it was, gaining a comment explaining
> what is going on.
> 
> With -ERESTART handling in place, the !domctl_lock_acquire() path can use the
> normal exit path, instead of opencoding a subset of it locally.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> ---
>   xen/arch/x86/domctl.c           |  5 ++++-
>   xen/arch/x86/mm/hap/hap.c       |  3 +--
>   xen/arch/x86/mm/shadow/common.c |  3 +--
>   xen/common/domctl.c             | 19 +++++--------------

You seem to have missed the hypercall_create_continuation() call in 
iommu_do_pci_domctl().

Cheers,
Jan Beulich Dec. 9, 2019, 4:19 p.m. UTC | #2
On 05.12.2019 23:30, Andrew Cooper wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -326,9 +326,12 @@ long arch_do_domctl(
>  
>      switch ( domctl->cmd )
>      {
> -
>      case XEN_DOMCTL_shadow_op:
>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
> +        /*
> +         * Continuations from paging_domctl() switch index to arch_1, and
> +         * can't use the common domctl continuation path.
> +         */
>          if ( ret == -ERESTART )
>              return hypercall_create_continuation(__HYPERVISOR_arch_1,
>                                                   "h", u_domctl);

There's also XEN_DOMCTL_getpageframeinfo3 down from here which
now invokes a continuation.

> @@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      if ( copyback && __copy_to_guest(u_domctl, op, 1) )
>          ret = -EFAULT;
>  
> +    if ( ret == -ERESTART )
> +        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
> +                                            "h", u_domctl);

You may want to mention in the description the bug you fix here:
Previously the -EFAULT returning visible in context should have
canceled any active continuation.

Jan
Andrew Cooper Dec. 9, 2019, 5:20 p.m. UTC | #3
On 08/12/2019 12:18, Julien Grall wrote:
> Hi,
>
> On 05/12/2019 22:30, Andrew Cooper wrote:
>> More paths are going start using hypercall continuations.  We could
>> add extra
>> calls to hypercall_create_continuation() but it is much easier to handle
>> -ERESTART once at the top level.
>>
>> One complication is XEN_DOMCTL_shadow_op, which for XSA-97 and ABI
>> compatibility in a security fix, turn a DOMCTL continuation into
>> __HYPERVISOR_arch_1.  This remains as it was, gaining a comment
>> explaining
>> what is going on.
>>
>> With -ERESTART handling in place, the !domctl_lock_acquire() path can
>> use the
>> normal exit path, instead of opencoding a subset of it locally.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> ---
>>   xen/arch/x86/domctl.c           |  5 ++++-
>>   xen/arch/x86/mm/hap/hap.c       |  3 +--
>>   xen/arch/x86/mm/shadow/common.c |  3 +--
>>   xen/common/domctl.c             | 19 +++++--------------
>
> You seem to have missed the hypercall_create_continuation() call in
> iommu_do_pci_domctl().

So I have.  Fixed.

~Andrew
Andrew Cooper Dec. 9, 2019, 5:29 p.m. UTC | #4
On 09/12/2019 16:19, Jan Beulich wrote:
> On 05.12.2019 23:30, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -326,9 +326,12 @@ long arch_do_domctl(
>>  
>>      switch ( domctl->cmd )
>>      {
>> -
>>      case XEN_DOMCTL_shadow_op:
>>          ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
>> +        /*
>> +         * Continuations from paging_domctl() switch index to arch_1, and
>> +         * can't use the common domctl continuation path.
>> +         */
>>          if ( ret == -ERESTART )
>>              return hypercall_create_continuation(__HYPERVISOR_arch_1,
>>                                                   "h", u_domctl);
> There's also XEN_DOMCTL_getpageframeinfo3 down from here which
> now invokes a continuation.

Fixed.

>
>> @@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      if ( copyback && __copy_to_guest(u_domctl, op, 1) )
>>          ret = -EFAULT;
>>  
>> +    if ( ret == -ERESTART )
>> +        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
>> +                                            "h", u_domctl);
> You may want to mention in the description the bug you fix here:
> Previously the -EFAULT returning visible in context should have
> canceled any active continuation.

That would be presuming I'd even spotted it...

Having looked though the paths once again, I don't think there was a
path which continued and had copyback set, so this is at most a latent bug.

~Andrew
Jan Beulich Dec. 10, 2019, 8:09 a.m. UTC | #5
On 09.12.2019 18:29, Andrew Cooper wrote:
> On 09/12/2019 16:19, Jan Beulich wrote:
>> On 05.12.2019 23:30, Andrew Cooper wrote:
>>> @@ -1080,6 +1068,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>>      if ( copyback && __copy_to_guest(u_domctl, op, 1) )
>>>          ret = -EFAULT;
>>>  
>>> +    if ( ret == -ERESTART )
>>> +        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
>>> +                                            "h", u_domctl);
>> You may want to mention in the description the bug you fix here:
>> Previously the -EFAULT returning visible in context should have
>> canceled any active continuation.
> 
> That would be presuming I'd even spotted it...
> 
> Having looked though the paths once again, I don't think there was a
> path which continued and had copyback set, so this is at most a latent
> bug.

Ah, good. I actually first meant to check, but then forgot before
sending the earlier reply.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b461aadbd6..2fa0e7dda5 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -326,9 +326,12 @@  long arch_do_domctl(
 
     switch ( domctl->cmd )
     {
-
     case XEN_DOMCTL_shadow_op:
         ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0);
+        /*
+         * Continuations from paging_domctl() switch index to arch_1, and
+         * can't use the common domctl continuation path.
+         */
         if ( ret == -ERESTART )
             return hypercall_create_continuation(__HYPERVISOR_arch_1,
                                                  "h", u_domctl);
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..3996e17b7e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -600,8 +600,7 @@  int hap_domctl(struct domain *d, struct xen_domctl_shadow_op *sc,
         paging_unlock(d);
         if ( preempted )
             /* Not finished.  Set up to re-run the call. */
-            rc = hypercall_create_continuation(__HYPERVISOR_domctl, "h",
-                                               u_domctl);
+            rc = -ERESTART;
         else
             /* Finished.  Return the new allocation */
             sc->mb = hap_get_allocation(d);
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 6212ec2c4a..17ca21104f 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3400,8 +3400,7 @@  int shadow_domctl(struct domain *d,
         paging_unlock(d);
         if ( preempted )
             /* Not finished.  Set up to re-run the call. */
-            rc = hypercall_create_continuation(
-                __HYPERVISOR_domctl, "h", u_domctl);
+            rc = -ERESTART;
         else
             /* Finished.  Return the new allocation */
             sc->mb = shadow_get_allocation(d);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 03d0226039..cb0295085d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -415,10 +415,8 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 
     if ( !domctl_lock_acquire() )
     {
-        if ( d && d != dom_io )
-            rcu_unlock_domain(d);
-        return hypercall_create_continuation(
-            __HYPERVISOR_domctl, "h", u_domctl);
+        ret = -ERESTART;
+        goto domctl_out_unlock_domonly;
     }
 
     switch ( op->cmd )
@@ -438,9 +436,6 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         if ( guest_handle_is_null(op->u.vcpucontext.ctxt) )
         {
             ret = vcpu_reset(v);
-            if ( ret == -ERESTART )
-                ret = hypercall_create_continuation(
-                          __HYPERVISOR_domctl, "h", u_domctl);
             break;
         }
 
@@ -469,10 +464,6 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             domain_pause(d);
             ret = arch_set_info_guest(v, c);
             domain_unpause(d);
-
-            if ( ret == -ERESTART )
-                ret = hypercall_create_continuation(
-                          __HYPERVISOR_domctl, "h", u_domctl);
         }
 
         free_vcpu_guest_context(c.nat);
@@ -585,9 +576,6 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         domain_lock(d);
         ret = domain_kill(d);
         domain_unlock(d);
-        if ( ret == -ERESTART )
-            ret = hypercall_create_continuation(
-                __HYPERVISOR_domctl, "h", u_domctl);
         goto domctl_out_unlock_domonly;
 
     case XEN_DOMCTL_setnodeaffinity:
@@ -1080,6 +1068,9 @@  long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     if ( copyback && __copy_to_guest(u_domctl, op, 1) )
         ret = -EFAULT;
 
+    if ( ret == -ERESTART )
+        ret = hypercall_create_continuation(__HYPERVISOR_domctl,
+                                            "h", u_domctl);
     return ret;
 }