Re: struct mctelem_cookie missing definition

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.02.2025 22:37, Stefano Stabellini wrote:
> On Tue, 18 Feb 2025, Jan Beulich wrote:
>> On 18.02.2025 03:45, Stefano Stabellini wrote:
>>> On Mon, 17 Feb 2025, Jan Beulich wrote:
>>>> On 15.02.2025 09:59, Nicola Vetrini wrote:
>>>>> On 2025-02-15 00:04, Stefano Stabellini wrote:
>>>>>> On Fri, 14 Feb 2025, Jan Beulich wrote:
>>>>>>>> Would deviating macros "COOKIE2MCTE" and "MCTE2COOKIE" work?
>>>>>>>
>>>>>>> If it did, COOKIE2ID and ID2COOKIE would likely need including as 
>>>>>>> well.
>>>>>>
>>>>>> I wrote this patch. Nicola, can you please check the changes to
>>>>>> deviations.ecl, this is the first time I try to write the deviation
>>>>>> myself :-)
>>>>>>
>>>>>> ---
>>>>>> misra: add 11.2 deviation for incomplete types
>>>>>>
>>>>>> struct mctelem_cookie* is used exactly because it is an incomplete type
>>>>>> so the pointer cannot be dereferenced. This is deliberate. So add a
>>>>>> deviation for it.
>>>>>>
>>>>>> In deviations.ecl, add the list of macros that do the conversions to 
>>>>>> and
>>>>>> from struct mctelem_cookie*.
>>>>>>
>>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
>>>>>>
>>>>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>>>>>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> index a28eb0ae76..87bfd2160c 100644
>>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> @@ -366,6 +366,14 @@ constant expressions are required.\""
>>>>>>  }
>>>>>>  -doc_end
>>>>>>
>>>>>> +-doc_begin="Certain pointers point to incomplete types purposely so 
>>>>>> that they are impossible to dereference."
>>>>>> +-config=MC3A2.R11.2,reports+={deliberate, 
>>>>>> "any_area(any_loc(any_exp(macro(^COOKIE2MCTE$))))"}
>>>>>> +-config=MC3A2.R11.2,reports+={deliberate, 
>>>>>> "any_area(any_loc(any_exp(macro(^MCTE2COOKIE$))))"}
>>>>>> +-config=MC3A2.R11.2,reports+={deliberate, 
>>>>>> "any_area(any_loc(any_exp(macro(^COOKIE2ID$))))"}
>>>>>> +-config=MC3A2.R11.2,reports+={deliberate, 
>>>>>> "any_area(any_loc(any_exp(macro(^ID2COOKIE$))))"}
>>>>>> +}
>>>>>
>>>>> -config=MC3A2.R11.2,reports+={deliberate, 
>>>>> "any_area(any_loc(any_exp(macro(name(COOKIE2MCTE||MCTE2COOKIE||COOKIE2ID||ID2COOKIE)))))"}
>>>>>
>>>>> Note however that there is also this deviation in place
>>>>>
>>>>> -doc_begin="The conversion from a pointer to an incomplete type to 
>>>>> unsigned long does not lose any information, provided that the target 
>>>>> type has enough bits to store it."
>>>>> -config=MC3A2.R11.2,casts+={safe,
>>>>>    "from(type(any()))
>>>>>     &&to(type(canonical(builtin(unsigned long))))
>>>>>     &&relation(definitely_preserves_value)"
>>>>> }
>>>>> -doc_end
>>>>>
>>>>> I was a bit confused by Jan's remark, which seemed correct, but I 
>>>>> couldn't see any violations in the report, so I dug a bit deeper. 
>>>>> ID2COOKIE and COOKIE2ID, which operate to/from unsigned long are already 
>>>>> excluded due to being safe. If you envision those macros to be used with 
>>>>> other types, then your deviation should mention them, otherwise they are 
>>>>> already handled.
>>>>
>>>> Yet then can't we leverage that deviation to also make the other two
>>>> covered:
>>>>
>>>> #define    COOKIE2MCTE(c)          ((struct mctelem_ent *)(unsigned 
>>>> long)(c))
>>>> #define    MCTE2COOKIE(tep)        ((mctelem_cookie_t)(unsigned long)(tep))
>>>
>>> Jan is asking why ID2COOKIE and COOKIE2ID are considered safe, while
>>> COOKIE2MCTE and MCTE2COOKIE are not. I think the reason is that
>>> ID2COOKIE and COOKIE2ID convert to/from unsigned long and that falls
>>> under the other deviation we already have:
>>>
>>> -doc_begin="The conversion from a pointer to an incomplete type to 
>>> unsigned long does not lose any information, provided that the target 
>>> type has enough bits to store it."
>>> -config=MC3A2.R11.2,casts+={safe,
>>>    "from(type(any()))
>>>     &&to(type(canonical(builtin(unsigned long))))
>>>     &&relation(definitely_preserves_value)"
>>>
>>> On the other hand COOKIE2MCTE and MCTE2COOKIE convert to/from another
>>> pointer type, so they don't fall under the same deviation.
>>
>> And then the adjusted forms suggested above ought to also be covered,
>> I would have thought.
> 
> I understand your point. I tried it, but it does not work. I do not know
> why. Someone with more knowledge of ECLAIR internals than I have might
> be able to explain.
> 
> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/people/sstabellini/xen/ECLAIR_normal/my-eclair-11.2-4-1/X86_64/9176469474/PROJECT.ecd;/by_service/MC3A2.R11.2.html#{%22select%22:true,%22selection%22:{%22hiddenAreaKinds%22:[],%22hiddenSubareaKinds%22:[],%22show%22:false,%22selector%22:{%22enabled%22:true,%22negated%22:true,%22kind%22:0,%22domain%22:%22kind%22,%22inputs%22:[{%22enabled%22:true,%22text%22:%22violation%22}]}}}
> 
> 
> I suggest we go with this patch instead.
> 
> ---
> misra: add 11.2 deviation for incomplete types
> 
> struct mctelem_cookie* is used exactly because it is an incomplete type
> so the pointer cannot be dereferenced. This is deliberate. So add a
> deviation for it.
> 
> In deviations.ecl, add the list of macros that do the conversions to and
> from struct mctelem_cookie*.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index a28eb0ae76..d33b777e6a 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -366,6 +366,10 @@ constant expressions are required.\""
>  }
>  -doc_end
>  
> +-doc_begin="Certain pointers point to incomplete types purposely so that 
> they are impossible to dereference."
> +-config=MC3A2.R11.2,reports+={deliberate, 
> "any_area(any_loc(any_exp(macro(name(COOKIE2MCTE||MCTE2COOKIE||COOKIE2ID||ID2COOKIE)))))"}
> +-doc_end
> +
>  -doc_begin="Conversions to object pointers that have a pointee type with a 
> smaller (i.e., less strict) alignment requirement are safe."
>  -config=MC3A2.R11.3,casts+={safe,
>    "!relation(more_aligned_pointee)"
> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> index fe0b1e10a2..04ffc62f44 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -324,6 +324,13 @@ Deviations related to MISRA C:2012 Rules:
>         semantics that do not lead to unexpected behaviour.
>       - Tagged as `safe` for ECLAIR.
>  
> +   * - R11.2
> +     - Certain pointers point to incomplete types purposely so that they
> +       are impossible to dereference, since they cannot be dereferenced,
> +       pointers alignments considerations do not apply.

But that's not true for COOKIE2MCTE(). Its result _is_ being dereferenced.
(Note how in an earlier reply, where I suggested intermediately casting to
unsigned long, I also said that this would be "undermining this rationale
of the rule then, though." The same would apply to putting in place a
deviation, imo.) In fact, looking e.g. at just mctelem_defer(),
mctelem_dataptr(), mctelem_dismiss(), mctelem_commit(), and
mctelem_consume_oldest_end() it's not clear how that's safe to do. For
every one of them it requires looking at all their call sites. And imo
it's the result of doing so which would form the justification.

The only one where just looking at the function using the macro is
sufficient to see things are kind of okay is mctelem_ack(). The argument
for being safe here is that the pointer first is checked against a value
we stored earlier.

For mctelem_consume_oldest_end() analysis is also reasonably easy: It's
only ever passed the return value from an earlier
mctelem_consume_oldest_begin(). In fact I question the need for going
through the cookie type here. I guess I'll make a patch to remove the
macro uses here, reducing the scope of the Misra task at least a little.

Jan