On 07.01.2025 17:48, Jürgen Groß wrote:
> On 07.01.25 17:28, Jan Beulich wrote:
>> On 07.01.2025 11:17, Juergen Gross wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -185,6 +185,76 @@ static void domain_changed_state(const struct domain
>>> *d)
>>> unlock_dom_exc_handler(hdl);
>>> }
>>>
>>> +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;
>>> + int rc = -ENOENT;
>>> + struct domain *hdl;
>>> +
>>> + if ( info->pad0 || info->pad1 )
>>> + return -EINVAL;
>>> +
>>> + if ( d )
>>> + {
>>> + set_domain_state_info(info, d);
>>> +
>>> + return 0;
>>> + }
>>> +
>>> + /*
>>> + * Only domain registered for VIRQ_DOM_EXC event is allowed to query
>>> + * domains having changed state.
>>> + */
>>> + if ( !domain_handles_global_virq(current->domain, VIRQ_DOM_EXC) )
>>> + return -EACCES;
>>> +
>>> + hdl = lock_dom_exc_handler();
>>
>> Instead of leaving a small window for races between the if() and this
>> function call, can't you simply compare hdl against current->domain?
>
> Good idea.
>
>>
>>> + while ( dom_state_changed )
>>> + {
>>> + dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
>>> + if ( dom >= DOMID_FIRST_RESERVED )
>>> + break;
>>> + if ( test_and_clear_bit(dom, dom_state_changed) )
>>
>> As this is now running under lock, does it really need to be test-and-clear?
>> What mechanism would allow the flag to be cleared between the find-1st and
>> here? Plus, like for patch 4, I think it could be __clear_bit() here.
>
> It is only under read_lock(), so there are concurrent calls possible.
> I don't think we want to use write_lock() here, do we?
Probably not; I have to admit I didn't even pay attention to this aspect.
Then the set_bit() in domain_changed_state() also need to remain as is (in
patch 4 I think it was).
Jan
