Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
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, >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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
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 >> --- >> CC: Jan Beulich >> CC: Wei Liu >> CC: Roger Pau Monné >> CC: Stefano Stabellini >> CC: Julien Grall >> CC: Volodymyr Babchuk >> --- >> 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
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, >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
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
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 --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: Stefano Stabellini CC: Julien Grall CC: Volodymyr Babchuk --- 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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 3/6] xen/domctl: Consolidate hypercall continuation handling at the top level
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
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
---
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(-)
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, >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;
}
--
2.11.0
___
Xen-devel mailing list
