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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----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).

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel