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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.02.2025 20:26, 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.  

FTAOD - I didn't think there was anything in the way of me doing so, once
the tree re-opens. Question here is how many _else_ issues there are in
the radix tree code we've got (and in anything else presently excluded).

Jan