Re: [PATCH v2 5/7] xen: add new domctl get_changed_domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2024 16:52, Jürgen Groß wrote:
> On 09.12.24 18:04, Jan Beulich wrote:
>> On 06.12.2024 14:02, Juergen Gross wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -192,6 +192,54 @@ static void domain_changed_state(const struct domain 
>>> *d)
>>>       spin_unlock(&dom_state_changed_lock);
>>>   }
>>>   
>>> +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
>>> +                                  const struct domain *d)
>>> +{
>>> +    info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
>>> +    if ( d->is_shut_down )
>>> +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
>>> +    if ( d->is_dying == DOMDYING_dead )
>>> +        info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
>>
>> The public constant saying "dying" isn't quite in line with the internal
>> constant saying "dead". It may well be that Xenstore only cares about the
>> "dead" state, but then it would better be nemaed this way also in the
>> public interface, I think.
> 
> Okay, I'll rename it to "XEN_DOMCTL_GETDOMSTATE_STATE_DEAD".

Well, maybe have both DYING and DEAD, even if Xenstore right now needs only one?

>>> @@ -866,6 +873,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>>> u_domctl)
>>>                   __HYPERVISOR_domctl, "h", 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);
>>> +        break;
>>
>> Especially with this being a stable interface, surely the two padding fields
>> want checking to be zero on input (to possibly allow their future use for
>> something input-ish). Then even the memset() in the function may not really
>> be needed.
> 
> I'll add the check. Removing the memset() is a little bit doubtful, as this
> might result in leaking hypervisor data e.g. in case a domain isn't existing
> (this will copy the internal struct to the user even in the -ENOENT case).

Which internal struct? The function is passed &op->... for both parameters.
And op is fully copied from guest space.

Jan