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

[Luca Fancellu <luca.fancellu@xxxxxxx>]

> On 12 Apr 2021, at 15:22, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

Thank you Jan, so as you pointed out, since there is no actual path to call
Is_hardware_domain() with a NULL pointer, then I will remove the check
from is_hardware_domain() in a v4 patch.

Cheers,
Luca

> 
> Jan