Re: [XEN PATCH v3 03/16] misra: add deviations for direct inclusion guards

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.03.2024 01:43, Stefano Stabellini wrote:
> On Fri, 15 Mar 2024, Jan Beulich wrote:
>> On 14.03.2024 23:59, Stefano Stabellini wrote:
>>> On Mon, 11 Mar 2024, Simone Ballarin wrote:
>>>> On 11/03/24 14:56, Jan Beulich wrote:
>>>>> On 11.03.2024 13:00, Simone Ballarin wrote:
>>>>>> On 11/03/24 11:08, Jan Beulich wrote:
>>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote:
>>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>> +/* SAF-5-safe direct inclusion guard before */
>>>>>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>>>>>    #error "asm/hypercall.h should not be included directly - include
>>>>>>>> xen/hypercall.h instead"
>>>>>>>>    #endif
>>>>>>>> --- a/xen/arch/x86/include/asm/hypercall.h
>>>>>>>> +++ b/xen/arch/x86/include/asm/hypercall.h
>>>>>>>> @@ -2,6 +2,7 @@
>>>>>>>>     * asm-x86/hypercall.h
>>>>>>>>     */
>>>>>>>>    +/* SAF-5-safe direct inclusion guard before */
>>>>>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>>>>>    #error "asm/hypercall.h should not be included directly - include
>>>>>>>> xen/hypercall.h instead"
>>>>>>>>    #endif
>>>>>>>
>>>>>>> Iirc it was said that this way checking for correct guards is suppressed
>>>>>>> altogether in Eclair, which is not what we want. Can you clarify this,
>>>>>>> please?
>>>>>>>
>>>>>>
>>>>>> My first change was moving this check inside the guard.
>>>>>> You commented my patch saying that this would be an error because someone
>>>>>> can
>>>>>> include it directly if it has already been included indirectly.
>>>>>> I replied telling that this was the case also before the change.
>>>>>> You agreed with me, and we decided that the correct thing would be fixing
>>>>>> the
>>>>>> check and not apply my temporary change to address the finding.
>>>>>>
>>>>>> Considering that the code should be amended, a SAF deviation seems to me
>>>>>> the most appropriate way for suppressing these findings.
>>>>>
>>>>> Since I don't feel your reply addresses my question, asking differently:
>>>>> With
>>>>> your change in place, will failure to have proper guards (later) in these
>>>>> headers still be reported by Eclair?
>>>>
>>>> No, if you put something between the check and the guard,
>>>> no violation will be reported.
>>>
>>> From this email exchange I cannot under if Jan is OK with this patch or
>>> not.
>>
>> Whether I'm okay(ish) with the patch here depends on our position towards
>> the lost checking in Eclair mentioned above. To me it still looks relevant
>> that checking for a guard occurs, even if that isn't first in a file for
>> some specific reason.
> 
> More checking is better than less checking, but if we cannot find a
> simple and actionable way forward on this violation, deviating it is
> still a big improvement because it allows us to enable the ECLAIR Dir
> 4.10 checks in xen.git overall (which again goes back to more checking
> is better than less checking). 

You have a point here. I think though that at the very least the lost
checking opportunity wants calling out quite explicitly.

> Do you have a simple alternative suggestion? If not, this is still an
> improvement.

I don't know the inner workings of Eclair. Without that I'm afraid I'm not
in a position to make alternative suggestions.

Jan