diff mbox series

[XEN,v2] x86: p2m-pod: address violation of MISRA C Rule 2.1

Message ID 43b3a42f9d323cc3f9747c56e8f59f9dffa69321.1719556140.git.nicola.vetrini@bugseng.com (mailing list archive)
State New
Headers show
Series [XEN,v2] x86: p2m-pod: address violation of MISRA C Rule 2.1 | expand

Commit Message

Nicola Vetrini June 28, 2024, 6:30 a.m. UTC
The label 'out_unmap' is only reachable after ASSERT_UNREACHABLE,
so the code below is only executed upon erroneously reaching that
program point and calling domain_crash, thus resulting in the
for loop after 'out_unmap' to become unreachable in some configurations.

This is a defensive coding measure to have a safe fallback that is
reachable in non-debug builds, and can thus be deviated with a
comment-based deviation.

No functional change.

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v2:
- rebased against current staging
---
 docs/misra/safe.json      | 8 ++++++++
 xen/arch/x86/mm/p2m-pod.c | 1 +
 2 files changed, 9 insertions(+)

Comments

Jan Beulich July 1, 2024, 8:36 a.m. UTC | #1
On 28.06.2024 08:30, Nicola Vetrini wrote:
> The label 'out_unmap' is only reachable after ASSERT_UNREACHABLE,
> so the code below is only executed upon erroneously reaching that
> program point and calling domain_crash, thus resulting in the
> for loop after 'out_unmap' to become unreachable in some configurations.

First: As you have come to be used to, briefly stating the rule itself
(rather than just its number, requiring people like me - who have not
memorized all the rule numbers - to go look up what rule this is) would
be nice.

This being about unreachable code, why are the domain_crash() not the
crucial points of "unreachability"? And even if they weren't there, why
wouldn't it be the goto or ...

> --- a/xen/arch/x86/mm/p2m-pod.c
> +++ b/xen/arch/x86/mm/p2m-pod.c
> @@ -1040,6 +1040,7 @@ out_unmap:
>       * Something went wrong, probably crashing the domain.  Unmap
>       * everything and return.
>       */
> +    /* SAF-8-safe Rule 2.1: defensive programming */
>      for ( i = 0; i < count; i++ )
>          if ( map[i] )
>              unmap_domain_page(map[i]);

... the label (just out of context) where the comment needs to go?

Jan
Nicola Vetrini Sept. 10, 2024, 8:56 a.m. UTC | #2
On 2024-07-01 10:36, Jan Beulich wrote:
> On 28.06.2024 08:30, Nicola Vetrini wrote:
>> The label 'out_unmap' is only reachable after ASSERT_UNREACHABLE,
>> so the code below is only executed upon erroneously reaching that
>> program point and calling domain_crash, thus resulting in the
>> for loop after 'out_unmap' to become unreachable in some 
>> configurations.
> 
> First: As you have come to be used to, briefly stating the rule itself
> (rather than just its number, requiring people like me - who have not
> memorized all the rule numbers - to go look up what rule this is) would
> be nice.

Sure

> 
> This being about unreachable code, why are the domain_crash() not the
> crucial points of "unreachability"? And even if they weren't there, why
> wouldn't it be the goto or ...
> 
>> --- a/xen/arch/x86/mm/p2m-pod.c
>> +++ b/xen/arch/x86/mm/p2m-pod.c
>> @@ -1040,6 +1040,7 @@ out_unmap:
>>       * Something went wrong, probably crashing the domain.  Unmap
>>       * everything and return.
>>       */
>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>      for ( i = 0; i < count; i++ )
>>          if ( map[i] )
>>              unmap_domain_page(map[i]);
> 
> ... the label (just out of context) where the comment needs to go?

Because of the way this rule's configuration work, deviations are placed 
on the construct that ends up being the target of the unreachability, 
rather than (one of) the causes of such unreachability. Putting the 
comment on the label works for ECLAIR by offsetting its target 
statement, but not for other tools afaik.
Jan Beulich Sept. 10, 2024, 9:08 a.m. UTC | #3
On 10.09.2024 10:56, Nicola Vetrini wrote:
> On 2024-07-01 10:36, Jan Beulich wrote:
>> On 28.06.2024 08:30, Nicola Vetrini wrote:
>> This being about unreachable code, why are the domain_crash() not the
>> crucial points of "unreachability"? And even if they weren't there, why
>> wouldn't it be the goto or ...
>>
>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>> @@ -1040,6 +1040,7 @@ out_unmap:
>>>       * Something went wrong, probably crashing the domain.  Unmap
>>>       * everything and return.
>>>       */
>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>>      for ( i = 0; i < count; i++ )
>>>          if ( map[i] )
>>>              unmap_domain_page(map[i]);
>>
>> ... the label (just out of context) where the comment needs to go?
> 
> Because of the way this rule's configuration work, deviations are placed 
> on the construct that ends up being the target of the unreachability, 

What's "target" here? What if this loop was removed from the function?
Then both the label and the domain_crash() invocations would still be
unreachable in debug builds. Are you telling me that this then wouldn't
be diagnosed by Eclair? Or that it would then consider the closing
figure brace of the function "the target of the unreachability"?

> rather than (one of) the causes of such unreachability. Putting the 
> comment on the label works for ECLAIR by offsetting its target 
> statement, but not for other tools afaik.

I don't recall whether I ever saw a Coverity report to this effect,
and hence I wouldn't be able to tell how that would want silencing if
so desired.

Jan
Nicola Vetrini Sept. 10, 2024, 9:53 a.m. UTC | #4
On 2024-09-10 11:08, Jan Beulich wrote:
> On 10.09.2024 10:56, Nicola Vetrini wrote:
>> On 2024-07-01 10:36, Jan Beulich wrote:
>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
>>> This being about unreachable code, why are the domain_crash() not the
>>> crucial points of "unreachability"? And even if they weren't there, 
>>> why
>>> wouldn't it be the goto or ...
>>> 
>>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>>> @@ -1040,6 +1040,7 @@ out_unmap:
>>>>       * Something went wrong, probably crashing the domain.  Unmap
>>>>       * everything and return.
>>>>       */
>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>>>      for ( i = 0; i < count; i++ )
>>>>          if ( map[i] )
>>>>              unmap_domain_page(map[i]);
>>> 
>>> ... the label (just out of context) where the comment needs to go?
>> 
>> Because of the way this rule's configuration work, deviations are 
>> placed
>> on the construct that ends up being the target of the unreachability,
> 
> What's "target" here? What if this loop was removed from the function?
> Then both the label and the domain_crash() invocations would still be
> unreachable in debug builds. Are you telling me that this then wouldn't
> be diagnosed by Eclair? Or that it would then consider the closing
> figure brace of the function "the target of the unreachability"?

Exactly, the end brace is a target to which the "function end" construct 
is associated.
It would be kind of strange, though: why not just doing "domain_crash(); 
return;" in that case?

> 
>> rather than (one of) the causes of such unreachability. Putting the
>> comment on the label works for ECLAIR by offsetting its target
>> statement, but not for other tools afaik.
> 
> I don't recall whether I ever saw a Coverity report to this effect,
> and hence I wouldn't be able to tell how that would want silencing if
> so desired.
> 
> Jan
Jan Beulich Sept. 10, 2024, 10:03 a.m. UTC | #5
On 10.09.2024 11:53, Nicola Vetrini wrote:
> On 2024-09-10 11:08, Jan Beulich wrote:
>> On 10.09.2024 10:56, Nicola Vetrini wrote:
>>> On 2024-07-01 10:36, Jan Beulich wrote:
>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
>>>> This being about unreachable code, why are the domain_crash() not the
>>>> crucial points of "unreachability"? And even if they weren't there, 
>>>> why
>>>> wouldn't it be the goto or ...
>>>>
>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
>>>>>       * Something went wrong, probably crashing the domain.  Unmap
>>>>>       * everything and return.
>>>>>       */
>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>>>>      for ( i = 0; i < count; i++ )
>>>>>          if ( map[i] )
>>>>>              unmap_domain_page(map[i]);
>>>>
>>>> ... the label (just out of context) where the comment needs to go?
>>>
>>> Because of the way this rule's configuration work, deviations are 
>>> placed
>>> on the construct that ends up being the target of the unreachability,
>>
>> What's "target" here? What if this loop was removed from the function?
>> Then both the label and the domain_crash() invocations would still be
>> unreachable in debug builds. Are you telling me that this then wouldn't
>> be diagnosed by Eclair? Or that it would then consider the closing
>> figure brace of the function "the target of the unreachability"?
> 
> Exactly, the end brace is a target to which the "function end" construct 
> is associated.
> It would be kind of strange, though: why not just doing "domain_crash(); 
> return;" in that case?

