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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.