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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 2/19/26 08:23, Jan Beulich wrote:
> 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.
My bad, you are correct, will drop.

> > > > +            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).
ack.

> > > > --- 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".
ack.