Sure, the question was theoretical. Now if "return" was used directly
there, what would then be the "target"? IOW - the more abstract question
of my earlier reply still wasn't answered.

Jan
Nicola Vetrini Sept. 10, 2024, 10:17 a.m. UTC | #6
On 2024-09-10 12:03, Jan Beulich wrote:
> On 10.09.2024 11:53, Nicola Vetrini wrote:
>> On 2024-09-10 11:08, Jan Beulich wrote:
>>> On 10.09.2024 10:56, Nicola Vetrini wrote:
>>>> On 2024-07-01 10:36, Jan Beulich wrote:
>>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
>>>>> This being about unreachable code, why are the domain_crash() not 
>>>>> the
>>>>> crucial points of "unreachability"? And even if they weren't there,
>>>>> why
>>>>> wouldn't it be the goto or ...
>>>>> 
>>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
>>>>>>       * Something went wrong, probably crashing the domain.  Unmap
>>>>>>       * everything and return.
>>>>>>       */
>>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>>>>>      for ( i = 0; i < count; i++ )
>>>>>>          if ( map[i] )
>>>>>>              unmap_domain_page(map[i]);
>>>>> 
>>>>> ... the label (just out of context) where the comment needs to go?
>>>> 
>>>> Because of the way this rule's configuration work, deviations are
>>>> placed
>>>> on the construct that ends up being the target of the 
>>>> unreachability,
>>> 
>>> What's "target" here? What if this loop was removed from the 
>>> function?
>>> Then both the label and the domain_crash() invocations would still be
>>> unreachable in debug builds. Are you telling me that this then 
>>> wouldn't
>>> be diagnosed by Eclair? Or that it would then consider the closing
>>> figure brace of the function "the target of the unreachability"?
>> 
>> Exactly, the end brace is a target to which the "function end" 
>> construct
>> is associated.
>> It would be kind of strange, though: why not just doing 
>> "domain_crash();
>> return;" in that case?
> 
> Sure, the question was theoretical. Now if "return" was used directly
> there, what would then be the "target"? IOW - the more abstract 
> question
> of my earlier reply still wasn't answered.
> 

The return statement in

...
domain_crash();
return;
<~~~~~>

Whichever statement is found to be unreachable in the current 
preprocessed code.
Jan Beulich Sept. 10, 2024, 10:36 a.m. UTC | #7
On 10.09.2024 12:17, Nicola Vetrini wrote:
> On 2024-09-10 12:03, Jan Beulich wrote:
>> On 10.09.2024 11:53, Nicola Vetrini wrote:
>>> On 2024-09-10 11:08, Jan Beulich wrote:
>>>> On 10.09.2024 10:56, Nicola Vetrini wrote:
>>>>> On 2024-07-01 10:36, Jan Beulich wrote:
>>>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
>>>>>> This being about unreachable code, why are the domain_crash() not 
>>>>>> the
>>>>>> crucial points of "unreachability"? And even if they weren't there,
>>>>>> why
>>>>>> wouldn't it be the goto or ...
>>>>>>
>>>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
>>>>>>>       * Something went wrong, probably crashing the domain.  Unmap
>>>>>>>       * everything and return.
>>>>>>>       */
>>>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>>>>>>      for ( i = 0; i < count; i++ )
>>>>>>>          if ( map[i] )
>>>>>>>              unmap_domain_page(map[i]);
>>>>>>
>>>>>> ... the label (just out of context) where the comment needs to go?
>>>>>
>>>>> Because of the way this rule's configuration work, deviations are
>>>>> placed
>>>>> on the construct that ends up being the target of the 
>>>>> unreachability,
>>>>
>>>> What's "target" here? What if this loop was removed from the 
>>>> function?
>>>> Then both the label and the domain_crash() invocations would still be
>>>> unreachable in debug builds. Are you telling me that this then 
>>>> wouldn't
>>>> be diagnosed by Eclair? Or that it would then consider the closing
>>>> figure brace of the function "the target of the unreachability"?
>>>
>>> Exactly, the end brace is a target to which the "function end" 
>>> construct
>>> is associated.
>>> It would be kind of strange, though: why not just doing 
>>> "domain_crash();
>>> return;" in that case?
>>
>> Sure, the question was theoretical. Now if "return" was used directly
>> there, what would then be the "target"? IOW - the more abstract 
>> question
>> of my earlier reply still wasn't answered.
>>
> 
> The return statement in
> 
> ...
> domain_crash();
> return;
> <~~~~~>
> 
> Whichever statement is found to be unreachable in the current 
> preprocessed code.

