>>> On 15.04.19 at 19:28, <george.dun...@citrix.com> wrote: >> On Apr 15, 2019, at 4:31 PM, Tamas K Lengyel <ta...@tklengyel.com> wrote: >> On Mon, Apr 15, 2019 at 9:21 AM George Dunlap <george.dun...@citrix.com> >> wrote: >>> On 4/12/19 8:47 PM, Tamas K Lengyel 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. >>> >>> Why? Is cd not the owner of the page? >> >> It's not, dom_cow is. >> >>>> @@ -1002,7 +994,10 @@ 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); >>>> + >>>> + ASSERT(put_count); >>>> + while ( put_count-- ) >>>> + put_page_and_type(cpage); >>> >>> I don't think this is in the spirit of the fix in 0502e0adae2; the idea >>> there seems to have been to make sure that cpage belongs to the right >>> domain, and that the ownership doesn't change. If such a race / mistake >>> were possible before that change, such a race / mistake would be >>> possible after this change, wouldn't it? >> >> I don't have an answer to your question. AFAIU this is just to ensure >> that the page isn't dropped before test_and_clear_bit is used on the >> page. > > Right, but why would that be a bad thing? > > I think it’s to avoid races like this: > > P1: Gets pointer to page A, owned by domain M > P2: Gets pointer to page A, owned by domain M > P2: test_and_clear_bit(PGC_allocated) succeeds > P2: put_page() > -> page freed > -> Page re-allocated to domain N > P1: test_and_clear_bit(PGC_allocated) succeeds > P1: put_page() > -> page freed ## > > Now P1 incorrectly “frees” a page owned by domain N. > > If both P1 and P2 call get_page(A, M) first, and drop that reference > afterwards, this race can’t happen. > > Jan, you’re the author of that patch — is that not the case?
Yes indeed, that's why I did add the extra get_page() (not realizing the owning domain is the wrong one). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
