Re: [PATCH v2 5/7] xen: add new domctl get_changed_domain

[Jürgen Groß <jgross@xxxxxxxx>]

On 09.12.24 18:04, Jan Beulich wrote:
> On 06.12.2024 14:02, Juergen Gross wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -192,6 +192,54 @@ static void domain_changed_state(const struct domain *d)
> >       spin_unlock(&dom_state_changed_lock);
> >   }
> >   
> > +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
> > +                                  const struct domain *d)
> > +{
> > +    info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
> > +    if ( d->is_shut_down )
> > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
> > +    if ( d->is_dying == DOMDYING_dead )
> > +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> The public constant saying "dying" isn't quite in line with the internal
> constant saying "dead". It may well be that Xenstore only cares about the
> "dead" state, but then it would better be nemaed this way also in the
> public interface, I think.
Okay, I'll rename it to "XEN_DOMCTL_GETDOMSTATE_STATE_DEAD".

> > +    info->unique_id = d->unique_id;
> > +}
> > +
> > +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain 
> > *d,
> > +                     domid_t *domid)
> > +{
> > +    unsigned int dom;
> > +
> > +    memset(info, 0, sizeof(*info));
> > +
> > +    if ( d )
> > +    {
> > +        set_domain_state_info(info, d);
> > +
> > +        return 0;
> > +    }
> > +
> > +    while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
> I can't spot any check that dom_state_changed was actually allocated. Also,
> unlike the public header comments says, the new sub-op looks to be usable
> by _anyone_ (eligible privilege-wise) as long as _someone_ did set up the
> vIRQ. There looks to even be a race possible when a "wrong" caller tries to
> call this at jut the "right" time.
Right, I'll add a check that the calling domain is registered for the
vIRQ. Additionally I'll put in an "ASSERT(dom_state_changed);".

> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -278,6 +278,11 @@ static struct vnuma_info *vnuma_init(const struct 
> > xen_domctl_vnuma *uinfo,
> >       return ERR_PTR(ret);
> >   }
> >   
> > +static bool is_stable_domctl(uint32_t cmd)
> > +{
> > +    return cmd == XEN_DOMCTL_get_domain_state;
> > +}
> Likely better as switch() from the very beginning.
>
> > @@ -866,6 +873,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> > u_domctl)
> >                   __HYPERVISOR_domctl, "h", u_domctl);
> >           break;
> >   
> > +    case XEN_DOMCTL_get_domain_state:
> > +        ret = xsm_get_domain_state(XSM_XS_PRIV, d);
> > +        if ( ret )
> > +            break;
> > +
> > +        copyback = 1;
> > +        ret = get_domain_state(&op->u.get_domain_state, d, &op->domain);
> > +        break;
> Especially with this being a stable interface, surely the two padding fields
> want checking to be zero on input (to possibly allow their future use for
> something input-ish). Then even the memset() in the function may not really
> be needed.
I'll add the check. Removing the memset() is a little bit doubtful, as this
might result in leaking hypervisor data e.g. in case a domain isn't existing
(this will copy the internal struct to the user even in the -ENOENT case).


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature