Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

On 16/10/23 17:26, Jan Beulich wrote:
> On 03.10.2023 17:24, Federico Serafini wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5901,17 +5901,17 @@ int destroy_xen_mappings(unsigned long s, unsigned long 
> > e)
> >    * a problem.
> >    */
> >   void init_or_livepatch modify_xen_mappings_lite(
> > -    unsigned long s, unsigned long e, unsigned int _nf)
> > +    unsigned long s, unsigned long e, unsigned int nf)
> >   {
> > -    unsigned long v = s, fm, nf;
> > +    unsigned long v = s, fm, flags;
> While it looks correct, I consider this an unacceptably dangerous
> change: What if by the time this is to be committed some new use of
> the local "nf" appears, without resulting in fuzz while applying the
> patch? Imo this needs doing in two steps: First nf -> flags, then
> _nf -> nf.
>
> Furthermore since you alter the local variable, is there any reason
> not to also change it to be "unsigned int", matching the function
> argument it's set from?
If you are referring to the local variable 'flags', it is set using the
value returned from put_pte_flags() which is an intpte_t (typedef for 
u64). Am I missing something?
> Yet then - can't we just delete "nf" and rename "_nf" to "nf"? The
> function parameter is only used in
>
>      nf = put_pte_flags(_nf & FLAGS_MASK);
>
> Jan
I thought about it but it will introduce a violation of Rule 17.8:
"A function parameter should not be modified".
It is an advisory rule that is not currently accepted but one day
it may be.

--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)