[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [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
Subject: [PATCH v2] xen/mm: Avoid assuming the page is inuse in

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?


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.