struct mctelem_cookie missing definition

struct mctelem_cookie missing definition

[Stefano Stabellini]

Hi all,

I am looking through the few remaining MISRA violations that we have
left.  One of them is R11.2:

https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}

Specifically, mctelem_cookie_t is a pointer to incomplete type and
therefore COOKIE2MCTE triggers a "conversion between a pointer to an
incomplete type and any other type".

mctelem_cookie_t is defined as:

typedef struct mctelem_cookie *mctelem_cookie_t;

I am looking through the code and I genuinely cannot find the definition
of struct mctelem_cookie.

If mctelem_cookie_t is only used as a pointer, wouldn't it make more
sense to do:

typedef struct mctelem_ent *mctelem_cookie_t;

?

What am I missing?

Cheers,

Stefano

Re: struct mctelem_cookie missing definition

[Andrew Cooper]

On 13/02/2025 1:25 am, Stefano Stabellini wrote:
> Hi all,
>
> I am looking through the few remaining MISRA violations that we have
> left.  One of them is R11.2:
>
> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
>
> Specifically, mctelem_cookie_t is a pointer to incomplete type and
> therefore COOKIE2MCTE triggers a "conversion between a pointer to an
> incomplete type and any other type".
>
> mctelem_cookie_t is defined as:
>
> typedef struct mctelem_cookie *mctelem_cookie_t;
>
> I am looking through the code and I genuinely cannot find the definition
> of struct mctelem_cookie.
>
> If mctelem_cookie_t is only used as a pointer, wouldn't it make more
> sense to do:
>
> typedef struct mctelem_ent *mctelem_cookie_t;
>
> ?
>
> What am I missing?

Nothing.  Or perhaps the twisted thinking of the original author.

It is genuinely a pointer type (== known size) which you can't deference
(because there is no definition), and can only operate on by casting to
an integer.  Except the code also requires it to be a uint64_t which is
why there's some fun disabling of relevant hypercalls for compat guests.

If someone could find the time to file it in /dev/null and replace it
with literally anything else, I'd be very thankful.

~Andrew

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1810 bytes --]

On Thu, 13 Feb 2025, Andrew Cooper wrote:
> On 13/02/2025 1:25 am, Stefano Stabellini wrote:
> > Hi all,
> >
> > I am looking through the few remaining MISRA violations that we have
> > left.  One of them is R11.2:
> >
> > https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
> >
> > Specifically, mctelem_cookie_t is a pointer to incomplete type and
> > therefore COOKIE2MCTE triggers a "conversion between a pointer to an
> > incomplete type and any other type".
> >
> > mctelem_cookie_t is defined as:
> >
> > typedef struct mctelem_cookie *mctelem_cookie_t;
> >
> > I am looking through the code and I genuinely cannot find the definition
> > of struct mctelem_cookie.
> >
> > If mctelem_cookie_t is only used as a pointer, wouldn't it make more
> > sense to do:
> >
> > typedef struct mctelem_ent *mctelem_cookie_t;
> >
> > ?
> >
> > What am I missing?
> 
> Nothing.  Or perhaps the twisted thinking of the original author.
> 
> It is genuinely a pointer type (== known size) which you can't deference
> (because there is no definition), and can only operate on by casting to
> an integer.  Except the code also requires it to be a uint64_t which is
> why there's some fun disabling of relevant hypercalls for compat guests.
> 
> If someone could find the time to file it in /dev/null and replace it
> with literally anything else, I'd be very thankful.

Are you OK with typedefing mctelem_cookie_t to uint64_t instead?

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

On Wed, 12 Feb 2025, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Andrew Cooper wrote:
> > On 13/02/2025 1:25 am, Stefano Stabellini wrote:
> > > Hi all,
> > >
> > > I am looking through the few remaining MISRA violations that we have
> > > left.  One of them is R11.2:
> > >
> > > https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
> > >
> > > Specifically, mctelem_cookie_t is a pointer to incomplete type and
> > > therefore COOKIE2MCTE triggers a "conversion between a pointer to an
> > > incomplete type and any other type".
> > >
> > > mctelem_cookie_t is defined as:
> > >
> > > typedef struct mctelem_cookie *mctelem_cookie_t;
> > >
> > > I am looking through the code and I genuinely cannot find the definition
> > > of struct mctelem_cookie.
> > >
> > > If mctelem_cookie_t is only used as a pointer, wouldn't it make more
> > > sense to do:
> > >
> > > typedef struct mctelem_ent *mctelem_cookie_t;
> > >
> > > ?
> > >
> > > What am I missing?
> > 
> > Nothing.  Or perhaps the twisted thinking of the original author.
> > 
> > It is genuinely a pointer type (== known size) which you can't deference
> > (because there is no definition), and can only operate on by casting to
> > an integer.  Except the code also requires it to be a uint64_t which is
> > why there's some fun disabling of relevant hypercalls for compat guests.
> > 
> > If someone could find the time to file it in /dev/null and replace it
> > with literally anything else, I'd be very thankful.
> 
> Are you OK with typedefing mctelem_cookie_t to uint64_t instead?

I confirm that the following resolves the MISRA violations

diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
index f4c5ff848d..2ccd490e5d 100644
--- a/xen/arch/x86/cpu/mcheck/mctelem.h
+++ b/xen/arch/x86/cpu/mcheck/mctelem.h
@@ -52,7 +52,7 @@
  * the element from the processing list.
  */
 
-typedef struct mctelem_cookie *mctelem_cookie_t;
+typedef uint64_t *mctelem_cookie_t;
 
 typedef enum mctelem_class {
     MC_URGENT,

Re: struct mctelem_cookie missing definition

[Jan Beulich]

On 13.02.2025 03:00, Stefano Stabellini wrote:
> On Wed, 12 Feb 2025, Stefano Stabellini wrote:
>> On Thu, 13 Feb 2025, Andrew Cooper wrote:
>>> On 13/02/2025 1:25 am, Stefano Stabellini wrote:
>>>> Hi all,
>>>>
>>>> I am looking through the few remaining MISRA violations that we have
>>>> left.  One of them is R11.2:
>>>>
>>>> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
>>>>
>>>> Specifically, mctelem_cookie_t is a pointer to incomplete type and
>>>> therefore COOKIE2MCTE triggers a "conversion between a pointer to an
>>>> incomplete type and any other type".
>>>>
>>>> mctelem_cookie_t is defined as:
>>>>
>>>> typedef struct mctelem_cookie *mctelem_cookie_t;
>>>>
>>>> I am looking through the code and I genuinely cannot find the definition
>>>> of struct mctelem_cookie.
>>>>
>>>> If mctelem_cookie_t is only used as a pointer, wouldn't it make more
>>>> sense to do:
>>>>
>>>> typedef struct mctelem_ent *mctelem_cookie_t;
>>>>
>>>> ?
>>>>
>>>> What am I missing?
>>>
>>> Nothing.  Or perhaps the twisted thinking of the original author.
>>>
>>> It is genuinely a pointer type (== known size) which you can't deference
>>> (because there is no definition), and can only operate on by casting to
>>> an integer.  Except the code also requires it to be a uint64_t which is
>>> why there's some fun disabling of relevant hypercalls for compat guests.
>>>
>>> If someone could find the time to file it in /dev/null and replace it
>>> with literally anything else, I'd be very thankful.
>>
>> Are you OK with typedefing mctelem_cookie_t to uint64_t instead?
> 
> I confirm that the following resolves the MISRA violations
> 
> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
> index f4c5ff848d..2ccd490e5d 100644
> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
> @@ -52,7 +52,7 @@
>   * the element from the processing list.
>   */
>  
> -typedef struct mctelem_cookie *mctelem_cookie_t;
> +typedef uint64_t *mctelem_cookie_t;

Yet that makes it possible to de-reference the pointer. Which, as Andrew
explained, is intended to be impossible. If this could be properly
replaced (not exactly what Andrew indicated by "file it in /dev/null"),
fine. Truly purging the code (i.e. as Andrew suggests) may still be an
option, with appropriate justification. But simply adjusting the type
and then moving on is too little, imo. Even if you used void * (to make
de-referencing impossible) I'd view it as largely papering over an issue;
then converting to other pointers (without explicit cast, and hence
without making apparent the badness of doing so) would become possible.

Jan

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 3826 bytes --]

On Thu, 13 Feb 2025, Jan Beulich wrote:
> On 13.02.2025 03:00, Stefano Stabellini wrote:
> > On Wed, 12 Feb 2025, Stefano Stabellini wrote:
> >> On Thu, 13 Feb 2025, Andrew Cooper wrote:
> >>> On 13/02/2025 1:25 am, Stefano Stabellini wrote:
> >>>> Hi all,
> >>>>
> >>>> I am looking through the few remaining MISRA violations that we have
> >>>> left.  One of them is R11.2:
> >>>>
> >>>> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
> >>>>
> >>>> Specifically, mctelem_cookie_t is a pointer to incomplete type and
> >>>> therefore COOKIE2MCTE triggers a "conversion between a pointer to an
> >>>> incomplete type and any other type".
> >>>>
> >>>> mctelem_cookie_t is defined as:
> >>>>
> >>>> typedef struct mctelem_cookie *mctelem_cookie_t;
> >>>>
> >>>> I am looking through the code and I genuinely cannot find the definition
> >>>> of struct mctelem_cookie.
> >>>>
> >>>> If mctelem_cookie_t is only used as a pointer, wouldn't it make more
> >>>> sense to do:
> >>>>
> >>>> typedef struct mctelem_ent *mctelem_cookie_t;
> >>>>
> >>>> ?
> >>>>
> >>>> What am I missing?
> >>>
> >>> Nothing.  Or perhaps the twisted thinking of the original author.
> >>>
> >>> It is genuinely a pointer type (== known size) which you can't deference
> >>> (because there is no definition), and can only operate on by casting to
> >>> an integer.  Except the code also requires it to be a uint64_t which is
> >>> why there's some fun disabling of relevant hypercalls for compat guests.
> >>>
> >>> If someone could find the time to file it in /dev/null and replace it
> >>> with literally anything else, I'd be very thankful.
> >>
> >> Are you OK with typedefing mctelem_cookie_t to uint64_t instead?
> > 
> > I confirm that the following resolves the MISRA violations
> > 
> > diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
> > index f4c5ff848d..2ccd490e5d 100644
> > --- a/xen/arch/x86/cpu/mcheck/mctelem.h
> > +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
> > @@ -52,7 +52,7 @@
> >   * the element from the processing list.
> >   */
> >  
> > -typedef struct mctelem_cookie *mctelem_cookie_t;
> > +typedef uint64_t *mctelem_cookie_t;
> 
> Yet that makes it possible to de-reference the pointer. Which, as Andrew
> explained, is intended to be impossible. If this could be properly
> replaced (not exactly what Andrew indicated by "file it in /dev/null"),
> fine. Truly purging the code (i.e. as Andrew suggests) may still be an
> option, with appropriate justification. But simply adjusting the type
> and then moving on is too little, imo. Even if you used void * (to make
> de-referencing impossible) I'd view it as largely papering over an issue;
> then converting to other pointers (without explicit cast, and hence
> without making apparent the badness of doing so) would become possible.

What about converting to uintptr_t (not a pointer)?


In general, there are quite a few MISRA rules that we could mark as
blocking (clean) in our GitLab scan with just a few code changes like
this one. My goal is to make these rules blocking as soon as possible.
If I can improve the code in the process, that is even better, but it is
not mandatory. And I would rather spend one more hour marking a second
rule as blocking instead. 

What I mean is that I believe it would be acceptable to make some
compromises and accept non-perfect changes to the code if it helps us
enforce more rules as blocking in GitLab CI.

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

On Thu, 13 Feb 2025, Stefano Stabellini wrote:
> > > diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
> > > index f4c5ff848d..2ccd490e5d 100644
> > > --- a/xen/arch/x86/cpu/mcheck/mctelem.h
> > > +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
> > > @@ -52,7 +52,7 @@
> > >   * the element from the processing list.
> > >   */
> > >  
> > > -typedef struct mctelem_cookie *mctelem_cookie_t;
> > > +typedef uint64_t *mctelem_cookie_t;
> > 
> > Yet that makes it possible to de-reference the pointer. Which, as Andrew
> > explained, is intended to be impossible. If this could be properly
> > replaced (not exactly what Andrew indicated by "file it in /dev/null"),
> > fine. Truly purging the code (i.e. as Andrew suggests) may still be an
> > option, with appropriate justification. But simply adjusting the type
> > and then moving on is too little, imo. Even if you used void * (to make
> > de-referencing impossible) I'd view it as largely papering over an issue;
> > then converting to other pointers (without explicit cast, and hence
> > without making apparent the badness of doing so) would become possible.
> 
> What about converting to uintptr_t (not a pointer)?
> 
> 
> In general, there are quite a few MISRA rules that we could mark as
> blocking (clean) in our GitLab scan with just a few code changes like
> this one. My goal is to make these rules blocking as soon as possible.
> If I can improve the code in the process, that is even better, but it is
> not mandatory. And I would rather spend one more hour marking a second
> rule as blocking instead. 
> 
> What I mean is that I believe it would be acceptable to make some
> compromises and accept non-perfect changes to the code if it helps us
> enforce more rules as blocking in GitLab CI.

After briefly speaking with Andrew about this, and re-reading Jan's
email above, I think it is best to resolve this as a deviation.

Would this deviation work for you? Please suggest a better wording if
you prefer.

Nicola, in reality I think it would be better to use deviations.rst
because the SAF comment below would need to be replicated at every call
side, if I am not mistaken.


diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index b8a4f878ea..d9fbe959d1 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -92,6 +92,14 @@
         },
         {
             "id": "SAF-11-safe",
+            "analyser": {
+                "eclair": "MC3A2.R11.2"
+            },
+            "name": "Rule 11.2: purposely impossible to dereference",
+            "text": "Certain pointers points to incomplete types purposely so that they are impossible to dereference."
+        },
+        {
+            "id": "SAF-12-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
index f4c5ff848d..e845360c7d 100644
--- a/xen/arch/x86/cpu/mcheck/mctelem.h
+++ b/xen/arch/x86/cpu/mcheck/mctelem.h
@@ -52,6 +52,7 @@
  * the element from the processing list.
  */
 
+/* SAF-11-safe: impossible to dereference */
 typedef struct mctelem_cookie *mctelem_cookie_t;
 
 typedef enum mctelem_class {

Re: struct mctelem_cookie missing definition

[Jan Beulich]

On 14.02.2025 04:00, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Stefano Stabellini wrote:
>>>> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
>>>> index f4c5ff848d..2ccd490e5d 100644
>>>> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
>>>> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
>>>> @@ -52,7 +52,7 @@
>>>>   * the element from the processing list.
>>>>   */
>>>>  
>>>> -typedef struct mctelem_cookie *mctelem_cookie_t;
>>>> +typedef uint64_t *mctelem_cookie_t;
>>>
>>> Yet that makes it possible to de-reference the pointer. Which, as Andrew
>>> explained, is intended to be impossible. If this could be properly
>>> replaced (not exactly what Andrew indicated by "file it in /dev/null"),
>>> fine. Truly purging the code (i.e. as Andrew suggests) may still be an
>>> option, with appropriate justification. But simply adjusting the type
>>> and then moving on is too little, imo. Even if you used void * (to make
>>> de-referencing impossible) I'd view it as largely papering over an issue;
>>> then converting to other pointers (without explicit cast, and hence
>>> without making apparent the badness of doing so) would become possible.
>>
>> What about converting to uintptr_t (not a pointer)?
>>
>>
>> In general, there are quite a few MISRA rules that we could mark as
>> blocking (clean) in our GitLab scan with just a few code changes like
>> this one. My goal is to make these rules blocking as soon as possible.
>> If I can improve the code in the process, that is even better, but it is
>> not mandatory. And I would rather spend one more hour marking a second
>> rule as blocking instead. 
>>
>> What I mean is that I believe it would be acceptable to make some
>> compromises and accept non-perfect changes to the code if it helps us
>> enforce more rules as blocking in GitLab CI.
> 
> After briefly speaking with Andrew about this, and re-reading Jan's
> email above, I think it is best to resolve this as a deviation.
> 
> Would this deviation work for you? Please suggest a better wording if
> you prefer.

Sounds reasonable to me; one nit below.

> Nicola, in reality I think it would be better to use deviations.rst
> because the SAF comment below would need to be replicated at every call
> side, if I am not mistaken.

If replication indeed would be needed, I agree doing it the other way
might be better.

> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -92,6 +92,14 @@
>          },
>          {
>              "id": "SAF-11-safe",
> +            "analyser": {
> +                "eclair": "MC3A2.R11.2"
> +            },
> +            "name": "Rule 11.2: purposely impossible to dereference",
> +            "text": "Certain pointers points to incomplete types purposely so that they are impossible to dereference."

Nit: s/ points / point /

Jan

Re: struct mctelem_cookie missing definition

[Nicola Vetrini]

On 2025-02-14 04:00, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Stefano Stabellini wrote:
>> > > diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
>> > > index f4c5ff848d..2ccd490e5d 100644
>> > > --- a/xen/arch/x86/cpu/mcheck/mctelem.h
>> > > +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
>> > > @@ -52,7 +52,7 @@
>> > >   * the element from the processing list.
>> > >   */
>> > >
>> > > -typedef struct mctelem_cookie *mctelem_cookie_t;
>> > > +typedef uint64_t *mctelem_cookie_t;
>> >
>> > Yet that makes it possible to de-reference the pointer. Which, as Andrew
>> > explained, is intended to be impossible. If this could be properly
>> > replaced (not exactly what Andrew indicated by "file it in /dev/null"),
>> > fine. Truly purging the code (i.e. as Andrew suggests) may still be an
>> > option, with appropriate justification. But simply adjusting the type
>> > and then moving on is too little, imo. Even if you used void * (to make
>> > de-referencing impossible) I'd view it as largely papering over an issue;
>> > then converting to other pointers (without explicit cast, and hence
>> > without making apparent the badness of doing so) would become possible.
>> 
>> What about converting to uintptr_t (not a pointer)?
>> 
>> 
>> In general, there are quite a few MISRA rules that we could mark as
>> blocking (clean) in our GitLab scan with just a few code changes like
>> this one. My goal is to make these rules blocking as soon as possible.
>> If I can improve the code in the process, that is even better, but it 
>> is
>> not mandatory. And I would rather spend one more hour marking a second
>> rule as blocking instead.
>> 
>> What I mean is that I believe it would be acceptable to make some
>> compromises and accept non-perfect changes to the code if it helps us
>> enforce more rules as blocking in GitLab CI.
> 
> After briefly speaking with Andrew about this, and re-reading Jan's
> email above, I think it is best to resolve this as a deviation.
> 
> Would this deviation work for you? Please suggest a better wording if
> you prefer.
> 
> Nicola, in reality I think it would be better to use deviations.rst
> because the SAF comment below would need to be replicated at every call
> side, if I am not mistaken.
> 

Would deviating macros "COOKIE2MCTE" and "MCTE2COOKIE" work?
In that case, that is a simple configuration tweak which then will be 
justified in deviations.rst.

Thanks,
  Nicola

> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index b8a4f878ea..d9fbe959d1 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -92,6 +92,14 @@
>          },
>          {
>              "id": "SAF-11-safe",
> +            "analyser": {
> +                "eclair": "MC3A2.R11.2"
> +            },
> +            "name": "Rule 11.2: purposely impossible to dereference",
> +            "text": "Certain pointers points to incomplete types 
> purposely so that they are impossible to dereference."
> +        },
> +        {
> +            "id": "SAF-12-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h 
> b/xen/arch/x86/cpu/mcheck/mctelem.h
> index f4c5ff848d..e845360c7d 100644
> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
> @@ -52,6 +52,7 @@
>   * the element from the processing list.
>   */
> 
> +/* SAF-11-safe: impossible to dereference */
>  typedef struct mctelem_cookie *mctelem_cookie_t;
> 
>  typedef enum mctelem_class {

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: struct mctelem_cookie missing definition

[Jan Beulich]

On 14.02.2025 08:46, Nicola Vetrini wrote:
> On 2025-02-14 04:00, Stefano Stabellini wrote:
>> On Thu, 13 Feb 2025, Stefano Stabellini wrote:
>>>>> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
>>>>> index f4c5ff848d..2ccd490e5d 100644
>>>>> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
>>>>> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
>>>>> @@ -52,7 +52,7 @@
>>>>>   * the element from the processing list.
>>>>>   */
>>>>>
>>>>> -typedef struct mctelem_cookie *mctelem_cookie_t;
>>>>> +typedef uint64_t *mctelem_cookie_t;
>>>>
>>>> Yet that makes it possible to de-reference the pointer. Which, as Andrew
>>>> explained, is intended to be impossible. If this could be properly
>>>> replaced (not exactly what Andrew indicated by "file it in /dev/null"),
>>>> fine. Truly purging the code (i.e. as Andrew suggests) may still be an
>>>> option, with appropriate justification. But simply adjusting the type
>>>> and then moving on is too little, imo. Even if you used void * (to make
>>>> de-referencing impossible) I'd view it as largely papering over an issue;
>>>> then converting to other pointers (without explicit cast, and hence
>>>> without making apparent the badness of doing so) would become possible.
>>>
>>> What about converting to uintptr_t (not a pointer)?
>>>
>>>
>>> In general, there are quite a few MISRA rules that we could mark as
>>> blocking (clean) in our GitLab scan with just a few code changes like
>>> this one. My goal is to make these rules blocking as soon as possible.
>>> If I can improve the code in the process, that is even better, but it 
>>> is
>>> not mandatory. And I would rather spend one more hour marking a second
>>> rule as blocking instead.
>>>
>>> What I mean is that I believe it would be acceptable to make some
>>> compromises and accept non-perfect changes to the code if it helps us
>>> enforce more rules as blocking in GitLab CI.
>>
>> After briefly speaking with Andrew about this, and re-reading Jan's
>> email above, I think it is best to resolve this as a deviation.
>>
>> Would this deviation work for you? Please suggest a better wording if
>> you prefer.
>>
>> Nicola, in reality I think it would be better to use deviations.rst
>> because the SAF comment below would need to be replicated at every call
>> side, if I am not mistaken.
>>
> 
> Would deviating macros "COOKIE2MCTE" and "MCTE2COOKIE" work?

If it did, COOKIE2ID and ID2COOKIE would likely need including as well.

Jan

> In that case, that is a simple configuration tweak which then will be 
> justified in deviations.rst.
> 
> Thanks,
>   Nicola

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

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@amd.com>

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$))))"}
+}
+-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..f3004d6023 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -324,6 +324,12 @@ 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.
+     - Tagged as `deliberate` for ECLAIR. Such pointer is struct
+       mctelem_cookie \*.
+
    * - R11.2
      - The conversion from a pointer to an incomplete type to unsigned long
        does not lose any information, provided that the target type has enough

Re: struct mctelem_cookie missing definition

[Nicola Vetrini]

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@amd.com>
> 
> 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.

> +-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..f3004d6023 100644
> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -324,6 +324,12 @@ 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.
> +     - Tagged as `deliberate` for ECLAIR. Such pointer is struct
> +       mctelem_cookie \*.
> +

I think here (or above in the deviation text) the concern raised by the 
rationale of the rule should be addressed; namely, the rule states: 
"Conversions shall not be performed between a pointer to an incomplete 
type and any other type" because the resulting pointer might be 
misaligned, therefore there should be an argument as to why such thing 
is not possible.

>     * - R11.2
>       - The conversion from a pointer to an incomplete type to unsigned 
> long
>         does not lose any information, provided that the target type 
> has enough

Thanks,
  Nicola

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: struct mctelem_cookie missing definition

[Jan Beulich]

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@amd.com>
>>
>> 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))

Arguable that's ...

>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -324,6 +324,12 @@ 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.
>> +     - Tagged as `deliberate` for ECLAIR. Such pointer is struct
>> +       mctelem_cookie \*.
>> +
> 
> I think here (or above in the deviation text) the concern raised by the 
> rationale of the rule should be addressed; namely, the rule states: 
> "Conversions shall not be performed between a pointer to an incomplete 
> type and any other type" because the resulting pointer might be 
> misaligned, therefore there should be an argument as to why such thing 
> is not possible.

... undermining this rationale of the rule then, though.

Jan

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

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@amd.com>
> >>
> >> 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))
> 
> Arguable that's ...

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.


> >> --- a/docs/misra/deviations.rst
> >> +++ b/docs/misra/deviations.rst
> >> @@ -324,6 +324,12 @@ 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.
> >> +     - Tagged as `deliberate` for ECLAIR. Such pointer is struct
> >> +       mctelem_cookie \*.
> >> +
> > 
> > I think here (or above in the deviation text) the concern raised by the 
> > rationale of the rule should be addressed; namely, the rule states: 
> > "Conversions shall not be performed between a pointer to an incomplete 
> > type and any other type" because the resulting pointer might be 
> > misaligned, therefore there should be an argument as to why such thing 
> > is not possible.

I think the explanation would be that it is OK to have misaligned pointers
because they cannot be dereferenced by design.

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

On Mon, 16 Feb 2025, 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@amd.com>
> > >>
> > >> 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))
> > 
> > Arguable that's ...
> 
> 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.


To expand on this, I believe the deviation was specifically intended for
conversions leading to integers, rather than pointers that could
potentially have alignment issues. This is why the original deviation
made sense, and expanding its scope to include COOKIE2MCTE and
MCTE2COOKIE may not be feasible. 

Re: struct mctelem_cookie missing definition

[Jan Beulich]

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@amd.com>
>>>>
>>>> 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.

Jan

Re: struct mctelem_cookie missing definition

[Stefano Stabellini]

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@amd.com>
> >>>>
> >>>> 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@amd.com>

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.
+     - Tagged as `deliberate` for ECLAIR. Such pointer is struct
+       mctelem_cookie \*.
+
    * - R11.2
      - The conversion from a pointer to an incomplete type to unsigned long
        does not lose any information, provided that the target type has enough

Re: struct mctelem_cookie missing definition

[Jan Beulich]

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@amd.com>
>>>>>>
>>>>>> 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@amd.com>
> 
> 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

Re: struct mctelem_cookie missing definition

[Nicola Vetrini]

On 2025-02-18 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@amd.com>
>> >>>>
>> >>>> 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}]}}}
> 

The reason is quite simple: the deviation is for casts from any type to 
unsigned long. It would need a similar configuration 
from(type(canonical(builtin(unsigned long)))) to(any()) in order to 
catch those.

> 
> 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@amd.com>
> 
> 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.
> +     - Tagged as `deliberate` for ECLAIR. Such pointer is struct
> +       mctelem_cookie \*.
> +
>     * - R11.2
>       - The conversion from a pointer to an incomplete type to unsigned 
> long
>         does not lose any information, provided that the target type 
> has enough

-- 
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253

Re: struct mctelem_cookie missing definition

[Jan Beulich]

