> On 12 Apr 2021, at 15:22, Jan Beulich <jbeul...@suse.com> wrote:
>
> On 12.04.2021 15:58, Luca Fancellu wrote:
>>
>>
>>> On 12 Apr 2021, at 12:03, Jan Beulich <jbeul...@suse.com> 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
