Re: [BUG v2] common/domctl: xsm update for get_domain_state access

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2026 12:03, Juergen Gross wrote:
> On 19.02.26 11:52, Jan Beulich wrote:
>> On 19.02.2026 00:04, Jason Andryuk wrote:
>>> On 2026-02-18 14:08, Daniel P. Smith wrote:
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -210,7 +210,7 @@ static void set_domain_state_info(struct 
>>>> xen_domctl_get_domain_state *info,
>>>>    int get_domain_state(struct xen_domctl_get_domain_state *info, struct 
>>>> domain *d,
>>>>                         domid_t *domid)
>>>>    {
>>>> -    unsigned int dom;
>>>> +    unsigned int dom = -1;
>>>>        int rc = -ENOENT;
>>>>        struct domain *hdl;
>>>>    
>>>> @@ -219,6 +219,10 @@ int get_domain_state(struct 
>>>> xen_domctl_get_domain_state *info, struct domain *d,
>>>>    
>>>>        if ( d )
>>>>        {
>>>> +        rc = xsm_get_domain_state(XSM_XS_PRIV, d);
>>>> +        if ( rc )
>>>> +            return rc;
>>>> +
>>>>            set_domain_state_info(info, d);
>>>>    
>>>>            return 0;
>>>> @@ -238,28 +242,39 @@ int get_domain_state(struct 
>>>> xen_domctl_get_domain_state *info, struct domain *d,
>>>
>>> Between the two hunks is this:
>>>
>>>       hdl = lock_dom_exc_handler();
>>>
>>>       /*
>>>        * Only domain registered for VIRQ_DOM_EXC event is allowed to query
>>>        * domains having changed state.
>>>        */
>>>       if ( current->domain != hdl )
>>>       {
>>>           rc = -EACCES;
>>>           goto out;
>>>       }
>>>
>>> So it is only the domain with VIRQ_DOM_EXC that can enter the while loop:
>>>
>>>>    
>>>>        while ( dom_state_changed )
>>>>        {
>>>> -        dom = find_first_bit(dom_state_changed, DOMID_MASK + 1);
>>>> +        dom = find_next_bit(dom_state_changed, DOMID_MASK + 1, dom + 1);
>>>>            if ( dom >= DOMID_FIRST_RESERVED )
>>>>                break;
>>>> +
>>>> +        d = rcu_lock_domain_by_id(dom);
>>>> +        if ( d && xsm_get_domain_state(XSM_XS_PRIV, d) )
>>>
>>> ... if the VIRQ_DOM_EXC owner is denied for domain d ...
>>>
>>>> +        {
>>>> +            rcu_unlock_domain(d);
>>>> +            d = NULL;
>>>> +            continue;
>>>
>>> ... the caller would continue ...
>>>
>>>> +        }
>>>> +
>>>>            if ( test_and_clear_bit(dom, dom_state_changed) )
>>>
>>> ... and this bit would never be cleared.  Should the VIRQ_DOM_EXC owner
>>> always get to clear the bit even if it cannot see the result?
>>
>> I don't think so, no. Whenever a legitimate consumer occurs (the owner of
>> VIRQ_DOM_EXC can change, after all), it'll then consume the bits as needed.
>> More generally, I think we're better off not making the code here depend
>> too much on that special VIRQ_DOM_EXC property.
> 
> OTOH a new VIRQ_DOM_EXC owner will result in a complete reset of the bitmap
> anyway (that is: the bits for all existing domains will be set, while all
> others will be cleared).

Yes, while writing my reply I wondered whether I should mention that. To keep
things a little more simple, I didn't. Plus for this aspect the last sentence
of my earlier reply also applies.

Jan