Re: [Xen-devel] [PATCH v2] xen/mm: Avoid assuming the page is inuse in assign_pages()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.02.2020 12:44, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xxxxxxx>
>> Sent: 06 February 2020 11:17
>> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Grall, Julien <jgrall@xxxxxxxxxx>
>> Subject: Re: [PATCH v2] xen/mm: Avoid assuming the page is inuse in
>> assign_pages()
>>
>> Hi Paul,
>>
>> On 06/02/2020 10:52, Durrant, Paul wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall <julien@xxxxxxx>
>>>> Sent: 06 February 2020 10:39
>>>> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Cc: julien@xxxxxxx; Durrant, Paul <pdurrant@xxxxxxxxxxxx>; Grall,
>> Julien
>>>> <jgrall@xxxxxxxxxx>
>>>> Subject: [PATCH v2] xen/mm: Avoid assuming the page is inuse in
>>>> assign_pages()
>>>>
>>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>>
>>>> At the moment, assign_pages() on the page to be inuse (PGC_state_inuse)
>>>> and the state value to be 0.
>>>>
>>>> However, the code may race with the page offlining code (see
>>>> offline_page()). Depending on the ordering, the page may be in
>> offlining
>>>> state (PGC_state_offlining) before it is assigned to a domain.
>>>>
>>>> On debug build, this may result to hit the assert or just clobber the
>>>> state. On non-debug build, the state will get clobbered.
>>>>
>>>> Incidentally the flag PGC_broken will get clobbered as well.
>>>>
>>>> Grab the heap_lock to prevent a race with offline_page() and keep the
>>>> state and broken flag around.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>
>>> This seems like a reasonable change. I guess having assign_pages() take
>> the global lock is no more problem than its existing call to
>> domain_adjust_tot_pages() which also takes the same lock.
>>
>> That's my understanding. Summarizing our discussion IRL for the other,
>> it is not clear whether the lock is enough here.
>>
>>  From my understanding the sequence
>>
>> pg[i].count_info &= ...;
>> pg[i].count_info |= ...;
>>
>> could result to multiple read/write from the compiler. We could use a
>> single assignment, but I still don't think this prevent the compiler to
>> be use multiple read/write.
>>
>> The concern would be a race with get_page_owner_and_reference(). If 1 is
>> set before the rest of the bits, then you may be able to get the page.
>>
>> So I might want to use write_atomic() below. Any opinion?
>>
> 
> TBH I wonder if we ought to say that any update to count_info ought to
> be done by a write_atomic (where it's not already done by cmpxchg).

I agree.

Jan

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