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.
This is not an issue for the later location where rcu_unlock_domain() is
called, because at that point the src domain does have domain state
privilege for the target domain.
+ 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.
{
*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;
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?
}
+
+ if ( d )
+ {
+ rcu_unlock_domain(d);
+ d = NULL;
+ }
}
out:
--- 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?
v/r,
dps
