Re: [PATCH] xen/domctl: add domain_lock in XEN_DOMCTL_setvcpucontext

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

On Tue, Jul 29, 2025 at 10:56 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 28.07.2025 20:40, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > 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@xxxxxxxx>
>
> 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