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
