Re: [XEN PATCH v4] xen/arm: ffa: reclaim shared memory on guest destroy

[Jens Wiklander <jens.wiklander@xxxxxxxxxx>]

On Mon, Feb 5, 2024 at 5:54 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 05/02/2024 3:49 pm, Jens Wiklander wrote:
> > diff --git a/xen/arch/arm/tee/ffa.c b/xen/arch/arm/tee/ffa.c
> > index 0793c1c7585d..bbb6b819ee2b 100644
> > --- a/xen/arch/arm/tee/ffa.c
> > +++ b/xen/arch/arm/tee/ffa.c
> > @@ -992,53 +1008,75 @@ static void put_shm_pages(struct ffa_shm_mem *shm)
> >      }
> >  }
> >
> > -static bool inc_ctx_shm_count(struct ffa_ctx *ctx)
> > +static bool inc_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
> >  {
> >      bool ret = true;
> >
> >      spin_lock(&ctx->lock);
> > +
> > +    /*
> > +     * If this is the first shm added, increase the domain reference
> > +     * counter as we need to keep domain around a bit longer to reclaim the
> > +     * shared memory in the teardown path.
> > +     */
> > +    if ( !ctx->shm_count )
> > +        get_knownalive_domain(d);
> > +
> >      if (ctx->shm_count >= FFA_MAX_SHM_COUNT)
> >          ret = false;
> >      else
> >          ctx->shm_count++;
> > +
> >      spin_unlock(&ctx->lock);
>
> This is subtle.  It reads as if there is a reference leak.  There really
> will be one if FFA_MAX_SHM_COUNT happens to be 0.
>
> You could add a BUILD_BUG_ON(), but IMO it would be far clearer to
> follow if you moved the get_knownalive_domain() into the else clause.

I see your point, I'll fix it in the next version.

>
> >
> >      return ret;
> >  }
> >
> > -static void dec_ctx_shm_count(struct ffa_ctx *ctx)
> > +static void dec_ctx_shm_count(struct domain *d, struct ffa_ctx *ctx)
> >  {
> >      spin_lock(&ctx->lock);
> > +
> >      ASSERT(ctx->shm_count > 0);
> >      ctx->shm_count--;
> > +
> > +    /*
> > +     * If this was the last shm removed, let go of the domain reference we
> > +     * took in inc_ctx_shm_count() above.
> > +     */
> > +    if ( !ctx->shm_count )
> > +        put_domain(d);
> > +
> >      spin_unlock(&ctx->lock);
>
> You want a local bool called drop_ref, set within the lock, and move the
> put_domain() down here.  put_domain() is potentially a large operation.

OK, I'll fix it in the next version.

Thanks,
Jens