[XEN PATCH v3 0/2] xen: address violations of MISRA C Rule 17.1

[XEN PATCH v3 0/2] xen: address violations of MISRA C Rule 17.1

[Simone Ballarin]

MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used".

The Xen community wants to avoid using variadic functions except for
specific circumstances where it feels appropriate by strict code review.

Functions hypercall_create_continuation and hypercall_xlat_continuation
are internal helpers made to break long running hypercalls into multiple
calls. They take a variable number of arguments depending on the original
hypercall they are trying to continue. Add SAF deviations for the aforementioned
functions.

Add deviation for printf()-like functions.

---
Changes in v3:
- use regexes to exempt all .*printk and .*printf functions, instead
  of manually listing them one by one;
- rebase: change SAF-3-safe in SAF-4-safe.
Changes in v2:
- replace "related to console output" with "printf()-like functions";
- replace "special hypercalls" with "internal helpers".

Simone Ballarin (2):
  MISRA C:2012 Rule 17.1 states: The features of `<stdarg.h>' shall not
    be used
  MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be
    used"

 automation/eclair_analysis/ECLAIR/deviations.ecl | 13 +++++++++++++
 docs/misra/deviations.rst                        |  5 +++++
 docs/misra/safe.json                             |  8 ++++++++
 xen/arch/arm/domain.c                            |  1 +
 xen/arch/x86/hypercall.c                         |  2 ++
 5 files changed, 29 insertions(+)

-- 
2.34.1

[XEN PATCH v3 1/2] MISRA C:2012 Rule 17.1 states: The features of `<stdarg.h>' shall not be used

[Simone Ballarin]

The Xen community wants to avoid using variadic functions except for
specific circumstances where it feels appropriate by strict code review.

Add deviation for printf()-like functions.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v3:
- use regex to exempt all .*printk and .*printf functions, instead
  of manually listing them one by one.
Changes in v2:
- replace "console output related" with "printf()-like".
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 13 +++++++++++++
 docs/misra/deviations.rst                        |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index de9ba723fb..936c738caf 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -386,6 +386,19 @@ explicit comment indicating the fallthrough intention is present."
 -config=MC3R1.R16.6,switch_clauses+={deliberate, "default(0)"}
 -doc_end
 
+#
+# Series 17.
+#
+
+-doc_begin="printf()-like functions are allowed to use the variadic features provided by stdarg.h."
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printk\(.*\)$)))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\(.*\)$)))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(panic)&&kind(function))))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(elf_call_log_callback)&&kind(function))))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(vprintk_common)&&kind(function))))"}
+-config=MC3R1.R17.1,macros+={hide , "^va_(arg|start|copy|end)$"}
+-doc_end
+
 #
 # Series 18.
 #
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index eb5ef2bd9d..dd254a9640 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -334,6 +334,11 @@ Deviations related to MISRA C:2012 Rules:
        improve readability.
      - Tagged as `deliberate` for ECLAIR.
 
+   * - R17.1
+     - printf()-like functions  are allowed to use the variadic features provided
+       by `stdarg.h`.
+     - Tagged as `deliberate` for ECLAIR.
+
    * - R20.4
      - The override of the keyword \"inline\" in xen/compiler.h is present so
        that section contents checks pass when the compiler chooses not to
-- 
2.34.1

Re: [XEN PATCH v3 1/2] MISRA C:2012 Rule 17.1 states: The features of `<stdarg.h>' shall not be used

[Stefano Stabellini]

On Thu, 28 Mar 2024, Simone Ballarin wrote:
> The Xen community wants to avoid using variadic functions except for
> specific circumstances where it feels appropriate by strict code review.
> 
> Add deviation for printf()-like functions.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

[XEN PATCH v3 2/2] MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used"

[Simone Ballarin]

The Xen community wants to avoid using variadic functions except for
specific circumstances where it feels appropriate by strict code review.

Functions hypercall_create_continuation and hypercall_xlat_continuation
are internal helper functions made to break long running hypercalls into
multiple calls. They take a variable number of arguments depending on the
original hypercall they are trying to continue.

Add SAF deviations for the aforementioned functions.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v3:
- rebase: change SAF-3-safe to SAF-4-safe.
Changes in v2:
- replaced "special hypercalls" with "internal helper functions".
---
 docs/misra/safe.json     | 8 ++++++++
 xen/arch/arm/domain.c    | 1 +
 xen/arch/x86/hypercall.c | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index d361d0e65c..fe2bc18509 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -36,6 +36,14 @@
         },
         {
             "id": "SAF-4-safe",
+            "analyser": {
+                "eclair": "MC3R1.R17.1"
+            },
+            "name": "Rule 17.1: internal helper functions made to break long running hypercalls into multiple calls.",
+            "text": "They need to take a variable number of arguments depending on the original hypercall they are trying to continue."
+        },
+        {
+            "id": "SAF-5-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index f38cb5e04c..34cbfe699a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -382,6 +382,7 @@ unsigned long hypercall_create_continuation(
     const char *p = format;
     unsigned long arg, rc;
     unsigned int i;
+    /* SAF-4-safe allowed variadic function */
     va_list args;
 
     current->hcall_preempted = true;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index 01cd73040d..133e9f221c 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -31,6 +31,7 @@ unsigned long hypercall_create_continuation(
     const char *p = format;
     unsigned long arg;
     unsigned int i;
+    /* SAF-4-safe allowed variadic function */
     va_list args;
 
     curr->hcall_preempted = true;
@@ -115,6 +116,7 @@ int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
     struct cpu_user_regs *regs;
     unsigned int i, cval = 0;
     unsigned long nval = 0;
+    /* SAF-4-safe allowed variadic function */
     va_list args;
 
     ASSERT(nr <= ARRAY_SIZE(mcs->call.args));
-- 
2.34.1

Re: [XEN PATCH v3 2/2] MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used"

[Jan Beulich]

On 28.03.2024 11:29, Simone Ballarin wrote:
> The Xen community wants to avoid using variadic functions except for
> specific circumstances where it feels appropriate by strict code review.

In the title, s/20.7/17.1/ I suppose?

Jan

> Functions hypercall_create_continuation and hypercall_xlat_continuation
> are internal helper functions made to break long running hypercalls into
> multiple calls. They take a variable number of arguments depending on the
> original hypercall they are trying to continue.
> 
> Add SAF deviations for the aforementioned functions.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> Changes in v3:
> - rebase: change SAF-3-safe to SAF-4-safe.
> Changes in v2:
> - replaced "special hypercalls" with "internal helper functions".
> ---
>  docs/misra/safe.json     | 8 ++++++++
>  xen/arch/arm/domain.c    | 1 +
>  xen/arch/x86/hypercall.c | 2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index d361d0e65c..fe2bc18509 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -36,6 +36,14 @@
>          },
>          {
>              "id": "SAF-4-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R17.1"
> +            },
> +            "name": "Rule 17.1: internal helper functions made to break long running hypercalls into multiple calls.",
> +            "text": "They need to take a variable number of arguments depending on the original hypercall they are trying to continue."
> +        },
> +        {
> +            "id": "SAF-5-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index f38cb5e04c..34cbfe699a 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -382,6 +382,7 @@ unsigned long hypercall_create_continuation(
>      const char *p = format;
>      unsigned long arg, rc;
>      unsigned int i;
> +    /* SAF-4-safe allowed variadic function */
>      va_list args;
>  
>      current->hcall_preempted = true;
> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
> index 01cd73040d..133e9f221c 100644
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -31,6 +31,7 @@ unsigned long hypercall_create_continuation(
>      const char *p = format;
>      unsigned long arg;
>      unsigned int i;
> +    /* SAF-4-safe allowed variadic function */
>      va_list args;
>  
>      curr->hcall_preempted = true;
> @@ -115,6 +116,7 @@ int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
>      struct cpu_user_regs *regs;
>      unsigned int i, cval = 0;
>      unsigned long nval = 0;
> +    /* SAF-4-safe allowed variadic function */
>      va_list args;
>  
>      ASSERT(nr <= ARRAY_SIZE(mcs->call.args));

Re: [XEN PATCH v3 2/2] MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used"

[Simone Ballarin]

On 28/03/24 11:31, Jan Beulich wrote:
> On 28.03.2024 11:29, Simone Ballarin wrote:
>> The Xen community wants to avoid using variadic functions except for
>> specific circumstances where it feels appropriate by strict code review.
> 
> In the title, s/20.7/17.1/ I suppose?
> 
> Jan
> 
>> Functions hypercall_create_continuation and hypercall_xlat_continuation
>> are internal helper functions made to break long running hypercalls into
>> multiple calls. They take a variable number of arguments depending on the
>> original hypercall they are trying to continue.
>>
>> Add SAF deviations for the aforementioned functions.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> ---
>> Changes in v3:
>> - rebase: change SAF-3-safe to SAF-4-safe.
>> Changes in v2:
>> - replaced "special hypercalls" with "internal helper functions".
>> ---
>>   docs/misra/safe.json     | 8 ++++++++
>>   xen/arch/arm/domain.c    | 1 +
>>   xen/arch/x86/hypercall.c | 2 ++
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
>> index d361d0e65c..fe2bc18509 100644
>> --- a/docs/misra/safe.json
>> +++ b/docs/misra/safe.json
>> @@ -36,6 +36,14 @@
>>           },
>>           {
>>               "id": "SAF-4-safe",
>> +            "analyser": {
>> +                "eclair": "MC3R1.R17.1"
>> +            },
>> +            "name": "Rule 17.1: internal helper functions made to break long running hypercalls into multiple calls.",
>> +            "text": "They need to take a variable number of arguments depending on the original hypercall they are trying to continue."
>> +        },
>> +        {
>> +            "id": "SAF-5-safe",
>>               "analyser": {},
>>               "name": "Sentinel",
>>               "text": "Next ID to be used"
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index f38cb5e04c..34cbfe699a 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -382,6 +382,7 @@ unsigned long hypercall_create_continuation(
>>       const char *p = format;
>>       unsigned long arg, rc;
>>       unsigned int i;
>> +    /* SAF-4-safe allowed variadic function */
>>       va_list args;
>>   
>>       current->hcall_preempted = true;
>> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
>> index 01cd73040d..133e9f221c 100644
>> --- a/xen/arch/x86/hypercall.c
>> +++ b/xen/arch/x86/hypercall.c
>> @@ -31,6 +31,7 @@ unsigned long hypercall_create_continuation(
>>       const char *p = format;
>>       unsigned long arg;
>>       unsigned int i;
>> +    /* SAF-4-safe allowed variadic function */
>>       va_list args;
>>   
>>       curr->hcall_preempted = true;
>> @@ -115,6 +116,7 @@ int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
>>       struct cpu_user_regs *regs;
>>       unsigned int i, cval = 0;
>>       unsigned long nval = 0;
>> +    /* SAF-4-safe allowed variadic function */
>>       va_list args;
>>   
>>       ASSERT(nr <= ARRAY_SIZE(mcs->call.args));
> 
> 

Yes, sorry.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v3 2/2] MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used"

[Stefano Stabellini]

On Thu, 28 Mar 2024, Simone Ballarin wrote:
> The Xen community wants to avoid using variadic functions except for
> specific circumstances where it feels appropriate by strict code review.
> 
> Functions hypercall_create_continuation and hypercall_xlat_continuation
> are internal helper functions made to break long running hypercalls into
> multiple calls. They take a variable number of arguments depending on the
> original hypercall they are trying to continue.
> 
> Add SAF deviations for the aforementioned functions.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>


Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I fixed the title on commit


> ---
> Changes in v3:
> - rebase: change SAF-3-safe to SAF-4-safe.
> Changes in v2:
> - replaced "special hypercalls" with "internal helper functions".
> ---
>  docs/misra/safe.json     | 8 ++++++++
>  xen/arch/arm/domain.c    | 1 +
>  xen/arch/x86/hypercall.c | 2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index d361d0e65c..fe2bc18509 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -36,6 +36,14 @@
>          },
>          {
>              "id": "SAF-4-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R17.1"
> +            },
> +            "name": "Rule 17.1: internal helper functions made to break long running hypercalls into multiple calls.",
> +            "text": "They need to take a variable number of arguments depending on the original hypercall they are trying to continue."
> +        },
> +        {
> +            "id": "SAF-5-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index f38cb5e04c..34cbfe699a 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -382,6 +382,7 @@ unsigned long hypercall_create_continuation(
>      const char *p = format;
>      unsigned long arg, rc;
>      unsigned int i;
> +    /* SAF-4-safe allowed variadic function */
>      va_list args;
>  
>      current->hcall_preempted = true;
> diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
> index 01cd73040d..133e9f221c 100644
> --- a/xen/arch/x86/hypercall.c
> +++ b/xen/arch/x86/hypercall.c
> @@ -31,6 +31,7 @@ unsigned long hypercall_create_continuation(
>      const char *p = format;
>      unsigned long arg;
>      unsigned int i;
> +    /* SAF-4-safe allowed variadic function */
>      va_list args;
>  
>      curr->hcall_preempted = true;
> @@ -115,6 +116,7 @@ int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
>      struct cpu_user_regs *regs;
>      unsigned int i, cval = 0;
>      unsigned long nval = 0;
> +    /* SAF-4-safe allowed variadic function */
>      va_list args;
>  
>      ASSERT(nr <= ARRAY_SIZE(mcs->call.args));
> -- 
> 2.34.1
> 

Re: [XEN PATCH v3 2/2] MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used"

[Jan Beulich]

On 05.04.2024 02:27, Stefano Stabellini wrote:
> On Thu, 28 Mar 2024, Simone Ballarin wrote:
>> The Xen community wants to avoid using variadic functions except for
>> specific circumstances where it feels appropriate by strict code review.
>>
>> Functions hypercall_create_continuation and hypercall_xlat_continuation
>> are internal helper functions made to break long running hypercalls into
>> multiple calls. They take a variable number of arguments depending on the
>> original hypercall they are trying to continue.
>>
>> Add SAF deviations for the aforementioned functions.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> I fixed the title on commit

Did you forget to push then? I don't see this change in the tree, yet.

Jan

Re: [XEN PATCH v3 2/2] MISRA C Rule 20.7 states: "The features of `<stdarg.h>' shall not be used"

[Stefano Stabellini]

On Fri, 5 Apr 2024, Jan Beulich wrote:
> On 05.04.2024 02:27, Stefano Stabellini wrote:
> > On Thu, 28 Mar 2024, Simone Ballarin wrote:
> >> The Xen community wants to avoid using variadic functions except for
> >> specific circumstances where it feels appropriate by strict code review.
> >>
> >> Functions hypercall_create_continuation and hypercall_xlat_continuation
> >> are internal helper functions made to break long running hypercalls into
> >> multiple calls. They take a variable number of arguments depending on the
> >> original hypercall they are trying to continue.
> >>
> >> Add SAF deviations for the aforementioned functions.
> >>
> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> > 
> > 
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > I fixed the title on commit
> 
> Did you forget to push then? I don't see this change in the tree, yet.

Yep, thanks!