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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew