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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: George Dunlap <george.dunlap@xxxxxxxxxx>
> Sent: 02 August 2019 16:28
> To: Jan Beulich <JBeulich@xxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian 
> Jackson
> <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Konrad 
> Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH] fix BUG in gnttab_unpopulate_status_frames()
> 
> 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.

Ok, I see you've committed a patch to that effect while I was away.

  Paul

> 
>  -George

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