Re: [Xen-devel] [PATCH v2 4/6] xen/arm: optee: handle shared buffer translation error

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Julien Grall writes:

> On 24/09/2019 12:37, Volodymyr Babchuk wrote:
>>
>> Hi Julien,
>>
>> Julien Grall writes:
>>
>>> Hi,
>>>
>>> On 18/09/2019 19:51, Volodymyr Babchuk wrote:
>>>> +/* Handles return from Xen-issued RPC */
>>>> +static void handle_xen_rpc_return(struct optee_domain *ctx,
>>>> +                                  struct cpu_user_regs *regs,
>>>> +                                  struct optee_std_call *call,
>>>> +                                  struct shm_rpc *shm_rpc)
>>>> +{
>>>> +    call->state = OPTEE_CALL_NORMAL;
>>>> +
>>>> +    /*
>>>> +     * Right now we have only one reason to be there - we asked guest
>>>> +     * to free shared buffer and it did it. Now we can tell OP-TEE
>>>> +     * that buffer allocation failed. We are not storing exact command
>>>> +     * type, only type of RPC return. So, this is the only check we
>>>> +     * can perform there.
>>>> +     */
>>>> +    ASSERT(call->rpc_op == OPTEE_SMC_RPC_FUNC_CMD);
>>>
>>> As I pointed out in v1, ASSERT() is probably the less desirable
>>> solution here as this is an error path.
>> Looks like I misunderstood you :(
>>
>>> Can you explain why you chose that over the 3 solutions I suggested?
>> I think I need some clarification there. ASSERT() is adopted way to tell
>> about invariant. Clearly, this is programming error if ASSERT() fails.
>
> That's correct.
>
>>
>> But I understand, that ASSERT() is available only in debug build. So, in
>> release it will never fire. As this is very unlikely error path, bug
>> there can live forever. Okay, so ASSERT() is the least desirable way.
>
> This is my concern, ASSERT() are fine in path that can be exercised
> quite well. By experience, error path as not so well tested, so any
> verbosity in non-debug build is always helpful.
Yep, I see.

>>
>> Warning is not enough, as we are already in incorrect state.
> Incorrect state for who? The guest?
Yes, for the current call of the current guest. State of other calls and
other guests should not be affected. But it is possible that our view of
OP-TEE state for that guest is not correct also.

>>
>> So, best way is to crash guest, it this correct? I'll do this in the
>> next version then.
>
> A rule of thumb is
>   - BUG_ON can be replaced by domain_crash() as this is a fatal error
> you can't recover (the scope depends on the function call).
This seems correct.

>
>   - ASSERT() can be replaced by WARN_ON() as the former will be a NOP
> in non-debug build. In both case, the error is not fatal and continue
> will not result So it means the error is not fatal.
I can't agree with this in general. But maybe this makes sense for Xen.
As I said, generally ASSERT() is used to, well, assert that condition is
true for any correct state of a program. So it cant' be replaced with
WARN_ON(). If we detected invalid state we should either try to correct
it (if know how) or to immediately stop the program.

But I can see, why this behavior is not desired for
hypervisor. Especially in production use.

> You used ASSERT() in your code, so it feels to me that WARN_ON() would
> be a suitable replacement.
Well, honestly I believe that it is better to crash guest to prevent
any additional harm. Look, we already detected that something is wrong
with mediator state for certain guest. We can pretend that all is fine
and try to continue the call. But who knows which consequences it will have?

> However, if the state inconsistency is for Xen, then domain_crash() is
> the best. If this is for the guest, then this is not really our
> business and it may be best to let him continue as it could provide
> more debug (domain_crash() will only dump the stack and registers).
I'm looking at this from different point: we promised to provide some
service for a guest and screwed up. It is not guest's fault. Now we know
that we can't provide reliable service for a guest anymore. From
safety point of view we should shut down the service. (But this is job
for another patch) For now, we at least should crash the
guest. This is the safest way. What do you think?

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel