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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.)


> > - have "return -EPERM;" or similar for defensive programming
> 
> You don't say how you'd deal with the not-reachable aspect then.

We'll have to find a way to account for all the code that cannot be
tested. We would have a problem anyway due to the ASSERT_UNREACHABLE
checks, but the addition of "return -EPERM;" will make things slightly
worse.

I have been told to prioritize safety of the code and defensive
programming over coverage calculations.