On 7/30/25 01:09, Stefano Stabellini wrote:
> On Fri, 25 Jul 2025, Dmytro Prokopchuk1 wrote:
>> On 7/23/25 16:58, Jan Beulich wrote:
>>> On 23.07.2025 12:12, Dmytro Prokopchuk1 wrote:
>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> @@ -568,6 +568,14 @@ C99 Undefined Behaviour 45: Pointers that do not
>>>> point into, or just beyond, the
>>>> -config=MC3A2.R18.2,reports+={safe,
>>>> "any_area(any_loc(any_exp(macro(^page_to_mfn$))))"}
>>>> -doc_end
>>>>
>>>> +-doc_begin="Consider relational pointer comparisons in kernel-related
>>>> sections as safe and compliant."
>>>> +-config=MC3R1.R18.3,reports+={safe,
>>>> "any_area(any_loc(any_exp(macro(name(is_kernel||is_kernel_text||is_kernel_rodata||is_kernel_inittext)))))"}
>>>> +-doc_end
>>>> +
>>>> +-doc_begin="Allow deviations for pointer comparisons in BUG_ON and ASSERT
>>>> macros, treating them as safe for debugging and validation."
>>>> +-config=MC3R1.R18.3,reports+={safe,
>>>> "any_area(any_loc(any_exp(macro(name(BUG_ON||ASSERT)))))"}
>>>> +-doc_end
>>>
>>> Nit: Drop "deviations for" from the verbal description?
>> Ok.
>>
>>>
>>>> --- a/xen/arch/x86/efi/efi-boot.h
>>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>>> @@ -461,7 +461,8 @@ static void __init efi_arch_edd(void)
>>>> params->device_path_info_length =
>>>> sizeof(struct edd_device_params) -
>>>> offsetof(struct edd_device_params, key);
>>>> - for ( p = (const u8 *)¶ms->key; p <
>>>> ¶ms->checksum; ++p )
>>>> + for ( p = (const u8 *)¶ms->key;
>>>> + (uintptr_t)p < (uintptr_t)¶ms->checksum; ++p )
>>>
>>> There mere addition of such casts makes code more fragile imo. What about
>>> the
>>> alternative of using != instead of < here? I certainly prefer < in such
>>> situations,
>>> but functionally != ought to be equivalent (and less constrained by C and
>>> Misra).
>>>
>>> As mentioned several times when discussing these rules, it's also not easy
>>> to see
>>> how "pointers of different objects" could be involved here: It's all within
>>> *params, isn't it?
>> Hard to say something. Let's hold this so far.
>>
>>>
>>> Finally, when touching such code it would be nice if u<N> was converted to
>>> uint<N>_t.
>>>
>>>> --- a/xen/common/sched/core.c
>>>> +++ b/xen/common/sched/core.c
>>>> @@ -360,7 +360,7 @@ static always_inline void sched_spin_lock_double(
>>>> {
>>>> *flags = _spin_lock_irqsave(lock1);
>>>> }
>>>> - else if ( lock1 < lock2 )
>>>> + else if ( (uintptr_t)lock1 < (uintptr_t)lock2 )
Could we assume that these 'lock1' and 'lock2' pointers belong to the
same allocation region - 'sched_resource' ?
Dmytro.
>>>
>>> Similarly, no matter what C or Misra may have to say here, imo such casts
>>> are
>>> simply dangerous. Especially when open-coded.
>> Well, this function 'sched_spin_lock_double' has the following description:
>> "If locks are different, take the one with the lower address first."
>>
>> I think it's save to compare the integer representations of 'lock1' and
>> 'lock2' addresses explicitly (casting the pointers values to an integer
>> type). We need to find the 'lower address'.
>> Any risks here?
>>
>> Dmytro
>>>
>>>> --- a/xen/common/virtual_region.c
>>>> +++ b/xen/common/virtual_region.c
>>>> @@ -83,8 +83,8 @@ const struct virtual_region *find_text_region(unsigned
>>>> long addr)
>>>> rcu_read_lock(&rcu_virtual_region_lock);
>>>> list_for_each_entry_rcu ( iter, &virtual_region_list, list )
>>>> {
>>>> - if ( (void *)addr >= iter->text_start &&
>>>> - (void *)addr < iter->text_end )
>>>> + if ( addr >= (unsigned long)iter->text_start &&
>>>> + addr < (unsigned long)iter->text_end )
>>>
>>> Considering further casts to unsigned long of the same struct fields, was it
>>> considered to alter the type of the struct fields instead?
>> There are present casts of struct fields 'text_start' and 'text_end' in
>> the file 'xen/common/virtual_region.c'.
>>
>> modify_xen_mappings_lite((unsigned long)region->text_start,
>> (unsigned long)region->text_end,
>> PAGE_HYPERVISOR_RWX);
>>
>> Changing fields type to 'unsigned long' will give the opportunity to
>> remove casts from source code (mentioned before),
>> and remove '(void*)' casts from here:
>>
>> if ( (void *)addr >= iter->text_start &&
>> (void *)addr < iter->text_end )
>>
>> Unfortunately there are places where these fields are initialized, so
>> cast to the 'unsigned long' should be there.
>> Example:
>> .text_start = _sinittext,
>> .text_end = _einittext,
>> and
>> .text_start = _sinittext,
>> .text_end = _einittext,
>>
>> where
>> extern char _sinittext[], _einittext[];
>> extern char _stext[], _etext[];
>>
>
> Everything related to stext/etext, sinittext/einittext, etc should be
> deviated as those are not even pointers: they are linker symbols. Also,
> they do refer to the same "object": the Xen text.
