[v2] misra: add deviation for MISRA C Rule R11.8.

[PATCH v2] misra: add deviation for MISRA C Rule R11.8.

Posted by Alessandro Zucchelli 1 year, 1 month ago

Rule 11.8 states as following: "A cast shall not remove any `const' or
`volatile' qualification from the type pointed to by a pointer".

Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use
function, where the parameter needs to not be const because it can be
set for write or not. As it was decided a new const-only function will
lead to more developer confusion than it's worth, this violation is
addressed by deviating the function.
All cases of casting away const-ness are accompanied with a comment
explaining why it is safe given the other flags passed in; such comment is used
by the deviation in order to match the appropriate function call.

No functional change.

Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
---
As this patch introduces a deviation for service MC3A2.R11.8, it
depends on the following patch and shall not be applied prior to its
application.
https://lore.kernel.org/xen-devel/cf13be4779f15620e94b99b3b91f9cb040319989.1733826952.git.alessandro.zucchelli@bugseng.com/T/#u

Changes from V1:
The deviation has been refined to specify that every instance of casting away
const-ness is accompanied by a comment explaining why it is safe.
This comment is a requirement that has been incorporated into the text defining
the deviation.
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 2f58f29203..c9d06b44f4 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -393,6 +393,12 @@ Fixing this violation would require to increase code complexity and lower readab
 -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"}
 -doc_end
 
+-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a double-use
+function, where the parameter needs to not be const because it can be set for
+write or not"
+-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest doesn't modify.*$)))"}
+-doc_end
+
 -doc_begin="This construct is used to check if the type is scalar, and for this purpose the use of 0 as a null pointer constant is deliberate."
 -config=MC3R1.R11.9,reports+={deliberate, "any_area(any_loc(any_exp(macro(^__ACCESS_ONCE$))))"
 }
-- 
2.43.0

Re: [PATCH v2] misra: add deviation for MISRA C Rule R11.8.

Posted by Jan Beulich 1 year, 1 month ago

On 18.12.2024 15:25, Alessandro Zucchelli wrote:
> Rule 11.8 states as following: "A cast shall not remove any `const' or
> `volatile' qualification from the type pointed to by a pointer".
> 
> Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use
> function, where the parameter needs to not be const because it can be
> set for write or not. As it was decided a new const-only function will
> lead to more developer confusion than it's worth, this violation is
> addressed by deviating the function.
> All cases of casting away const-ness are accompanied with a comment
> explaining why it is safe given the other flags passed in; such comment is used
> by the deviation in order to match the appropriate function call.
> 
> No functional change.
> 
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> ---
> As this patch introduces a deviation for service MC3A2.R11.8, it
> depends on the following patch and shall not be applied prior to its
> application.
> https://lore.kernel.org/xen-devel/cf13be4779f15620e94b99b3b91f9cb040319989.1733826952.git.alessandro.zucchelli@bugseng.com/T/#u

This wasn't applicable anymore at the time you posted the patch, I suppose?

> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -393,6 +393,12 @@ Fixing this violation would require to increase code complexity and lower readab
>  -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"}
>  -doc_end
>  
> +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a double-use
> +function, where the parameter needs to not be const because it can be set for
> +write or not"
> +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest doesn't modify.*$)))"}

This is probably good enough for now, yet still: It constrains re-formatting
that we may want to do on such function calls. Personally I'd consider it
entirely unexpected if I ended up (re)introducing a violation just by re-
formatting one of those function calls to

    return __hvm_copy(
               (void *)buf /* HVMCOPY_to_guest doesn't modify */,
               addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear,
               PFEC_page_present | PFEC_write_access | pfec, pfinfo);

yet aiui the pattern above would have this effect (I don't think .* matches
newlines; instead I expect such regex-es to be applied to individual lines
only). Thoughts anyone?

Jan

Re: [PATCH v2] misra: add deviation for MISRA C Rule R11.8.

Posted by Nicola Vetrini 1 year, 1 month ago

On 2024-12-19 09:49, Jan Beulich wrote:
> On 18.12.2024 15:25, Alessandro Zucchelli wrote:
>> Rule 11.8 states as following: "A cast shall not remove any `const' or
>> `volatile' qualification from the type pointed to by a pointer".
>> 
>> Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use
>> function, where the parameter needs to not be const because it can be
>> set for write or not. As it was decided a new const-only function will
>> lead to more developer confusion than it's worth, this violation is
>> addressed by deviating the function.
>> All cases of casting away const-ness are accompanied with a comment
>> explaining why it is safe given the other flags passed in; such 
>> comment is used
>> by the deviation in order to match the appropriate function call.
>> 
>> No functional change.
>> 
>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>> ---

>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -393,6 +393,12 @@ Fixing this violation would require to increase 
>> code complexity and lower readab
>>  
>> -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"}
>>  -doc_end
>> 
>> +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a 
>> double-use
>> +function, where the parameter needs to not be const because it can be 
>> set for
>> +write or not"
>> +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest 
>> doesn't modify.*$)))"}
> 
> This is probably good enough for now, yet still: It constrains 
> re-formatting
> that we may want to do on such function calls. Personally I'd consider 
> it
> entirely unexpected if I ended up (re)introducing a violation just by 
> re-
> formatting one of those function calls to
> 
>     return __hvm_copy(
>                (void *)buf /* HVMCOPY_to_guest doesn't modify */,
>                addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear,
>                PFEC_page_present | PFEC_write_access | pfec, pfinfo);
> 
> yet aiui the pattern above would have this effect (I don't think .* 
> matches
> newlines; instead I expect such regex-es to be applied to individual 
> lines
> only). Thoughts anyone?
> 

Hi Jan,

we can simply drop the "__hvm_copy" part from the regex. The regex can 
be made multiline, or alternatively we can apply the search to a range 
of lines. By default it searches on the same location mentioned by the 
report, which in this case is the line containing __hvm_copy (range 
defaults to 0..0). However I would leave it either as is or without the 
__hvm_copy prefix.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Re: [PATCH v2] misra: add deviation for MISRA C Rule R11.8.

Posted by Jan Beulich 1 year, 1 month ago

On 19.12.2024 09:58, Nicola Vetrini wrote:
> On 2024-12-19 09:49, Jan Beulich wrote:
>> On 18.12.2024 15:25, Alessandro Zucchelli wrote:
>>> Rule 11.8 states as following: "A cast shall not remove any `const' or
>>> `volatile' qualification from the type pointed to by a pointer".
>>>
>>> Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use
>>> function, where the parameter needs to not be const because it can be
>>> set for write or not. As it was decided a new const-only function will
>>> lead to more developer confusion than it's worth, this violation is
>>> addressed by deviating the function.
>>> All cases of casting away const-ness are accompanied with a comment
>>> explaining why it is safe given the other flags passed in; such 
>>> comment is used
>>> by the deviation in order to match the appropriate function call.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
>>> ---
> 
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -393,6 +393,12 @@ Fixing this violation would require to increase 
>>> code complexity and lower readab
>>>  
>>> -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"}
>>>  -doc_end
>>>
>>> +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a 
>>> double-use
>>> +function, where the parameter needs to not be const because it can be 
>>> set for
>>> +write or not"
>>> +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest 
>>> doesn't modify.*$)))"}
>>
>> This is probably good enough for now, yet still: It constrains 
>> re-formatting
>> that we may want to do on such function calls. Personally I'd consider 
>> it
>> entirely unexpected if I ended up (re)introducing a violation just by 
>> re-
>> formatting one of those function calls to
>>
>>     return __hvm_copy(
>>                (void *)buf /* HVMCOPY_to_guest doesn't modify */,
>>                addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear,
>>                PFEC_page_present | PFEC_write_access | pfec, pfinfo);
>>
>> yet aiui the pattern above would have this effect (I don't think .* 
>> matches
>> newlines; instead I expect such regex-es to be applied to individual 
>> lines
>> only). Thoughts anyone?
> 
> we can simply drop the "__hvm_copy" part from the regex. The regex can 
> be made multiline, or alternatively we can apply the search to a range 
> of lines. By default it searches on the same location mentioned by the 
> report, which in this case is the line containing __hvm_copy (range 
> defaults to 0..0). However I would leave it either as is or without the 
> __hvm_copy prefix.

Omitting the __hvm_copy part would again widen it too much for my taste.

Jan

Re: [PATCH v2] misra: add deviation for MISRA C Rule R11.8.

Posted by Stefano Stabellini 1 year, 1 month ago

On Thu, 19 Dec 2024, Jan Beulich wrote:
> On 19.12.2024 09:58, Nicola Vetrini wrote:
> > On 2024-12-19 09:49, Jan Beulich wrote:
> >> On 18.12.2024 15:25, Alessandro Zucchelli wrote:
> >>> Rule 11.8 states as following: "A cast shall not remove any `const' or
> >>> `volatile' qualification from the type pointed to by a pointer".
> >>>
> >>> Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use
> >>> function, where the parameter needs to not be const because it can be
> >>> set for write or not. As it was decided a new const-only function will
> >>> lead to more developer confusion than it's worth, this violation is
> >>> addressed by deviating the function.
> >>> All cases of casting away const-ness are accompanied with a comment
> >>> explaining why it is safe given the other flags passed in; such 
> >>> comment is used
> >>> by the deviation in order to match the appropriate function call.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>
> >>> ---
> > 
> >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> >>> @@ -393,6 +393,12 @@ Fixing this violation would require to increase 
> >>> code complexity and lower readab
> >>>  
> >>> -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"}
> >>>  -doc_end
> >>>
> >>> +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a 
> >>> double-use
> >>> +function, where the parameter needs to not be const because it can be 
> >>> set for
> >>> +write or not"
> >>> +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest 
> >>> doesn't modify.*$)))"}
> >>
> >> This is probably good enough for now, yet still: It constrains 
> >> re-formatting
> >> that we may want to do on such function calls. Personally I'd consider 
> >> it
> >> entirely unexpected if I ended up (re)introducing a violation just by 
> >> re-
> >> formatting one of those function calls to
> >>
> >>     return __hvm_copy(
> >>                (void *)buf /* HVMCOPY_to_guest doesn't modify */,
> >>                addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear,
> >>                PFEC_page_present | PFEC_write_access | pfec, pfinfo);
> >>
> >> yet aiui the pattern above would have this effect (I don't think .* 
> >> matches
> >> newlines; instead I expect such regex-es to be applied to individual 
> >> lines
> >> only). Thoughts anyone?
> > 
> > we can simply drop the "__hvm_copy" part from the regex. The regex can 
> > be made multiline, or alternatively we can apply the search to a range 
> > of lines. By default it searches on the same location mentioned by the 
> > report, which in this case is the line containing __hvm_copy (range 
> > defaults to 0..0). However I would leave it either as is or without the 
> > __hvm_copy prefix.
> 
> Omitting the __hvm_copy part would again widen it too much for my taste.

I am also OK with the change as it is. However, I would ask that we also
update docs/misra/deviations.rst with the same