Re: [Xen-devel] [PATCH 1/2] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Apr 12, 2019 at 5:55 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 12.04.19 at 06:29, <tamas@xxxxxxxxxxxxx> wrote:
> > Patch 0502e0adae2 "x86: correct instances of PGC_allocated clearing" 
> > introduced
> > grabbing extra references for pages that drop references tied to 
> > PGC_allocated.
> > However, the way these extra references were grabbed were incorrect, 
> > resulting
> > in both share_pages and unshare_pages failing.
>
> I'm sorry for this.

It's my bad for not catching it earlier when I acked it. Reading the
patch it looked fine and made sense but evidently that's no substitute
for actually testing it.

>
> > @@ -984,7 +976,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, 
> > shr_handle_t sh,
> >           * Don't change the type of rmap for the client page. */
> >          rmap_del(gfn, cpage, 0);
> >          rmap_add(gfn, spage);
> > -        put_page_and_type(cpage);
> > +        put_count++;
>
> Since this sits in a while(), ...
>
> > @@ -1002,7 +994,9 @@ static int share_pages(struct domain *sd, gfn_t sgfn, 
> > shr_handle_t sh,
> >      /* Free the client page */
> >      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >          put_page(cpage);
> > -    put_page(cpage);
> > +
> > +    while(put_count--)
> > +        put_page_and_type(cpage);
>
> ... put_count may be zero before this while(). At which point the
> earlier put_page() would still be unsafe.

The page should have at least one reference it got before when the
page went through page_make_sharable function. We also verified that
the page is still sharable so that reference is still there, so this
count is at least 1. We could certainly add an ASSERT(put_count)
before though.

>
> Also, despite the if() in context suggesting the style you used,
> the rest of the function looks to use proper Xen style, so would
> you please add the three missing blanks to the while() you add?

Ack.

Thanks,
Tamas

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