On 8/2/19 3:44 PM, Jan Beulich wrote:
> On 30.07.2019 18:44, Paul Durrant wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1682,6 +1682,14 @@ gnttab_unpopulate_status_frames(struct domain *d,
>> struct grant_table *gt)
>> struct page_info *pg = virt_to_page(gt->status[i]);
>> gfn_t gfn = gnttab_get_frame_gfn(gt, true, i);
>>
>> + if ( !get_page(pg, d) )
>> + {
>> + gprintk(XENLOG_ERR,
>> + "Could not get a reference to status frame %u\n", i);
>> + domain_crash(d);
>> + return -EINVAL;
>> + }
>> +
>> /*
>> * For translated domains, recovering from failure after partial
>> * changes were made is more complicated than it seems worth
>> @@ -1708,6 +1716,7 @@ gnttab_unpopulate_status_frames(struct domain *d,
>> struct grant_table *gt)
>>
>> BUG_ON(page_get_owner(pg) != d);
>> put_page_alloc_ref(pg);
>> + put_page(pg);
>>
>> if ( pg->count_info & ~PGC_xen_heap )
>> {
>>
>
> I dislike this approach, and not chosing the alternative of excluding
> xenheap pages in the check in put_page_alloc_ref() (as I had recommended
> elsewhere) should at least be discussed in the description. It is the
> very nature of xenheap pages that they won't get freed, and hence don't
> need this extra ref to be held for clearing PGC_allocated.
Also, it looks like there are other places where the BUG_ON() may /
should be firing: namely, vmx_free_vlapic_mapping() and
unshare_xenoprof_page_with_guest(). Teaching put_page_alloc_ref() that
dropping PGC_allocated when PGC_xen_heap is set is safe would fix all
three at once.
Possibly more importantly, suppose that the first time
gnttab_unpopulate_status_frames() comes around, gt->status[1] is still
mapped by the guest. Then gt->status[0] will have its refcount reduced
to 0 (but not freed), but gt->status[1] will be restored to its previous
state. If the guest unmaps gt->status[1] and
gnttab_unpopulate_status_frames() is called again, then the
get_page(gt->status[0]) will fail (since its refcount is 0), causing the
guest to be crashed instead.
Not terrible for such a wonkily-behaving guest; but I think I'd rather
go with the "special-case xenheap pages" option.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
