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(-)
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;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.