Re: [PATCH v3 2/4] xen/arm: Handle cases when hardware_domain is NULL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.04.2021 15:58, Luca Fancellu wrote:
> 
> 
>> On 12 Apr 2021, at 12:03, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 12.04.2021 12:52, Luca Fancellu wrote:
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -1022,6 +1022,9 @@ static always_inline bool is_hardware_domain(const 
>>> struct domain *d)
>>>     if ( IS_ENABLED(CONFIG_PV_SHIM_EXCLUSIVE) )
>>>         return false;
>>>
>>> +    if ( !d )
>>> +        return false;
>>> +
>>>     return evaluate_nospec(d == hardware_domain);
>>> }
>>
>> On v2 I did say on the respective code that was here (and my
>> suggestion of this alternative adjustment): "Can you point out
>> code paths where d may actually be NULL, and where [...] would
>> not behave as intended (i.e. where bad speculation would
>> result)?"
>>
>> Since you've taken the suggestion as-is, and since the commit
>> message says nothing in either direction here, did you actually
>> verify that there's no abuse of speculation possible with this
>> extra return path? And did you find any caller at all which may
>> pass NULL into here?
> 
> Hi Jan,
> 
> I’ve analysed the code and seems that there are no path that calls 
> Is_hardware_domain() with a NULL domain, however I found this
> function in xen/arch/arm/irq.c:
> 
> bool irq_type_set_by_domain(const struct domain *d)
> {
>     return is_hardware_domain(d);
> }
> 
> That is calling directly is_hardware_domain and it is global.
> It can be the case that a future code can call irq_type_set_by_domain
> potentially with a null domain...

I don't think that a function being global (or not) matters here. This
might be different in an environment like Linux, where modules may
also call functions, and where guarding against NULL might be desirable
in certain cases.

> I introduced a check for the domain for that reason, do you think that
> maybe it’s better to put that check on the irq_type_set_by_domain instead?

If there's a specific reason to have a guard here, then it should be
here, yes. As per above I would think though that if there's no
present reason to check for NULL, such a check would best be omitted.
We don't typically check internal function arguments like this, after
all.

Jan