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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


> > 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.