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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Apr 22, 2020 at 9:50 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> 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.

Ah yea, that would make sense. Good idea!

> > >
> > > > +        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?

Sure thing.

Thanks,
Tamas