Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> Calling _put_page_type while also holding the page_lock
> for that page can cause a deadlock.

I can't immediately see what the mem_sharing_page_[un]lock() functions
are meant to be protecting against.  Is there a comment anywhere
describing what they're used for or how they work?

Because...

> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> ---
> v3: simplified patch by keeping the additional references already in-place
> ---
>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index dfc279d371..e2f74ac770 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct 
> page_info *page)
>          return -EBUSY;
>      }
>  
> -    /* We can only change the type if count is one */
> -    /* Because we are locking pages individually, we need to drop
> -     * the lock here, while the page is typed. We cannot risk the 
> -     * race of page_unlock and then put_page_type. */

...the comment you're removing explicitly says that what you're doing is
"risk"-y.; but you don't explain at all why the comment is wrong.

Ultimately you're the maintainer, and this is non-security-supported, so
if you insist it's safe, I won't oppose it; but it seems like having
some clarity about what is or is not risky and why would be helpful.

 -George

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