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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-03-19 08:45, Jan Beulich wrote:
> 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.
Coming back to this thread. Yes, I'll update the message to reflect this 
change.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)