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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.04.19 at 15:41, <tamas@xxxxxxxxxxxxx> wrote:
> On Fri, Apr 12, 2019 at 5:55 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 12.04.19 at 06:29, <tamas@xxxxxxxxxxxxx> wrote:
>> > @@ -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.

I see - back when I created the change you're now trying to fix,
I was wondering but couldn't find any proof. So yes, adding an
ASSERT() would surely be helpful.

Jan



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