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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> Warning is not enough, as we are already in incorrect state.
Incorrect state for who? The guest?

> 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).
  - 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.
You used ASSERT() in your code, so it feels to me that WARN_ON() would be a 
suitable replacement.
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).
Cheers,

--
Julien Grall

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