| Message ID | 20191205223008.8623-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Support continuations from tasklets | expand |
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,
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
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
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
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
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; }
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(-)