Re: [XEN PATCH v8 02/22] xen/arm: tee: add a primitive FF-A mediator

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/04/2023 1:26 pm, Julien Grall wrote:
>> +static int ffa_domain_init(struct domain *d)
>> +{
>> +    struct ffa_ctx *ctx;
>> +
>> +    if ( !ffa_version )
>> +        return -ENODEV;
>> +
>> +    ctx = xzalloc(struct ffa_ctx);
>> +    if ( !ctx )
>> +        return -ENOMEM;
>> +
>> +    d->arch.tee = ctx;
>> +
>> +    return 0;
>> +}
>> +
>> +/* This function is supposed to undo what ffa_domain_init() has done */
>
> I think there is a problem in the TEE framework. The callback
> .relinquish_resources() will not be called if domain_create() failed.
> So this will result to a memory leak.
>
> We also can't call .relinquish_resources() on early domain creation
> failure because relinquishing resources can take time and therefore
> needs to be preemptible.
>
> So I think we need to introduce a new callback domain_free() that will
> be called arch_domain_destroy(). Is this something you can look at?


Cleanup of an early domain creation failure, however you do it, is at
most "the same amount of time again".  It cannot (absent of development
errors) take the same indefinite time periods of time that a full
domain_destroy() can.

The error path in domain_create() explicitly does call domain_teardown()
so we can (eventually) purge these duplicate cleanup paths.  There are
far too many easy errors to be made which occur from having split
cleanup, and we have had to issue XSAs in the past to address some of
them.  (Hence the effort to try and specifically change things, and
remove the ability to introduce the errors in the first place.)


Right now, it is specifically awkward to do this nicely because
domain_teardown() doesn't call into a suitable arch hook.

IMO the best option here is extend domain_teardown() with an
arch_domain_teardown() state/hook, and wire in the TEE cleanup path into
this too.

Anything else is explicitly adding to technical debt that I (or someone
else) is going to have to revert further down the line.

If you want, I am happy to prototype the arch_domain_teardown() bit of
the fix, but I will have to defer wiring in the TEE part to someone
capable of testing it.

~Andrew