[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 2/4] xen/arm: Handle cases when hardware_domain is NULL
> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |