misra: address Rule 11.1 violation in cmpxchgptr()

[PATCH] misra: address Rule 11.1 violation in cmpxchgptr()

Posted by Dmytro Prokopchuk1 2 months, 2 weeks ago

Misra Rule 11.1 states: "Conversions shall not be performed between a
pointer to a function and any other type."

The violation occurs in the macro:
    __typeof__(**(ptr)) *const o_ = (o);                                \
    __typeof__(**(ptr)) *n_ = (n);                                      \
    ((__typeof__(*(ptr)))__cmpxchg(ptr, (unsigned long)o_,              \
                                   (unsigned long)n_, sizeof(*(ptr)))); \
})
when it is used for handling function pointers of type:
typedef void (*)(struct vcpu *, unsigned int).
The issue happens because the '__cmpxchg()' function returns an 'unsigned
long', which is then converted back into a function pointer, causing a
violation of Rule 11.1. In this particular usage, the return value of the
macro 'cmpxchgptr()' is not required. To address the violation, the macro
has been replaced to discard the return value of '__cmpxchg()', preventing
the conversion.

Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
---
Probably separate macro is too much for this single case.

And the following will be enought:
__cmpxchg(&xen_consumers[i], (unsigned long)NULL, (unsigned long)fn, sizeof(*(&xen_consumers[i])));
---
 xen/common/event_channel.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 67700b050a..2094338b28 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -93,6 +93,17 @@ static void cf_check default_xen_notification_fn(
         vcpu_wake(v);
 }
 
+/*
+ * A slightly more updated variant of cmpxchgptr() where old value
+ * is not returned.
+ */
+#define cmpxchgptr_noret(ptr, o, n) ({                  \
+    __typeof__(**(ptr)) *const o_ = (o);                \
+    __typeof__(**(ptr)) *n_ = (n);                      \
+    (void)__cmpxchg(ptr, (unsigned long)o_,             \
+                    (unsigned long)n_, sizeof(*(ptr))); \
+})
+
 /*
  * Given a notification function, return the value to stash in
  * the evtchn->xen_consumer field.
@@ -106,9 +117,9 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn)
 
     for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
     {
-        /* Use cmpxchgptr() in lieu of a global lock. */
+        /* Use cmpxchgptr_noret() in lieu of a global lock. */
         if ( xen_consumers[i] == NULL )
-            cmpxchgptr(&xen_consumers[i], NULL, fn);
+            cmpxchgptr_noret(&xen_consumers[i], NULL, fn);
         if ( xen_consumers[i] == fn )
             break;
     }
-- 
2.43.0

Re: [PATCH] misra: address Rule 11.1 violation in cmpxchgptr()

Posted by Teddy Astie 2 months, 2 weeks ago

Hello,

Le 13/08/2025 à 20:07, Dmytro Prokopchuk1 a écrit :
> Misra Rule 11.1 states: "Conversions shall not be performed between a
> pointer to a function and any other type."
> 
> The violation occurs in the macro:
>      __typeof__(**(ptr)) *const o_ = (o);                                \
>      __typeof__(**(ptr)) *n_ = (n);                                      \
>      ((__typeof__(*(ptr)))__cmpxchg(ptr, (unsigned long)o_,              \
>                                     (unsigned long)n_, sizeof(*(ptr)))); \
> })
> when it is used for handling function pointers of type:
> typedef void (*)(struct vcpu *, unsigned int).
> The issue happens because the '__cmpxchg()' function returns an 'unsigned
> long', which is then converted back into a function pointer, causing a
> violation of Rule 11.1. In this particular usage, the return value of the
> macro 'cmpxchgptr()' is not required. To address the violation, the macro
> has been replaced to discard the return value of '__cmpxchg()', preventing
> the conversion.
> 
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> ---
> Probably separate macro is too much for this single case.
> 
> And the following will be enought:
> __cmpxchg(&xen_consumers[i], (unsigned long)NULL, (unsigned long)fn, sizeof(*(&xen_consumers[i])));
> ---
>   xen/common/event_channel.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 67700b050a..2094338b28 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -93,6 +93,17 @@ static void cf_check default_xen_notification_fn(
>           vcpu_wake(v);
>   }
>   
> +/*
> + * A slightly more updated variant of cmpxchgptr() where old value
> + * is not returned.
> + */
> +#define cmpxchgptr_noret(ptr, o, n) ({                  \
> +    __typeof__(**(ptr)) *const o_ = (o);                \
> +    __typeof__(**(ptr)) *n_ = (n);                      \
> +    (void)__cmpxchg(ptr, (unsigned long)o_,             \
> +                    (unsigned long)n_, sizeof(*(ptr))); \
> +})
> +
>   /*
>    * Given a notification function, return the value to stash in
>    * the evtchn->xen_consumer field.
> @@ -106,9 +117,9 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn)
>   
>       for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
>       {
> -        /* Use cmpxchgptr() in lieu of a global lock. */
> +        /* Use cmpxchgptr_noret() in lieu of a global lock. */
>           if ( xen_consumers[i] == NULL )
> -            cmpxchgptr(&xen_consumers[i], NULL, fn);
> +            cmpxchgptr_noret(&xen_consumers[i], NULL, fn);
>           if ( xen_consumers[i] == fn )
>               break;
>       }

AFAICS, Rule 11.1 has a deviation which allows this specific case.

In docs/misra/deviations.rst
> * - R11.1
>   - The conversion from a function pointer to unsigned long or (void \*) does
>     not lose any information, provided that the target type has enough bits
>     to store it.
>   - Tagged as `safe` for ECLAIR.

Here, we are constructing a function pointer from a unsigned long. I 
assume this rule goes the other way it says, and allow converting a 
unsigned long into a function pointer as long as its value is a valid 
function pointer.

Teddy


Teddy Astie | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH] misra: address Rule 11.1 violation in cmpxchgptr()

Posted by Stefano Stabellini 2 months, 2 weeks ago

On Thu, 14 Aug 2025, Teddy Astie wrote:
> Hello,
> 
> Le 13/08/2025 à 20:07, Dmytro Prokopchuk1 a écrit :
> > Misra Rule 11.1 states: "Conversions shall not be performed between a
> > pointer to a function and any other type."
> >
> > The violation occurs in the macro:
> >      __typeof__(**(ptr)) *const o_ = (o);                                \
> >      __typeof__(**(ptr)) *n_ = (n);                                      \
> >      ((__typeof__(*(ptr)))__cmpxchg(ptr, (unsigned long)o_,              \
> >                                     (unsigned long)n_, sizeof(*(ptr)))); \
> > })
> > when it is used for handling function pointers of type:
> > typedef void (*)(struct vcpu *, unsigned int).
> > The issue happens because the '__cmpxchg()' function returns an 'unsigned
> > long', which is then converted back into a function pointer, causing a
> > violation of Rule 11.1. In this particular usage, the return value of the
> > macro 'cmpxchgptr()' is not required. To address the violation, the macro
> > has been replaced to discard the return value of '__cmpxchg()', preventing
> > the conversion.
> >
> > Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@epam.com>
> > ---
> > Probably separate macro is too much for this single case.
> >
> > And the following will be enought:
> > __cmpxchg(&xen_consumers[i], (unsigned long)NULL, (unsigned long)fn, sizeof(*(&xen_consumers[i])));
> > ---
> >   xen/common/event_channel.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index 67700b050a..2094338b28 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -93,6 +93,17 @@ static void cf_check default_xen_notification_fn(
> >           vcpu_wake(v);
> >   }
> >
> > +/*
> > + * A slightly more updated variant of cmpxchgptr() where old value
> > + * is not returned.
> > + */
> > +#define cmpxchgptr_noret(ptr, o, n) ({                  \
> > +    __typeof__(**(ptr)) *const o_ = (o);                \
> > +    __typeof__(**(ptr)) *n_ = (n);                      \
> > +    (void)__cmpxchg(ptr, (unsigned long)o_,             \
> > +                    (unsigned long)n_, sizeof(*(ptr))); \
> > +})
> > +
> >   /*
> >    * Given a notification function, return the value to stash in
> >    * the evtchn->xen_consumer field.
> > @@ -106,9 +117,9 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn)
> >
> >       for ( i = 0; i < ARRAY_SIZE(xen_consumers); i++ )
> >       {
> > -        /* Use cmpxchgptr() in lieu of a global lock. */
> > +        /* Use cmpxchgptr_noret() in lieu of a global lock. */
> >           if ( xen_consumers[i] == NULL )
> > -            cmpxchgptr(&xen_consumers[i], NULL, fn);
> > +            cmpxchgptr_noret(&xen_consumers[i], NULL, fn);
> >           if ( xen_consumers[i] == fn )
> >               break;
> >       }
> 
> AFAICS, Rule 11.1 has a deviation which allows this specific case.
> 
> In docs/misra/deviations.rst
> > * - R11.1
> >   - The conversion from a function pointer to unsigned long or (void \*) does
> >     not lose any information, provided that the target type has enough bits
> >     to store it.
> >   - Tagged as `safe` for ECLAIR.
> 
> Here, we are constructing a function pointer from a unsigned long. I
> assume this rule goes the other way it says, and allow converting a
> unsigned long into a function pointer as long as its value is a valid
> function pointer.

You are right, we should need to update the deviation instead here:


-doc_begin="The conversion from a function pointer to unsigned long or (void *) does not lose any information, provided that the target type has enough bits to store it."
-config=MC3A2.R11.1,casts+={safe,
  "from(type(canonical(__function_pointer_types)))
   &&to(type(canonical(builtin(unsigned long)||pointer(builtin(void)))))
   &&relation(definitely_preserves_value)"
}
-doc_end