Yet then again: Why is it the return statement and not the function call
one (really, it being a macro invocation: the do/while one that the macro
expands to)? That's the first thing that won't be reached.

Jan
Stefano Stabellini Sept. 12, 2024, 1:05 a.m. UTC | #8
On Tue, 10 Sep 2024, Jan Beulich wrote:
> On 10.09.2024 12:17, Nicola Vetrini wrote:
> > On 2024-09-10 12:03, Jan Beulich wrote:
> >> On 10.09.2024 11:53, Nicola Vetrini wrote:
> >>> On 2024-09-10 11:08, Jan Beulich wrote:
> >>>> On 10.09.2024 10:56, Nicola Vetrini wrote:
> >>>>> On 2024-07-01 10:36, Jan Beulich wrote:
> >>>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
> >>>>>> This being about unreachable code, why are the domain_crash() not 
> >>>>>> the
> >>>>>> crucial points of "unreachability"? And even if they weren't there,
> >>>>>> why
> >>>>>> wouldn't it be the goto or ...
> >>>>>>
> >>>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
> >>>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
> >>>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
> >>>>>>>       * Something went wrong, probably crashing the domain.  Unmap
> >>>>>>>       * everything and return.
> >>>>>>>       */
> >>>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
> >>>>>>>      for ( i = 0; i < count; i++ )
> >>>>>>>          if ( map[i] )
> >>>>>>>              unmap_domain_page(map[i]);
> >>>>>>
> >>>>>> ... the label (just out of context) where the comment needs to go?
> >>>>>
> >>>>> Because of the way this rule's configuration work, deviations are
> >>>>> placed
> >>>>> on the construct that ends up being the target of the 
> >>>>> unreachability,
> >>>>
> >>>> What's "target" here? What if this loop was removed from the 
> >>>> function?
> >>>> Then both the label and the domain_crash() invocations would still be
> >>>> unreachable in debug builds. Are you telling me that this then 
> >>>> wouldn't
> >>>> be diagnosed by Eclair? Or that it would then consider the closing
> >>>> figure brace of the function "the target of the unreachability"?
> >>>
> >>> Exactly, the end brace is a target to which the "function end" 
> >>> construct
> >>> is associated.
> >>> It would be kind of strange, though: why not just doing 
> >>> "domain_crash();
> >>> return;" in that case?
> >>
> >> Sure, the question was theoretical. Now if "return" was used directly
> >> there, what would then be the "target"? IOW - the more abstract 
> >> question
> >> of my earlier reply still wasn't answered.
> >>
> > 
> > The return statement in
> > 
> > ...
> > domain_crash();
> > return;
> > <~~~~~>
> > 
> > Whichever statement is found to be unreachable in the current 
> > preprocessed code.
> 
> Yet then again: Why is it the return statement and not the function call
> one (really, it being a macro invocation: the do/while one that the macro
> expands to)? That's the first thing that won't be reached.

Hi Jan,

Are you trying to get clarity on the specific locations where the SAF
deviations could be placed for the sake of understanding how the
deviation system work?

Or are you asking for the SAF comment to be moved elsewhere because you
don't like the SAF comment after the out_unmap macro?


I think that the position Nicola has used is better than any of the
alternatives. It is clear and immediately obvious when you read it in
context (I admit that looking at the patch alone, without applying it,
it is a bit puzzling).

