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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2024 08:44, Jürgen Groß wrote:
> On 10.12.24 17:29, Jan Beulich wrote:
>> 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?
> 
> Yes, might be interesting in the future.
> 
>>
>>>>> @@ -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.
> 
> Sigh. I shouldn't have answered so quickly while being deep into other
> topics. :-(
> 
> While I agree that the caller _should_ pass these fields zeroed, I'm still
> not sure we should rely on it.

You said you'd add the check. Then we not just rely on caller zeroing, but
we actually refuse non-zero fields there. And we fill all other fields.
Hence ...

> Do you insist on removing the memset()? If not I'd rather keep it.

... while I guess I wouldn't insist, to me such a memset() effectively
would count as dead code (for having no recognizable effect). And you know
what Misra thinks of dead code, even if strictly by their criteria this
wouldn't count as "dead". Yet it would violate the underlying principle of
there wanting to be a (functional) reason for everything there is.

Jan