[PATCH] xen: privcmd: do not return pages which we have failed to unmap

[PATCH] xen: privcmd: do not return pages which we have failed to unmap

[Ian Campbell]

This failure represents a hypervisor issue, but if it does occur then nothing
good can come of returning pages which still refer to a foreign owned page
into the general allocation pool.

Instead we are foced to leak them. Log that we have done so.

The potential for failure only exists for autotranslated guest (e.g. ARM and
x86 PVH).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xenproject.org
---
 drivers/xen/privcmd.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 8e74590..2efc720 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -533,11 +533,17 @@ static void privcmd_close(struct vm_area_struct *vma)
 {
 	struct page **pages = vma->vm_private_data;
 	int numpgs = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+	int rc;
 
 	if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
 		return;
 
-	xen_unmap_domain_mfn_range(vma, numpgs, pages);
+	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
+	if (rc < 0) {
+		pr_crit("unable to unmap MFN range: leaking %d pages\n",
+			numpgs);
+		return;
+	}
 	free_xenballooned_pages(numpgs, pages);
 	kfree(pages);
 }
-- 
1.7.10.4

Re: [PATCH] xen: privcmd: do not return pages which we have failed to unmap

[David Vrabel]

On 04/12/13 15:52, Ian Campbell wrote:
> This failure represents a hypervisor issue, but if it does occur then nothing
> good can come of returning pages which still refer to a foreign owned page
> into the general allocation pool.
> 
> Instead we are foced to leak them. Log that we have done so.
                 ^forced
> 
> The potential for failure only exists for autotranslated guest (e.g. ARM and
> x86 PVH).
[...]
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -533,11 +533,17 @@ static void privcmd_close(struct vm_area_struct *vma)
>  {
>  	struct page **pages = vma->vm_private_data;
>  	int numpgs = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> +	int rc;
>  
>  	if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
>  		return;
>  
> -	xen_unmap_domain_mfn_range(vma, numpgs, pages);
> +	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
> +	if (rc < 0) {
> +		pr_crit("unable to unmap MFN range: leaking %d pages\n",
> +			numpgs);

kfree(pages) here?  I think that would be safe. Although at this point
it probably doesn't really matter.

David

Re: [PATCH] xen: privcmd: do not return pages which we have failed to unmap

[Ian Campbell]

> > -	xen_unmap_domain_mfn_range(vma, numpgs, pages);
> > +	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
> > +	if (rc < 0) {
> > +		pr_crit("unable to unmap MFN range: leaking %d pages\n",
> > +			numpgs);
> 
> kfree(pages) here?  I think that would be safe. Although at this point
> it probably doesn't really matter.

I suppose we might as well not make it any worse than it needs to be and
it's easy enough to arrange.

---------------------8<---------------------

>From 900f1e903bacf376800b078aef03e8d5ff524562 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Wed, 4 Dec 2013 14:19:52 +0000
Subject: [PATCH] xen: privcmd: do not return pages which we have failed to
 unmap

This failure represents a hypervisor issue, but if it does occur then nothing
good can come of returning pages which still refer to a foreign owned page
into the general allocation pool.

Instead we are forced to leak them. Log that we have done so.

The potential for failure only exists for autotranslated guest (e.g. ARM and
x86 PVH).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xenproject.org
---
v2: Don't leak the actual pages array as well
    Log rc
---
 drivers/xen/privcmd.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 8e74590..569a13b 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -533,12 +533,17 @@ static void privcmd_close(struct vm_area_struct *vma)
 {
 	struct page **pages = vma->vm_private_data;
 	int numpgs = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+	int rc;
 
 	if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
 		return;
 
-	xen_unmap_domain_mfn_range(vma, numpgs, pages);
-	free_xenballooned_pages(numpgs, pages);
+	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
+	if (rc == 0)
+		free_xenballooned_pages(numpgs, pages);
+	else
+		pr_crit("unable to unmap MFN range: leaking %d pages. rc=%d\n",
+			numpgs, rc);
 	kfree(pages);
 }
 
-- 
1.7.10.4

Re: [PATCH] xen: privcmd: do not return pages which we have failed to unmap

[David Vrabel]

On 04/12/13 16:03, Ian Campbell wrote:
> Subject: [PATCH] xen: privcmd: do not return pages which we have failed to
>  unmap
> 
> This failure represents a hypervisor issue, but if it does occur then nothing
> good can come of returning pages which still refer to a foreign owned page
> into the general allocation pool.
> 
> Instead we are forced to leak them. Log that we have done so.
> 
> The potential for failure only exists for autotranslated guest (e.g. ARM and
> x86 PVH).

Reviewed-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH] xen: privcmd: do not return pages which we have failed to unmap

[Stefano Stabellini]

On Wed, 4 Dec 2013, Ian Campbell wrote:
> >From 900f1e903bacf376800b078aef03e8d5ff524562 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Wed, 4 Dec 2013 14:19:52 +0000
> Subject: [PATCH] xen: privcmd: do not return pages which we have failed to
>  unmap
> 
> This failure represents a hypervisor issue, but if it does occur then nothing
> good can come of returning pages which still refer to a foreign owned page
> into the general allocation pool.
> 
> Instead we are forced to leak them. Log that we have done so.
> 
> The potential for failure only exists for autotranslated guest (e.g. ARM and
> x86 PVH).
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: xen-devel@lists.xenproject.org
> ---
> v2: Don't leak the actual pages array as well
>     Log rc


Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Konrad, you can go ahead and add it to xentip, unless you would rather
have me do it.


>  drivers/xen/privcmd.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 8e74590..569a13b 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -533,12 +533,17 @@ static void privcmd_close(struct vm_area_struct *vma)
>  {
>  	struct page **pages = vma->vm_private_data;
>  	int numpgs = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> +	int rc;
>  
>  	if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
>  		return;
>  
> -	xen_unmap_domain_mfn_range(vma, numpgs, pages);
> -	free_xenballooned_pages(numpgs, pages);
> +	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
> +	if (rc == 0)
> +		free_xenballooned_pages(numpgs, pages);
> +	else
> +		pr_crit("unable to unmap MFN range: leaking %d pages. rc=%d\n",
> +			numpgs, rc);
>  	kfree(pages);
>  }
>  
> -- 
> 1.7.10.4
> 
> 
> 

Re: [PATCH] xen: privcmd: do not return pages which we have failed to unmap

[Konrad Rzeszutek Wilk]

On Fri, Dec 06, 2013 at 05:58:25PM +0000, Stefano Stabellini wrote:
> On Wed, 4 Dec 2013, Ian Campbell wrote:
> > >From 900f1e903bacf376800b078aef03e8d5ff524562 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Wed, 4 Dec 2013 14:19:52 +0000
> > Subject: [PATCH] xen: privcmd: do not return pages which we have failed to
> >  unmap
> > 
> > This failure represents a hypervisor issue, but if it does occur then nothing
> > good can come of returning pages which still refer to a foreign owned page
> > into the general allocation pool.
> > 
> > Instead we are forced to leak them. Log that we have done so.
> > 
> > The potential for failure only exists for autotranslated guest (e.g. ARM and
> > x86 PVH).
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: David Vrabel <david.vrabel@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: xen-devel@lists.xenproject.org
> > ---
> > v2: Don't leak the actual pages array as well
> >     Log rc
> 
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Konrad, you can go ahead and add it to xentip, unless you would rather
> have me do it.

Why don't you do it. Thanks!
> 
> 
> >  drivers/xen/privcmd.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index 8e74590..569a13b 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -533,12 +533,17 @@ static void privcmd_close(struct vm_area_struct *vma)
> >  {
> >  	struct page **pages = vma->vm_private_data;
> >  	int numpgs = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> > +	int rc;
> >  
> >  	if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
> >  		return;
> >  
> > -	xen_unmap_domain_mfn_range(vma, numpgs, pages);
> > -	free_xenballooned_pages(numpgs, pages);
> > +	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
> > +	if (rc == 0)
> > +		free_xenballooned_pages(numpgs, pages);
> > +	else
> > +		pr_crit("unable to unmap MFN range: leaking %d pages. rc=%d\n",
> > +			numpgs, rc);
> >  	kfree(pages);
> >  }
> >  
> > -- 
> > 1.7.10.4
> > 
> > 
> > 

Re: [PATCH] xen: privcmd: do not return pages which we have failed to unmap

[Stefano Stabellini]

On Fri, 6 Dec 2013, Konrad Rzeszutek Wilk wrote:
> On Fri, Dec 06, 2013 at 05:58:25PM +0000, Stefano Stabellini wrote:
> > On Wed, 4 Dec 2013, Ian Campbell wrote:
> > > >From 900f1e903bacf376800b078aef03e8d5ff524562 Mon Sep 17 00:00:00 2001
> > > From: Ian Campbell <ian.campbell@citrix.com>
> > > Date: Wed, 4 Dec 2013 14:19:52 +0000
> > > Subject: [PATCH] xen: privcmd: do not return pages which we have failed to
> > >  unmap
> > > 
> > > This failure represents a hypervisor issue, but if it does occur then nothing
> > > good can come of returning pages which still refer to a foreign owned page
> > > into the general allocation pool.
> > > 
> > > Instead we are forced to leak them. Log that we have done so.
> > > 
> > > The potential for failure only exists for autotranslated guest (e.g. ARM and
> > > x86 PVH).
> > > 
> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > > Cc: David Vrabel <david.vrabel@citrix.com>
> > > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Cc: xen-devel@lists.xenproject.org
> > > ---
> > > v2: Don't leak the actual pages array as well
> > >     Log rc
> > 
> > 
> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Konrad, you can go ahead and add it to xentip, unless you would rather
> > have me do it.
> 
> Why don't you do it. Thanks!

done


> > 
> > >  drivers/xen/privcmd.c |    9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > > index 8e74590..569a13b 100644
> > > --- a/drivers/xen/privcmd.c
> > > +++ b/drivers/xen/privcmd.c
> > > @@ -533,12 +533,17 @@ static void privcmd_close(struct vm_area_struct *vma)
> > >  {
> > >  	struct page **pages = vma->vm_private_data;
> > >  	int numpgs = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
> > > +	int rc;
> > >  
> > >  	if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
> > >  		return;
> > >  
> > > -	xen_unmap_domain_mfn_range(vma, numpgs, pages);
> > > -	free_xenballooned_pages(numpgs, pages);
> > > +	rc = xen_unmap_domain_mfn_range(vma, numpgs, pages);
> > > +	if (rc == 0)
> > > +		free_xenballooned_pages(numpgs, pages);
> > > +	else
> > > +		pr_crit("unable to unmap MFN range: leaking %d pages. rc=%d\n",
> > > +			numpgs, rc);
> > >  	kfree(pages);
> > >  }
> > >  
> > > -- 
> > > 1.7.10.4
> > > 
> > > 
> > > 
>