[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v16 1/3] mem_sharing: fix sharability check during fork reset



On Wed, Apr 22, 2020 at 06:42:42AM -0600, Tamas K Lengyel wrote:
> On Wed, Apr 22, 2020 at 3:00 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Tue, Apr 21, 2020 at 10:47:23AM -0700, Tamas K Lengyel wrote:
> > > @@ -666,20 +670,31 @@ static int page_make_sharable(struct domain *d,
> > >       */
> > >      if ( page->count_info != (PGC_allocated | (2 + expected_refcnt)) )
> > >      {
> > > -        spin_unlock(&d->page_alloc_lock);
> > >          /* Return type count back to zero */
> > >          put_page_and_type(page);
> > > -        return -E2BIG;
> > > +        rc = -E2BIG;
> > > +        goto out;
> > > +    }
> > > +
> > > +    rc = 0;
> > > +
> > > +    if ( validate_only )
> > > +    {
> > > +        put_page_and_type(page);
> >
> > You seem to check some page attributes but then put the page again,
> > which looks racy to me. Since you put the page, couldn't the checks
> > that you have performed be stale by the point the data is consumed by
> > the caller?
> 
> During fork reset when this is called with validate_only = true the
> domain is paused. Furthermore, fork reset is only for forks that have
> no device model running, so nothing is interacting with its memory
> that could acquire extra references. So no, this isn't racy since
> there is nothing to race against that I'm aware of. Also, this check
> is really to check for special pages, all of which are setup during
> the initial fork process, not during runtime of the fork.

Right, it would feel safer to me however if you just return from
page_make_sharable while having a page reference, and drop it in
mem_sharing_fork_reset if the page shouldn't be removed from the fork.

This way you could also avoid having to take an extra reference just
after returning from nominate_page in mem_sharing_fork_reset.
page_make_sharable already returns while having taken an extra
reference to the page in the non validate only case anyway.

> >
> > > +        goto out;
> > >      }
> > >
> > >      page_set_owner(page, dom_cow);
> > >      drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> > >      page_list_del(page, &d->page_list);
> > > -    spin_unlock(&d->page_alloc_lock);
> > >
> > > +out:
> > > +    if ( !validate_only )
> > > +        spin_unlock(&d->page_alloc_lock);
> > >      if ( drop_dom_ref )
> > >          put_domain(d);
> > > -    return 0;
> > > +
> > > +    return rc;
> > >  }
> > >
> > >  static int page_make_private(struct domain *d, struct page_info *page)
> > > @@ -809,8 +824,8 @@ static int debug_gref(struct domain *d, grant_ref_t 
> > > ref)
> > >      return debug_gfn(d, gfn);
> > >  }
> > >
> > > -static int nominate_page(struct domain *d, gfn_t gfn,
> > > -                         int expected_refcnt, shr_handle_t *phandle)
> > > +static int nominate_page(struct domain *d, gfn_t gfn, int 
> > > expected_refcnt,
> >
> > Is there any reason for expected_refcnt to be signed? All callers use
> > unsigned values.
> 
> No reason. It's just how the code was written by the original author
> and we never changed it.

Since you are already changing those lines, can I ask you to also
change it to unsigned in the places that you touch?

Thanks, Roger.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.