But I guess you probably also prefer a single SAF comment after the
out_unmap label rather than multiple SAF comment after every single
domain_crash ?  I don't think this patch can be improved.
Jan Beulich Sept. 12, 2024, 6:56 a.m. UTC | #9
On 12.09.2024 03:05, Stefano Stabellini wrote:
> On Tue, 10 Sep 2024, Jan Beulich wrote:
>> On 10.09.2024 12:17, Nicola Vetrini wrote:
>>> On 2024-09-10 12:03, Jan Beulich wrote:
>>>> On 10.09.2024 11:53, Nicola Vetrini wrote:
>>>>> On 2024-09-10 11:08, Jan Beulich wrote:
>>>>>> On 10.09.2024 10:56, Nicola Vetrini wrote:
>>>>>>> On 2024-07-01 10:36, Jan Beulich wrote:
>>>>>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
>>>>>>>> This being about unreachable code, why are the domain_crash() not 
>>>>>>>> the
>>>>>>>> crucial points of "unreachability"? And even if they weren't there,
>>>>>>>> why
>>>>>>>> wouldn't it be the goto or ...
>>>>>>>>
>>>>>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
>>>>>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
>>>>>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
>>>>>>>>>       * Something went wrong, probably crashing the domain.  Unmap
>>>>>>>>>       * everything and return.
>>>>>>>>>       */
>>>>>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
>>>>>>>>>      for ( i = 0; i < count; i++ )
>>>>>>>>>          if ( map[i] )
>>>>>>>>>              unmap_domain_page(map[i]);
>>>>>>>>
>>>>>>>> ... the label (just out of context) where the comment needs to go?
>>>>>>>
>>>>>>> Because of the way this rule's configuration work, deviations are
>>>>>>> placed
>>>>>>> on the construct that ends up being the target of the 
>>>>>>> unreachability,
>>>>>>
>>>>>> What's "target" here? What if this loop was removed from the 
>>>>>> function?
>>>>>> Then both the label and the domain_crash() invocations would still be
>>>>>> unreachable in debug builds. Are you telling me that this then 
>>>>>> wouldn't
>>>>>> be diagnosed by Eclair? Or that it would then consider the closing
>>>>>> figure brace of the function "the target of the unreachability"?
>>>>>
>>>>> Exactly, the end brace is a target to which the "function end" 
>>>>> construct
>>>>> is associated.
>>>>> It would be kind of strange, though: why not just doing 
>>>>> "domain_crash();
>>>>> return;" in that case?
>>>>
>>>> Sure, the question was theoretical. Now if "return" was used directly
>>>> there, what would then be the "target"? IOW - the more abstract 
>>>> question
>>>> of my earlier reply still wasn't answered.
>>>>
>>>
>>> The return statement in
>>>
>>> ...
>>> domain_crash();
>>> return;
>>> <~~~~~>
>>>
>>> Whichever statement is found to be unreachable in the current 
>>> preprocessed code.
>>
>> Yet then again: Why is it the return statement and not the function call
>> one (really, it being a macro invocation: the do/while one that the macro
>> expands to)? That's the first thing that won't be reached.
> 
> Are you trying to get clarity on the specific locations where the SAF
> deviations could be placed for the sake of understanding how the
> deviation system work?
> 
> Or are you asking for the SAF comment to be moved elsewhere because you
> don't like the SAF comment after the out_unmap macro?

The former, in order to make up my mind at all.

> I think that the position Nicola has used is better than any of the
> alternatives. It is clear and immediately obvious when you read it in
> context (I admit that looking at the patch alone, without applying it,
> it is a bit puzzling).

I disagree, but maybe the clarification asked for would change that.

Jan
Stefano Stabellini Sept. 12, 2024, 10:57 p.m. UTC | #10
On Thu, 12 Sep 2024, Jan Beulich wrote:
> On 12.09.2024 03:05, Stefano Stabellini wrote:
> > On Tue, 10 Sep 2024, Jan Beulich wrote:
> >> On 10.09.2024 12:17, Nicola Vetrini wrote:
> >>> On 2024-09-10 12:03, Jan Beulich wrote:
> >>>> On 10.09.2024 11:53, Nicola Vetrini wrote:
> >>>>> On 2024-09-10 11:08, Jan Beulich wrote:
> >>>>>> On 10.09.2024 10:56, Nicola Vetrini wrote:
> >>>>>>> On 2024-07-01 10:36, Jan Beulich wrote:
> >>>>>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
> >>>>>>>> This being about unreachable code, why are the domain_crash() not 
> >>>>>>>> the
> >>>>>>>> crucial points of "unreachability"? And even if they weren't there,
> >>>>>>>> why
> >>>>>>>> wouldn't it be the goto or ...
> >>>>>>>>
> >>>>>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
> >>>>>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
> >>>>>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
> >>>>>>>>>       * Something went wrong, probably crashing the domain.  Unmap
> >>>>>>>>>       * everything and return.
> >>>>>>>>>       */
> >>>>>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
> >>>>>>>>>      for ( i = 0; i < count; i++ )
> >>>>>>>>>          if ( map[i] )
> >>>>>>>>>              unmap_domain_page(map[i]);
> >>>>>>>>
> >>>>>>>> ... the label (just out of context) where the comment needs to go?
> >>>>>>>
> >>>>>>> Because of the way this rule's configuration work, deviations are
> >>>>>>> placed
> >>>>>>> on the construct that ends up being the target of the 
> >>>>>>> unreachability,
> >>>>>>
> >>>>>> What's "target" here? What if this loop was removed from the 
> >>>>>> function?
> >>>>>> Then both the label and the domain_crash() invocations would still be
> >>>>>> unreachable in debug builds. Are you telling me that this then 
> >>>>>> wouldn't
> >>>>>> be diagnosed by Eclair? Or that it would then consider the closing
> >>>>>> figure brace of the function "the target of the unreachability"?
> >>>>>
> >>>>> Exactly, the end brace is a target to which the "function end" 
> >>>>> construct
> >>>>> is associated.
> >>>>> It would be kind of strange, though: why not just doing 
> >>>>> "domain_crash();
> >>>>> return;" in that case?
> >>>>
> >>>> Sure, the question was theoretical. Now if "return" was used directly
> >>>> there, what would then be the "target"? IOW - the more abstract 
> >>>> question
> >>>> of my earlier reply still wasn't answered.
> >>>>
> >>>
> >>> The return statement in
> >>>
> >>> ...
> >>> domain_crash();
> >>> return;
> >>> <~~~~~>
> >>>
> >>> Whichever statement is found to be unreachable in the current 
> >>> preprocessed code.
> >>
> >> Yet then again: Why is it the return statement and not the function call
> >> one (really, it being a macro invocation: the do/while one that the macro
> >> expands to)? That's the first thing that won't be reached.
> > 
> > Are you trying to get clarity on the specific locations where the SAF
> > deviations could be placed for the sake of understanding how the
> > deviation system work?
> > 
> > Or are you asking for the SAF comment to be moved elsewhere because you
> > don't like the SAF comment after the out_unmap macro?
> 
> The former, in order to make up my mind at all.

OK.

Nicola, I think I understand Jan's question and I'll try to clarify. The code in
p2m_pod_zero_check looks like this:

p2m_pod_zero_check(..
{
    [...]

    if ( something )
    {
        ASSERT_UNREACHABLE();
        /* potential SAF comment position#1 */
        domain_crash(d);
        /* potential SAF comment position#2 */
        goto out_unmap;
    }

    [...]

    return;

out_unmap:
    /* SAF comment added by patch */

    [...]
}

Jan is trying to understand why the SAF comment is placed after the
label "out_unmap" instead of position#1 or position#2 in my example.

The question arises from the following observations:
- anything after ASSERT_UNREACHABLE should be unreachable
- ignoring ASSERT_UNREACHABLE, anything after domain_crash should be
  unreachable
- "goto out_unmap" is a statement in itself, why is the SAF comment
  placed after the execution of "goto out_unmap" instead of before
  (position#2)?


In general, I agree it would be good for us to understand which
positions are allowed for the SAF comment. But at the same time in my
opinion among all these possible position, Nicola already picked the
best one and I wouldn't be in favor of moving the SAF comment in
position#1 or position#2.
diff mbox series

Patch

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 3f18ef401c7d..880aef784ea1 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -68,6 +68,14 @@ 
         },
         {
             "id": "SAF-8-safe",
+            "analyser": {
+                "eclair": "MC3R1.R2.1"
+            },
+            "name": "MC3R1.R2.1: statement unreachable in some configurations",
+            "text": "Every path that can reach this statement is preceded by statements that make it unreachable in certain configurations (e.g. ASSERT_UNREACHABLE). This is understood as a means of defensive programming."
+        },
+        {
+            "id": "SAF-9-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index bd84fe9e27ee..736d3ffd1ff6 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1040,6 +1040,7 @@  out_unmap:
      * Something went wrong, probably crashing the domain.  Unmap
      * everything and return.
      */
+    /* SAF-8-safe Rule 2.1: defensive programming */
     for ( i = 0; i < count; i++ )
         if ( map[i] )
             unmap_domain_page(map[i]);