Re: [XEN PATCH v2 05/13] x86/traps: address violations of MISRA C Rule 16.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.06.2024 00:59, Stefano Stabellini wrote:
> On Thu, 27 Jun 2024, Jan Beulich wrote:
>> On 27.06.2024 03:53, Stefano Stabellini wrote:
>>> On Wed, 26 Jun 2024, Jan Beulich wrote:
>>>> On 26.06.2024 03:11, Stefano Stabellini wrote:
>>>>> On Tue, 25 Jun 2024, Jan Beulich wrote:
>>>>>> On 25.06.2024 02:54, Stefano Stabellini wrote:
>>>>>>> On Mon, 24 Jun 2024, Federico Serafini wrote:
>>>>>>>> Add break or pseudo keyword fallthrough to address violations of
>>>>>>>> MISRA C Rule 16.3: "An unconditional `break' statement shall terminate
>>>>>>>> every switch-clause".
>>>>>>>>
>>>>>>>> No functional change.
>>>>>>>>
>>>>>>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>>  xen/arch/x86/traps.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>>>>>>>> index 9906e874d5..cbcec3fafb 100644
>>>>>>>> --- a/xen/arch/x86/traps.c
>>>>>>>> +++ b/xen/arch/x86/traps.c
>>>>>>>> @@ -1186,6 +1186,7 @@ void cpuid_hypervisor_leaves(const struct vcpu 
>>>>>>>> *v, uint32_t leaf,
>>>>>>>>  
>>>>>>>>      default:
>>>>>>>>          ASSERT_UNREACHABLE();
>>>>>>>> +        break;
>>>>>>>
>>>>>>> Please add ASSERT_UNREACHABLE to the list of "unconditional flow control
>>>>>>> statements" that can terminate a case, in addition to break.
>>>>>>
>>>>>> Why? Exactly the opposite is part of the subject of a recent patch, iirc.
>>>>>> Simply because of the rules needing to cover both debug and release 
>>>>>> builds.
>>>>>
>>>>> The reason is that ASSERT_UNREACHABLE() might disappear from the release
>>>>> build but it can still be used as a marker during static analysis. In
>>>>> my view, ASSERT_UNREACHABLE() is equivalent to a noreturn function call
>>>>> which has an empty implementation in release builds.
>>>>>
>>>>> The only reason I can think of to require a break; after an
>>>>> ASSERT_UNREACHABLE() would be if we think the unreachability only apply
>>>>> to debug build, not release build:
>>>>>
>>>>> - debug build: it is unreachable
>>>>> - release build: it is reachable
>>>>>
>>>>> I don't think that is meant to be possible so I think we can use
>>>>> ASSERT_UNREACHABLE() as a marker.
>>>>
>>>> Well. For one such an assumption takes as a prereq that a debug build will
>>>> be run through full coverage testing, i.e. all reachable paths proven to
>>>> be taken. I understand that this prereq is intended to somehow be met,
>>>> even if I'm having difficulty seeing how such a final proof would look
>>>> like: Full coverage would, to me, mean that _every_ line is reachable. Yet
>>>> clearly any ASSERT_UNREACHABLE() must never be reached.
>>>>
>>>> And then not covering for such cases takes the further assumption that
>>>> debug and release builds are functionally identical. I'm afraid this would
>>>> be a wrong assumption to make:
>>>> 1) We may screw up somewhere, with code wrongly enabled only in one of the
>>>>    two build modes.
>>>> 2) The compiler may screw up, in particular with optimization.
>>>
>>> I think there are two different issues here we are discussing.
>>>
>>> One issue, like you said, has to do with coverage. It is important to
>>> mark as "unreachable" any part of the code that is indeed unreachable
>>> so that we can account it properly when we do coverage analysis. At the
>>> moment the only "unreachable" marker that we have is
>>> ASSERT_UNREACHABLE(), and I am hoping we can use it as part of the
>>> coverage analysis we'll do.
>>>
>>> However, there is a different separate question about what to do in the
>>> Xen code after an ASSERT_UNREACHABLE(). E.g.:
>>>
>>>              default:
>>>                  ASSERT_UNREACHABLE();
>>>                  return -EPERM; /* is it better with or without this? */
>>>              }
>>>
>>> Leaving coverage aside, would it be better to be defensive and actually
>>> attempt to report errors back after an ASSERT_UNREACHABLE() like in the
>>> example? Or is it better to assume the code is actually unreachable
>>> hence there is no need to do anything afterwards?
>>>
>>> One one hand, being defensive sounds good, on the other hand, any code
>>> we add after ASSERT_UNREACHABLE() is dead code which cannot be tested,
>>> which is also not good. In this example, there is no way to test the
>>> return -EPERM code path. We also need to consider what is the right
>>> thing to do if Xen finds itself in an erroneous situation such as being
>>> in an unreachable code location.
>>>
>>> So, after thinking about it and also talking to the safety manager, I
>>> think we should:
>>> - implement ASSERT_UNREACHABLE with a warning in release builds
>>
>> If at all, then controlled by a default-off Kconfig setting. This would,
>> after all, raise the question of how ASSERT() should behave then. Imo
>> the two should be consistent in this regard, and NDEBUG clearly results
>> in the expectation that ASSERT() expands to nothing. Perhaps this is
>> finally the time where we need to separate NDEBUG from CONFIG_DEBUG; we
>> did discuss doing so before. Then in your release builds, you could
>> actually leave assertions active.
>  
> Yes, a kconfig to define the behavior of ASSERT_UNREACHABLE in release
> builds is fine. And you are right that we should consider doing
> something similar for ASSERT too.
> 
> I think that in any environment where safety (i.e. correctness of
> behavior) is a primary concern, attempting to continue without doing
> anything after reaching a point that is supposed to be unreachable is
> not a good idea.
> 
> I think Xen should do something in response to reaching a point it is
> not supposed to reach. I don't know yet what is the best course of
> action but printing a warning seems to be the bare minimum.
> 
> Crashing the system is not a good idea as it could potentially be
> exploited by malicious guests (security might not be the primary concern
> but still.)

Yet continuing may set the system up for much harder to understand issues,
including crashes and exploitable issues later on.

Jan