Re: [Xen-devel] [PATCH] fix BUG in gnttab_unpopulate_status_frames()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel