Re: [PATCH] radix-tree: don't left-shift negative values

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/02/2025 7:26 pm, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Luca Fancellu wrote:
>>> On 13 Feb 2025, at 15:42, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> wrote:
>>> On 2025-02-13 16:32, Nicola Vetrini wrote:
>>>> On 2025-02-13 16:01, Jan Beulich wrote:
>>>>> On 13.02.2025 15:52, Nicola Vetrini wrote:
>>>>>> On 2025-02-13 15:22, Jan Beulich wrote:
>>>>>>> Any (signed) integer is okay to pass into radix_tree_int_to_ptr(), yet
>>>>>>> left shifting negative values is UB. Use an unsigned intermediate type,
>>>>>>> reducing the impact to implementation defined behavior (for the
>>>>>>> unsigned->signed conversion).
>>>>>>> Also please Misra C:2012 rule 7.3 by dropping the lower case numeric
>>>>>>> 'l'
>>>>>>> tag.
>>>>>>> No difference in generated code, at least on x86.
>>>>>>> Fixes: b004883e29bb ("Simplify and build-fix (for some gcc versions)
>>>>>>> radix_tree_int_to_ptr()")
>>>>>>> Reported-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>>> ---
>>>>>>> Bugseng: Why was the 7.3 violation not spotted by Eclair? According to
>>>>>>>         tagging.ecl the codebase is clean for this rule, aiui.
>>>>>> radix-tree.{c,h} is out of scope:
>>>>>> automation/eclair_analysis/ECLAIR/out_of_scope.ecl:32:-file_tag+={out_of_scope,"^xen/include/xen/radix-tree\\.h$"}
>>>>>> docs/misra/exclude-list.json:153:            "rel_path":
>>>>>> "common/radix-tree.c",
>>>>> Is there a record of why they are excluded? Is it further explainable
>>>>> why exclude-list.json mentions only the .c file and out_of_scope.ecl
>>>>> mentions only the .h one? Shouldn't different parts be in sync?
>>>> exclude-list.json is used to generate a configuration file for ECLAIR just 
>>>> before the analysis starts, so effectively both are excluded. It's a good 
>>>> point however to have only one file to handle exclusions, and use that 
>>>> file to generate the exclusion list dynamically, but then someone might 
>>>> want to exclude certain files only in some analyses and not others, which 
>>>> is not a good fit for exclude-list.json as it is now.
>>>> @Stefano, thoughts?
>>> I forgot to address the first question: the (vague) reasons are listed in 
>>> exclude-list.json as the "comment" field; in most cases, it's because the 
>>> files have been imported from Linux, but the full rationale is something 
>>> that should be asked to the original author, which is Luca Fancellu.
>> So IIRC the full rationale is that since some files are imported from Linux, 
>> we would like to maintain them as they are
>> in order to ease backports. Misra fixes can be done, but they need to be 
>> upstreamed to Linux and backported to Xen.
>>
>> Probably a re-evaluation could be done by the maintainers to see if some of 
>> these files could be removed from the exclusion
>> list.
> Yes, it is as Luca said. At the beginning of the project, we reviewed
> the codebase to define what was in scope and what was out of scope. One
> area of contention was the files imported from Linux. Many of these
> files were declared out of scope because we wanted to retain the ability
> to easily synchronize them with their corresponding files in Linux.  
>
> Now, years have passed, and we have gained significant experience from
> running this project. It is completely acceptable to redefine the scope,
> including making changes to exclude-list.json.
>
> However, we do not necessarily need to modify exclude-list.json to
> accept a single, clearly beneficial fix like this one. So, Jan, feel
> free to proceed and commit it.  
>
> I just wanted to provide some background. If you believe that removing
> common/radix-tree.c from docs/misra/exclude-list.json, and thereby
> including it in ECLAIR's regular scanning, would be the best approach, I
> am also fine with that.

I agree with Jan that it's important that we have a single source of truth.

Furthermore, it is critical that the justification of why things are in
certain categories are identified.  It only needs to be a single
sentence in a comment, but a developer needs to be able to look at the
file and figure out *why* a decision was taken...

... because as Stefano says, decisions change over time, opinions and
scope change, etc.


As to the specifics of radix-tree, I personally think is rather
disingenuous to say "here's a data-structure fundamental to the
operation of Xen, but because the code is written in Linux style we
chose to ignore problems in it."   A certifier would be well with their
rights to tell you where to go if you tried to argue that point.

It is code in Xen, and critical to Xen's behaviour.  It doesn't matter
if you want to do a Linux-first or Xen-first approach to fixing issues;
the issues need fixing.

~Andrew