Hi Volodymyr,
On 20/03/2019 17:42, Volodymyr Babchuk wrote:
[...]
+static void put_std_call(struct optee_domain *ctx, struct optee_std_call *call) +{
- spin_lock(&ctx->lock);
- ASSERT(call->in_flight);
- unmap_xen_arg(call);
Same question for the unmap.
Yeah, in normal circumstances guest should not try to resume call on another vCPU, because we didn't returned from the original call on current vCPU. But what if gust will try to do this? There are chances, that current CPU will unmap buffer that was mapped by other CPU an instance ago.
Wasn't it the point to have the in_flight field? As soon as you set in_flight to true, then only one CPU can have the optee_std_call structure in hand.
So, as long as you map/unmap within the section protected by "in_flight", you protected against any race. The lock is only here to protect the field in_flight and the list. Did I miss anything?
This is partially true. I can call map_xen_arg() after spin_unlock(), yes.
But then I need to call unmap_xen_arg() before spin_lock() in put_std_call() function. If there were no ASSERT() that would be okay. But with ASSERT() it is looking weird: firstly we are unmapping buffer and the we are asserting that we had a right to do this. This is sort of "use before check"
If you hit the ASSERT (or if it where a BUG_ON) then something has already got horribly wrong. You will crash in any case, it is not going to matter much whether you crash before unmapping or after unmapping.
And obviously, I can't call ASSERT() without holding the the spinlock. > On other hand, ASSERT() is a debugging feature, so we can pretend that is not there... If you okay with this, I'll move mapping/unmapping out of the spinlocks.
ASSERT/BUG_ON are only here to catch against anything would be horribly wrong if we continue (the severity will drive the choice between ASSERT and BUG_ON).
In normal case, this should never be hit. So I would prefer if lock section are kept to minimal.
Cheers,