Re: [Xen-devel] [PATCH v4 14/18] x86/mem_sharing: check page type count earlier

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jan 20, 2020 at 9:34 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 08.01.2020 18:14, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -652,19 +652,18 @@ static int page_make_sharable(struct domain *d,
> >          return -EBUSY;
> >      }
> >
> > -    /* Change page type and count atomically */
> > -    if ( !get_page_and_type(page, d, PGT_shared_page) )
> > +    /* Check if page is already typed and bail early if it is */
> > +    if ( (page->u.inuse.type_info & PGT_count_mask) != 1 )
> >      {
> >          spin_unlock(&d->page_alloc_lock);
> > -        return -EINVAL;
> > +        return -EEXIST;
> >      }
> >
> > -    /* Check it wasn't already sharable and undo if it was */
> > -    if ( (page->u.inuse.type_info & PGT_count_mask) != 1 )
> > +    /* Change page type and count atomically */
> > +    if ( !get_page_and_type(page, d, PGT_shared_page) )
> >      {
> >          spin_unlock(&d->page_alloc_lock);
> > -        put_page_and_type(page);
> > -        return -EEXIST;
> > +        return -EINVAL;
> >      }
>
> It would seem to me that either the original or the new code cannot
> have worked / work: The original variant checked the count _after_
> having incremented it, i.e. it expected a 0->1 transition. The new
> code checks that the count is 1 _before_ doing the get.
>
> However, even if this was changed to
>
>     if ( page->u.inuse.type_info & PGT_count_mask )
>
> I would recommend against the change: Aiui you build upon the fact
> that a transition to PGT_shared_page can happen only here, and this
> code holds d->page_alloc_lock. But imo this is making the code more
> fragile. In fact I can't easily see why the other two cases where
> PGT_shared_page gets passed to get_page_and_type() can't also
> effect a 0->1 transition. I can only guess from their BUG_ON()-s
> that they assume a reference was already acquired somewhere else.

Hm, right, it certainly looks like this patch isn't needed. It has
been a while now and I don't recall why exactly I was moving the type
count check, it might have just been while I was experimenting and it
never got reverted.

Thanks,
Tamas

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