Re: Locking pages allocated via paging_get_guest_pages()

['Pratyush Yadav' via Jailhouse]

On 02/07/19 5:26 PM, Jan Kiszka wrote:
> On 02.07.19 13:50, Pratyush Yadav wrote:
>> Hi Jan, all
>>
>> The pages mapped via paging_get_guest_pages() are mapped on 
>> TEMPORARY_MAPPING_BASE, and the mapping is over-written when another call to 
>> the function is made. This produces a race condition when two cells have two 
>> drivers calling this function. If one driver is not done using the mapping, 
>> and another over-writes it, bad things could happen. Of course, if it is the 
>> same driver in both cells, you could use a global locking construct for it, 
>> but neither is this a very clean fix, nor does it solve the case when two 
>> different drivers are in play.
>>
>> One alternative is to lock TEMPORARY_MAPPING_BASE until a release function 
>> is called to release that mapping (using, say, 
>> paging_release_guest_pages()). If the base is locked, the call to 
>> paging_get_guest() can block. This is problematic when the locking is too 
>> long. Another option is to return NULL when the base is locked, and then it 
>> is the driver's job to re-try.
>>
>> The second alternative is to do away with TEMPORARY_MAPPING_BASE entirely, 
>> and use the remap pool instead.
>>
>> I personally like the second alternative better.
>>
>> Either way, all code using paging_get_guest_pages() needs to be updated. 
>> Luckily, it is only used in a handful of places so it should not be too 
>> difficult to update.
>>
>> Thoughts on this problem? Any better ideas than mine?
>>
> 
> I think there is some misunderstanding how mapping in Jailhouse work:
> 
> paging_get_guest_pages() requests temporary, per-cpu access to some guest 
> pages in order to process a guest request synchronously. When returning to 
> the guest, that mapping becomes logically invalid, even if it may stay active 
> until some other call to paging_get_guest_pages() on the same CPU. This 
> pattern both avoids locking needs and exposing information to the hypervisor 
> that should not leak to other cores and, thus, potentially other cells.
> 
> Mapping things to an address from remap_pool is to expose hardware resources 
> permanently to the hypervisor. That is done during setup, not during runtime. 
> Thus, no locking is needed here as well.
> 
> Now, how do your needs not fit into this model, and why?

Yes, there indeed was a misunderstanding. I thought the mapping was for all 
CPUs, not per-CPU. The current implementation works fine in that case.

> Jan
> 

-- 
Regards,
Pratyush Yadav

-- 
You received this message because you are subscribed to the Google Groups 
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jailhouse-dev/6a5bfa46-03a8-caf0-1636-45f9f10f7f64%40ti.com.
For more options, visit https://groups.google.com/d/optout.