Re: [XEN PATCH v9 02/24] xen/arm: add TEE teardown to arch_domain_teardown()

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

Hi,

On Wed, Jul 12, 2023 at 10:31 AM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 05/07/2023 10:34 am, Jens Wiklander wrote:
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 15d9709a97d2..18171decdc66 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -795,6 +795,42 @@ fail:
> >
> >  int arch_domain_teardown(struct domain *d)
> >  {
> > +    int ret = 0;
> > +
> > +    BUG_ON(!d->is_dying);
> > +
> > +    /* See domain_teardown() for an explanation of all of this magic. */
> > +    switch ( d->teardown.arch_val )
> > +    {
> > +#define PROGRESS(x)                             \
> > +        d->teardown.arch_val = PROG_ ## x;      \
> > +        fallthrough;                            \
> > +    case PROG_ ## x
> > +
> > +        enum {
> > +            PROG_none,
> > +            PROG_tee,
> > +            PROG_done,
> > +        };
> > +
> > +    case PROG_none:
> > +        BUILD_BUG_ON(PROG_none != 0);
> > +
> > +    PROGRESS(tee):
> > +        ret = tee_domain_teardown(d);
> > +        if ( ret )
> > +            return ret;
> > +        break;
>
> This unconditional break isn't correct.
>
> The logic functions right now (because upon hitting return 0, you don't
> re-enter this function), but will cease working when you add a new
> PROG_*, or when the calling code gets more complicated.
>
> > +
> > +    PROGRESS(done):
> > +        break;
>
> This needs to be the only break in the switch statement, for it to
> behave in the intended manner.

Got it, thanks for the explanation. I'll fix it in the next version.

Thanks,
Jens

>
> ~Andrew