On 2/18/26 10:47, Jan Beulich wrote:
On 18.02.2026 16:32, Daniel P. Smith wrote:
On 2/18/26 10:03, Jan Beulich wrote:
On 18.02.2026 15:33, Daniel P. Smith wrote:
On 2/17/26 04:34, Jan Beulich wrote:
On 16.02.2026 22:57, 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 = 0;
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,10 +242,10 @@ 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);
if ( dom >= DOMID_FIRST_RESERVED )
break;
- if ( test_and_clear_bit(dom, dom_state_changed) )
+ if ( test_bit(dom, dom_state_changed) )
{
*domid = dom;
This is problematic wrt other work (already talked about in the distant past,
but sadly only making little progress) towards trying to pull some of the
sub-ops out of the domctl-locked region. This subop is one of the prime
candidates, yet only if the test_and_clear_bit() remains here.
Okay, but we can't be clearing the bit if the src domain doesn't have
access. When considering that xsm_domctl() does a no-op check for
XEN_DOMCTL_get_domain_state, deferring to xsm_get_domain_state(), then
any domain could invoke the OP with DOMID_INVALID and clear the bit
before access is checked.
If you want to ensure atomic operations on the bit field, while I am not
a fan of this, a combination with set_bit() could be done. Let the
test_and_clear_bit() remain and then if access check fails, use
set_bit() to put it back. Would that be sufficient for your objective?
No, that could then confuse a legitimate (for that domain) caller. IOW
you would still build upon the domctl lock serializing things. I think
you want to do the XSM check first, and only then use test_and_clear_bit().
Currently, acquiring the struct domain pointer is inside the if
condition. This would have to be moved outside the if condition to be
able to do the access check before calling the bit operation. Which
brings up the question for me, if it is reordered in this fashion, why
not use clear_bit(), aiui it should be atomic as well.
The problem isn't with clear_bit() by itself (yes, it is atomic), but
with how things want doing here: The test-and-clear ensures data for a
given domain can only be retrieved exactly once (until the bit would be
set another time later). With test split from clear, multiple callers
could get data for the same domain, as in the window between test and
clear a 2nd caller can come in and also commit to returning data for
that domain. (Given the restriction on who can invoke this, one might
conclude no such race is possible, but that single domain could process
things on multiple vCPU-s, and would then wrongly be notified twice for
the same domain.)
Thinking further, you are correct the rcu_read_lock will not be enough.
The test_and_clear call will ensure this thread is the one that read and
cleared the bit.
@@ -249,6 +253,15 @@ 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 )
+ {
+ rcu_unlock_domain(d);
+ rc = -ENOENT;
As you don't otherwise use xsm_get_domain_state()'s return value, the need
for this assignment can be eliminated by putting the function call straight
in the if(). Then again, to address the remark above, overall code structure
will need to change quite a bit anyway (so the remark here may be moot).
I can drop the use of rc here and inline it.
+ dom++;
It may be nice to eliminate the need to have this in two places (here and ...
+ continue;
+ }
+
set_domain_state_info(info, d);
rcu_unlock_domain(d);
@@ -256,10 +269,13 @@ int get_domain_state(struct xen_domctl_get_domain_state
*info, struct domain *d,
else
memset(info, 0, sizeof(*info));
+ clear_bit(dom, dom_state_changed);
rc = 0;
break;
}
+
+ dom++;
}
... here), by having the variable's initializer be -1 and then using dom + 1
in the find_next_bit() invocation.
If you want this way, then there are two options, make dom no longer
unsigned or be willing to allow unsigned int overflow. If we go with the
former, If you agree, I would leave it as an int as that should cover
the range of valid domids.
I wouldn't outright nak use of plain int, but I'm putting in effort to remove
such undue uses of that type. Unsigned overflow is well-defined aiui, so I
see no reason why the variable can't remain "unsigned int".
Honestly, I'm not sure why it was not domid_t in the first place. I
can't keep all the rules, but I thought MISRA frowned on overflow usages
(abuse?) like this. If you will ack it with it as a uint or as a
domid_t, then I have no issue doing it this way.
A patch without changing the type I would ack. A switch to domid_t, if indeed
correct (I simply didn't go check yet), likely would want to be a separate
change.
Then I will leave it uint and init it to -1.
v/r,
dps
