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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> +    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.

> --- 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.

Jan