Re: [XEN PATCH 5/7] xen/arm: traps: add ASSERT_UNREACHABLE() where needed

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2023-12-14 23:32, Stefano Stabellini wrote:
> On Thu, 14 Dec 2023, Julien Grall wrote:
> > Hi,
> >
> > On 13/12/2023 14:02, Nicola Vetrini wrote:
> > > On 2023-12-12 16:49, Julien Grall wrote:
> > > > Hi,
> > > >
> > > > On 11/12/2023 12:32, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > On 11/12/2023 10:30, Nicola Vetrini wrote:
> > > > > > The branches of the switch after a call to 'do_unexpected_trap'
> > > > > > cannot return, but there is one path that may return, hence
> > > > > > only some clauses are marked with ASSERT_UNREACHABLE().
> > > > > I don't understand why this is necessary. The code should never be
> > > > > reachable because do_unexpected_trap() is a noreturn().
> > > >
> > > > From the matrix discussion, it wasn't clear what was my position on this
> > > > patch.
> > > >
> > > > I would much prefer if the breaks are kept. I could accept:
> > > >
> > > > ASSERT_UNREACHABLE();
> > > > break;
> > > >
> > > > But this solution is a Nack because if you are concerned about functions
> > > > like do_unexpected_trap() to return by mistaken, then it needs to also be
> > > > safe in production.
> > > >
> > > > The current proposal is not safe.
> I re-read the email thread. I also do not think that this is useful:
>
>          do_unexpected_trap("SVE trap at EL2", regs);
> -        break;
> +        ASSERT_UNREACHABLE();
>
> I also do not think that we should be concerned about functions like
> do_unexpected_trap() to return by mistaken.
>
> That said, what is the problem from MISRA point of view that we are
> trying to fix? Is the only problem the presence of break; after the 
> call
> to a noreturn function?
>
> If that's not allowed, I would suggest to do this:
>
>
>          do_unexpected_trap("SVE trap at EL2", regs);
> -        break;
> +        /* break; */
>
>
> Or deviate "break" globally as it doesn't seem to be a safety risk in 
> my
> opinion. If nothing else, it should make the code a bit safer because 
> in
> case of mistakes in do_unexpected_trap, at least we would continue to
> follow a more reasonable code path rather than blindly falling through
> the next switch case by accident.
That doesn't seem like a good idea to deviate just "break". However, 
Julien's earlier proposal
ASSERT_UNREACHABLE();
break;

is ok, though it could be shrunk in a macro

#define unreachable_break ASSERT_UNREACHABLE(); break;

or just

#define unreachable_break break;

so that "unreachable_break" can be deviated.

> > > Ok. I wonder whether the should be applied here in vcpreg.c:
> > >
> > > diff --git a/xen/arch/arm/vcpreg.c b/xen/arch/arm/vcpreg.c
> > > index 39aeda9dab62..089d2f03eb5e 100644
> > > --- a/xen/arch/arm/vcpreg.c
> > > +++ b/xen/arch/arm/vcpreg.c
> > > @@ -707,7 +707,8 @@ void do_cp10(struct cpu_user_regs *regs, const union hsr
> > > hsr)
> > >           inject_undef_exception(regs, hsr);
> > >           return;
> > >       }
> > > -
> > > +
> > > +    ASSERT_UNREACHABLE();
> > >       advance_pc(regs, hsr);
> > >   }
> > >
> > > the rationale being that, should the switch somehow fail to return, the
> > > advance_pc would be called, rather than doing nothing.
> >
> > To clarify, advance_pc(regs, hsr) would still be called in production 
> > build.
> > So if you are concerned about advance_pc() been called, then adding an
> > ASSERT_UNREACHABLE() is not going to help.
> >
> > It took me a little while to confirm that none of the path effectively 
> > returns
> > due to the macros (in hindsight, it wasn't a good idea of mine to 
> > introduce
> > them).
> >
> > Depending on what we are trying to solve there are 3 possible 
> > approach:
> >   1. Leave advance_pc(). This means we could potentially
> >      a. Advance the PC twice (if it was already called) and therefore 
> > skipping
> > an instruction
> >      b. Advance the PC once without an emulation
> >   2. Remove advance_pc(). If we already called the function, then 
> > there is no
> > problem. Otherwise, we would trap in a loop effectively rendering the 
> > guest
> > vCPU unusable.
> >   3. Replace with domain_crash()
> >
> > Here it feels, that 3 is more suitable as this gives a clear 
> > indication
> > why/where the emulation gone wrong.
> >
> > This may still need to be accompanied with a ASSERT_UNREACHABLE() to 
> > please
> > MISRA.
> >
> > Bertrand, Michal, Stefano, what do you think?
> Yes, I would go with 3., replace advance_pc with domain_crash. Assuming
> that it would also solve the violation in ECLAIR.
It needs to be prefixed with an ASSERT_UNREACHABLE(), though, because 
it's still a violation if there is no execution path leading to 
domain_crash(), but other than that it seems the safest choice.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)