On 13.12.2024 17:24, Juergen Gross wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -193,6 +193,57 @@ 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_dying )
> + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> + if ( d->is_dying == DOMDYING_dead )
> + info->state |= 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;
> +
> + if ( info->pad0 || info->pad1 )
> + return -EINVAL;
> +
> + if ( d )
> + {
> + set_domain_state_info(info, d);
> +
> + return 0;
> + }
> +
> + while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
> + DOMID_FIRST_RESERVED )
> + {
> + if ( test_and_clear_bit(dom, dom_state_changed) )
For these two accesses to dom_state_changed don't you need to hold the
lock patch 4 introduces? Also didn't you say you'd constrain the new
sub-op to the sole domain having VIRQ_DOM_EXEC bound (which, ftaod,
isn't enough to eliminate the race)?
> + {
> + *domid = dom;
> +
> + d = rcu_lock_domain_by_id(dom);
> +
> + if ( d )
> + {
> + set_domain_state_info(info, d);
> +
> + rcu_unlock_domain(d);
> + }
Oh, on the implicit "else" is where the original memset() would come into
play: You want to make sure at least ->state, but perhaps also ->unique_id
are cleared (rather than demanding the caller to clear them ahead of making
the call).
Jan
