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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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