Re: [Xen-devel] [PATCH] add page_get_owner_and_reference() related ASSERT()s

[Tim Deegan <tim@xxxxxxx>]

At 03:46 -0600 on 13 Aug (1439437600), Jan Beulich wrote:
> >>> On 13.08.15 at 11:31, <tim@xxxxxxx> wrote:
> > At 09:50 +0100 on 13 Aug (1439459451), Ian Campbell wrote:
> >> On Fri, 2015-07-24 at 06:37 -0600, Jan Beulich wrote:
> >> > The function shouldn't return NULL after having obtained a reference,
> >> > or else the caller won't know to drop it.
> >> 
> >> > Also its result shouldn't be ignored - if calling code is certain that
> >> > a page already has a non-zero refcount, it better ASSERT()s so.
> >> 
> >> How is page_get_owner_and_reference sure the reference count was not zero
> >> on entry? (WRT the call to page_get_owner in page_get_owner_and_reference
> >> itself) Or have I misunderstood the assertion being made here?
> > 
> > That path is fine -- if the count was zero, it will return NULL before
> > trying to get the owner.  But I'm not convinced that the assertion is
> > correct -- are there really no pages anywhere that have a recount but
> > no owner?  What about, e.g. shadow pool pages or other uses of
> > MEMF_no_owner?  If a guest can cause Xen to try to get_page() one of
> > those, then we'd hit the new assertion.
> > 
> > How about unlikely(!owner) path that drops the taken ref instead?
> 
> That would be dead code - a page the ref count of wasn't zero
> prior to the function taking an extra ref shouldn't be unowned.

Right.  That being the case, Reviewed-by: Tim Deegan <tim@xxxxxxx>.

Cheers,

Tim.

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