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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.03.2024 04:34, Stefano Stabellini wrote:
> On Mon, 18 Mar 2024, Jan Beulich wrote:
>> 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.
> 
> All right, then maybe this patch can go in with a clarification in the
> commit message?
> 
> Something like:
> 
> Note that with SAF-5-safe in place, failures to have proper guards later
> in the header files will not be reported

That would be okay with me.

Jan