Re: [Xen-devel] Is this a racing bug in page_make_sharable()?

[Andres Lagar-Cavilla <andreslc@xxxxxxxxxxxxxx>]

Hi there, thanks for the report. Sorry I didn't respond earlier, fell through 
the cracks.

Having said that...
On Jan 10, 2013, at 8:00 AM, Tim Deegan <tim@xxxxxxx> wrote:

> Hi,
> 
> At 23:35 +0800 on 27 Dec (1356651327), Nai Xia wrote:
>> I think I can construct a bug by interleaving the two code paths:
>> 
>> in guest_remove_page()              |              in page_make_sharable()
>> ------------------------------------------------------------------------------------------------------------------------------
>> if ( p2m_is_shared(p2mt) )                       .....
>> ...                                              .....
>> page = mfn_to_page(mfn);                         .....
>>                                                 .....
>> 
>>                                                 if ( 
>>                                                 !get_page_and_type(page, 
>>                                                 d, PGT_shared_page) )    
>>                                                 // success
>> 
>>                                                 .........
>>                                                 if ( page->count_info != 
>>                                                 (PGC_allocated | (2 + 
>>                                                 expected_refcnt)) ) // 
>>                                                 also pass
>> 
>> 
>> if ( unlikely(!get_page(page, d)) )
>> 
>> /* go on to remove page */                       /* go on to add page to 
>> cow domain */
>> -------------------------------------------------------------------------------------------------------------------------------------
>> 
>> 
>> is there anything that can already prevent such racing or is this really 
>> can happen?
> 
> I think this race can happen.  

I beg to disagree.

Both page_make_sharable and page_make_private are protected by the per-mfn page 
lock (see __grab_shared_page). This lock will serialize strictly 
page_make_sharable against the page_make_private in quest_remove_page -> 
mem_sharing_unshare_page -> page_make_private.

So page_make_sharable will execute atomically, add to cow, and then unshare 
will come in, make the page private again, remove from cow and hand over to the 
domain for whom the page is being removed.

Or, the page has other shared references in which case page_make_sharable is a 
no-op, and page_make_private will generate a new different page (mfn).

Or, and this is the tricky case you refer to, page_make_private will complete 
before page_make_sharable kicks in. The question then is: how did 
page_make_sharable even got to be called? The mfn is being nominated for 
sharing. How? Through a p2m entry in a domain. Is this the same domain as the 
one for which quest_remove_page is executing? Then all is serialized through 
the p2m lock, no race. Is this a different domain? Then the page is already 
shared, no race as per above. 

Finally, is this a different domain and the page is not shared? This Should Not 
Be! As in, never happens with the current APIs, and IMHO is borderline 
uncheckable for.

Hope that helps,
Andres

> I'm not sure exactly what the effect
> is, though.  I guess the page ends up belonging to dom_cow, but
> without the PGC_allocated bit set.  So when it becomes unshared again,
> it's immediately freed. :(
> 
> Andres, what do you think?
> 
> Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel