On 19.02.2026 13:34, Daniel P. Smith wrote:
> On 2/19/26 06:11, Jan Beulich wrote:
>> On 18.02.2026 20:08, Daniel P. Smith wrote:
>>> @@ -238,28 +242,39 @@ int get_domain_state(struct
>>> xen_domctl_get_domain_state *info, struct domain *d,
>>>
>>> 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) )
>>> + {
>>> + rcu_unlock_domain(d);
>>> + d = NULL;
>>
>> This looks unnecessary; the next loop iteration will set d unconditionally,
>> and d isn't (and wasn't) used past the loop. Plus there is also no such
>> clearing after the other rcu_unlock_domain().
>>
>
> If the src domain didn't have the permission on any of the target
> domains, then *d will leak the last domain checked back to the caller.
> While I didn't see it being used after the call site, it's a good
> principle not to leak from a function an object for which access was denied.
I don't follow: What do you mean by "*d will leak"? d is local to this function;
no domain pointer is being returned.
>>> + continue;
>>> + }
>>
>> This cleanup here also is redundant with the one done further down. Imo where
>> possible we should prefer to have only a single such instance, which looks to
>> be easily possible by using ...
>>
>>> if ( test_and_clear_bit(dom, dom_state_changed) )
>>
>>
>> if ( (!d || !xsm_get_domain_state(XSM_XS_PRIV, d)) &&
>> test_and_clear_bit(dom, dom_state_changed) )
>>
>> or
>>
>> if ( (d && xsm_get_domain_state(XSM_XS_PRIV, d)) ||
>> !test_and_clear_bit(dom, dom_state_changed) )
>> {
>> ...
>> continue;
>> }
>>
>> albeit then the reduction of indentation of the subsequent code would cause
>> quite a bit more code churn.
>
> I would prefer the latter to keep the clearing of d to NULL only when
> access was denied.
I again don't understand, as ...
>>> {
>>> *domid = dom;
>>>
>>> - d = rcu_lock_domain_by_id(dom);
>>> -
>>> if ( d )
>>> {
>>> set_domain_state_info(info, d);
>>> -
>>> rcu_unlock_domain(d);
>>> }
>>> else
>>> memset(info, 0, sizeof(*info));
>>>
>>> rc = 0;
>>> -
>>> break;
... this code would remain unchanged apart from the reduced indentation.
>> I don't think the blank lines need dropping for the purpose of the patch?
>> Yes, they may seem excessive, but nevertheless some prefer to have rather
>> too many of them than too few. (Personally I don't mind their removal,
>> but that really doesn't look to belong here.)
>
> Okay, but as you state above, with the compounding of the if statement,
> the indentation will go away and the whole section will a remove/add
> section to the diff. Would you still want the line spacing maintained?
As said, I don't mind those particular blank lines being dropped (if that's
because adjacent lines are touched anyway).
>>> --- a/xen/common/domctl.c
>>> +++ b/xen/common/domctl.c
>>> @@ -860,12 +860,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>>> 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);
>>> + if ( !ret )
>>> + copyback = 1;
>>
>> Nit: As you need to touch this, please switch to using "true".
>
> Just to be clear, you mean switching to a conditional statement vs
> testing for not zero?
No. I mean using "true" in place of "1", since copyback is of type "bool".
Jan