On 13.02.2025 22:47, Stefano Stabellini wrote:
> On Thu, 13 Feb 2025, Jan Beulich wrote:
>> On 13.02.2025 03:00, Stefano Stabellini wrote:
>>> On Wed, 12 Feb 2025, Stefano Stabellini wrote:
>>>> On Thu, 13 Feb 2025, Andrew Cooper wrote:
>>>>> On 13/02/2025 1:25 am, Stefano Stabellini wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> I am looking through the few remaining MISRA violations that we have
>>>>>> left.  One of them is R11.2:
>>>>>>
>>>>>> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
>>>>>>
>>>>>> Specifically, mctelem_cookie_t is a pointer to incomplete type and
>>>>>> therefore COOKIE2MCTE triggers a "conversion between a pointer to an
>>>>>> incomplete type and any other type".
>>>>>>
>>>>>> mctelem_cookie_t is defined as:
>>>>>>
>>>>>> typedef struct mctelem_cookie *mctelem_cookie_t;
>>>>>>
>>>>>> I am looking through the code and I genuinely cannot find the definition
>>>>>> of struct mctelem_cookie.
>>>>>>
>>>>>> If mctelem_cookie_t is only used as a pointer, wouldn't it make more
>>>>>> sense to do:
>>>>>>
>>>>>> typedef struct mctelem_ent *mctelem_cookie_t;
>>>>>>
>>>>>> ?
>>>>>>
>>>>>> What am I missing?
>>>>>
>>>>> Nothing.  Or perhaps the twisted thinking of the original author.
>>>>>
>>>>> It is genuinely a pointer type (== known size) which you can't deference
>>>>> (because there is no definition), and can only operate on by casting to
>>>>> an integer.  Except the code also requires it to be a uint64_t which is
>>>>> why there's some fun disabling of relevant hypercalls for compat guests.
>>>>>
>>>>> If someone could find the time to file it in /dev/null and replace it
>>>>> with literally anything else, I'd be very thankful.
>>>>
>>>> Are you OK with typedefing mctelem_cookie_t to uint64_t instead?
>>>
>>> I confirm that the following resolves the MISRA violations
>>>
>>> diff --git a/xen/arch/x86/cpu/mcheck/mctelem.h b/xen/arch/x86/cpu/mcheck/mctelem.h
>>> index f4c5ff848d..2ccd490e5d 100644
>>> --- a/xen/arch/x86/cpu/mcheck/mctelem.h
>>> +++ b/xen/arch/x86/cpu/mcheck/mctelem.h
>>> @@ -52,7 +52,7 @@
>>>   * the element from the processing list.
>>>   */
>>>  
>>> -typedef struct mctelem_cookie *mctelem_cookie_t;
>>> +typedef uint64_t *mctelem_cookie_t;
>>
>> Yet that makes it possible to de-reference the pointer. Which, as Andrew
>> explained, is intended to be impossible. If this could be properly
>> replaced (not exactly what Andrew indicated by "file it in /dev/null"),
>> fine. Truly purging the code (i.e. as Andrew suggests) may still be an
>> option, with appropriate justification. But simply adjusting the type
>> and then moving on is too little, imo. Even if you used void * (to make
>> de-referencing impossible) I'd view it as largely papering over an issue;
>> then converting to other pointers (without explicit cast, and hence
>> without making apparent the badness of doing so) would become possible.
> 
> What about converting to uintptr_t (not a pointer)?

That'll lose type checking the compiler does. A type-safe wrapper struct
(like we have for mfn_t and alike in debug builds) may do.

Jan

Re: struct mctelem_cookie missing definition

[Jan Beulich]

On 13.02.2025 02:34, Andrew Cooper wrote:
> On 13/02/2025 1:25 am, Stefano Stabellini wrote:
>> Hi all,
>>
>> I am looking through the few remaining MISRA violations that we have
>> left.  One of them is R11.2:
>>
>> https://saas.eclairit.com:3787/fs/var/local/eclair/xen-project.ecdf/xen-project/hardware/xen/ECLAIR_normal/staging/X86_64/9118578464/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}]}}}
>>
>> Specifically, mctelem_cookie_t is a pointer to incomplete type and
>> therefore COOKIE2MCTE triggers a "conversion between a pointer to an
>> incomplete type and any other type".
>>
>> mctelem_cookie_t is defined as:
>>
>> typedef struct mctelem_cookie *mctelem_cookie_t;
>>
>> I am looking through the code and I genuinely cannot find the definition
>> of struct mctelem_cookie.
>>
>> If mctelem_cookie_t is only used as a pointer, wouldn't it make more
>> sense to do:
>>
>> typedef struct mctelem_ent *mctelem_cookie_t;
>>
>> ?
>>
>> What am I missing?
> 
> Nothing.  Or perhaps the twisted thinking of the original author.
> 
> It is genuinely a pointer type (== known size) which you can't deference
> (because there is no definition), and can only operate on by casting to
> an integer.  Except the code also requires it to be a uint64_t which is
> why there's some fun disabling of relevant hypercalls for compat guests.

That "fun disabling" is for the COMPAT=n case afaics, not for compat guests.
Or else I screwed up in d23d792478db.

Jan