1. Patchew
  2. Xen
  3. View series

[PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext

Mykola Kvach posted 1 patch 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/034559c3324e137285065b12642cbf58b7ab5f58.1753727619.git.mykola._5Fkvach@epam.com
xen/common/domctl.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext
Posted by Mykola Kvach 3 months ago 
From: Mykola Kvach <mykola_kvach@epam.com>

Add domain_{lock,unlock} in the XEN_DOMCTL_setvcpucontext operation
for protecting arch_set_info_guest.

This aligns with the locking pattern used by other operations that
modify vCPU state.

Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/common/domctl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index f2a7caaf85..f7bf6f4534 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -392,7 +392,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         if ( ret == 0 )
         {
             domain_pause(d);
+            domain_lock(d);
             ret = arch_set_info_guest(v, c);
+            domain_unlock(d);
             domain_unpause(d);
 
             if ( ret == -ERESTART )
-- 
2.48.1
Re: [PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext
Posted by Jan Beulich 3 months ago 
On 28.07.2025 20:40, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@epam.com>
> 
> Add domain_{lock,unlock} in the XEN_DOMCTL_setvcpucontext operation
> for protecting arch_set_info_guest.
> 
> This aligns with the locking pattern used by other operations that
> modify vCPU state.
> 
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>

I think this requires more of a description / justification. May I in
particular turn your attention to this comment that we have in x86'es
handling of HVM_PARAM_IDENT_PT (disregard the 1st sentence for the
purpose here):

        /*
         * Update GUEST_CR3 in each VMCS to point at identity map.
         * All foreign updates to guest state must synchronise on
         * the domctl_lock.
         */
        rc = -ERESTART;
        if ( !domctl_lock_acquire(d) )
            break;

IOW in particular I'd expect you to explain why holding the domctl
lock isn't sufficient here, and hence what (theoretical?) race it is
you're concerned about. That may in turn clarify whether a Fixes: tag
would actually be appropriate here.

Jan

> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -392,7 +392,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          if ( ret == 0 )
>          {
>              domain_pause(d);
> +            domain_lock(d);
>              ret = arch_set_info_guest(v, c);
> +            domain_unlock(d);
>              domain_unpause(d);
>  
>              if ( ret == -ERESTART )
Re: [PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext
Posted by Mykola Kvach 2 months, 3 weeks ago 
Hi Jan,

On Tue, Jul 29, 2025 at 10:56 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.07.2025 20:40, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@epam.com>
> >
> > Add domain_{lock,unlock} in the XEN_DOMCTL_setvcpucontext operation
> > for protecting arch_set_info_guest.
> >
> > This aligns with the locking pattern used by other operations that
> > modify vCPU state.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>
> I think this requires more of a description / justification. May I in
> particular turn your attention to this comment that we have in x86'es
> handling of HVM_PARAM_IDENT_PT (disregard the 1st sentence for the
> purpose here):
>
>         /*
>          * Update GUEST_CR3 in each VMCS to point at identity map.
>          * All foreign updates to guest state must synchronise on
>          * the domctl_lock.
>          */
>         rc = -ERESTART;
>         if ( !domctl_lock_acquire(d) )
>             break;
>
> IOW in particular I'd expect you to explain why holding the domctl
> lock isn't sufficient here, and hence what (theoretical?) race it is
> you're concerned about. That may in turn clarify whether a Fixes: tag
> would actually be appropriate here.

For example, on ARM systems, we bring up vCPUs via PSCI. At the same time,
from another domain, XEN_DOMCTL_setvcpucontext may be called. In the PSCI
path, access is protected by domain_lock, but domain_pause alone is not
sufficient to prevent races during modification of the vCPU context from
XEN_DOMCTL_setvcpucontext.

>
> Jan
>
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -392,7 +392,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >          if ( ret == 0 )
> >          {
> >              domain_pause(d);
> > +            domain_lock(d);
> >              ret = arch_set_info_guest(v, c);
> > +            domain_unlock(d);
> >              domain_unpause(d);
> >
> >              if ( ret == -ERESTART )
>

Best regards,
Mykola
Re: [PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext
Posted by Mykola Kvach 2 months, 3 weeks ago 
On Thu, Aug 7, 2025 at 8:47 AM Mykola Kvach <xakep.amatop@gmail.com> wrote:
>
> Hi Jan,
>
> On Tue, Jul 29, 2025 at 10:56 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 28.07.2025 20:40, Mykola Kvach wrote:
> > > From: Mykola Kvach <mykola_kvach@epam.com>
> > >
> > > Add domain_{lock,unlock} in the XEN_DOMCTL_setvcpucontext operation
> > > for protecting arch_set_info_guest.
> > >
> > > This aligns with the locking pattern used by other operations that
> > > modify vCPU state.
> > >
> > > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> >
> > I think this requires more of a description / justification. May I in
> > particular turn your attention to this comment that we have in x86'es
> > handling of HVM_PARAM_IDENT_PT (disregard the 1st sentence for the
> > purpose here):
> >
> >         /*
> >          * Update GUEST_CR3 in each VMCS to point at identity map.
> >          * All foreign updates to guest state must synchronise on
> >          * the domctl_lock.
> >          */
> >         rc = -ERESTART;
> >         if ( !domctl_lock_acquire(d) )
> >             break;
> >
> > IOW in particular I'd expect you to explain why holding the domctl
> > lock isn't sufficient here, and hence what (theoretical?) race it is
> > you're concerned about. That may in turn clarify whether a Fixes: tag
> > would actually be appropriate here.
>
> For example, on ARM systems, we bring up vCPUs via PSCI. At the same time,
> from another domain, XEN_DOMCTL_setvcpucontext may be called. In the PSCI
> path, access is protected by domain_lock, but domain_pause alone is not
> sufficient to prevent races during modification of the vCPU context from
> XEN_DOMCTL_setvcpucontext.

Sorry, we don't need any extra locking here, because domain_pause
performs vCPU blocking synchronously. It waits until the vCPU becomes
non-runnable, and only after that the context is updated.

So this patch isn't needed.

>
> >
> > Jan
> >
> > > --- a/xen/common/domctl.c
> > > +++ b/xen/common/domctl.c
> > > @@ -392,7 +392,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> > >          if ( ret == 0 )
> > >          {
> > >              domain_pause(d);
> > > +            domain_lock(d);
> > >              ret = arch_set_info_guest(v, c);
> > > +            domain_unlock(d);
> > >              domain_unpause(d);
> > >
> > >              if ( ret == -ERESTART )
> >
>
> Best regards,
> Mykola

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.