libxc: correctly unmap pages after core-dumping a pv guest

libxc: correctly unmap pages after core-dumping a pv guest

[Markus Groß]

Hi,

while implementing core dumping functionality for the libxl driver
of libvirt, I discovered an issue with mapping pages of a pv guest.

After dumping the core of a pv guest the domain was not cleared up
properly and some pages were not unmapped. This issue is similar
to the one reported here:
http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01314.html

In xc_domain_dumpcore_via_callback in the file xc_core.c the function
xc_core_arch_map_p2m is called to map P2M_FL_ENTRIES pages to the variable p2m.
But to unmap the pages later, the dinfo->p2m_size has to be set accordingly.
This was not done, instead a variable named p2m_size was set.
This way P2M_FL_ENTRIES was always zero and the pages were left mapped.

The following patch fixes this.

Best regards,
Markus
 
diff -r 7c7ef1b6f4e5 tools/libxc/xc_core.c
--- a/tools/libxc/xc_core.c     Tue Apr 26 14:11:18 2011 +0100
+++ b/tools/libxc/xc_core.c     Mon May 23 13:36:23 2011 +0200
@@ -468,7 +468,6 @@
 
     int auto_translated_physmap;
     xen_pfn_t *p2m = NULL;
-    unsigned long p2m_size = 0;
     struct xen_dumpcore_p2m *p2m_array = NULL;
 
     uint64_t *pfn_array = NULL;
@@ -569,7 +568,7 @@
         }
 
         sts = xc_core_arch_map_p2m(xch, dinfo->guest_width, &info, live_shinfo,
-                                   &p2m, &p2m_size);
+                                   &p2m, &dinfo->p2m_size);
         if ( sts != 0 )
             goto out;
     }

Re: libxc: correctly unmap pages after core-dumping a pv guest

[Markus Groß]

I forgot to put [PATCH] in the subject, sorry about that.

Am Montag 23 Mai 2011 13:42:28 schrieb Markus Groß:
> Hi,
> 
> while implementing core dumping functionality for the libxl driver
> of libvirt, I discovered an issue with mapping pages of a pv guest.
> 
> After dumping the core of a pv guest the domain was not cleared up
> properly and some pages were not unmapped. This issue is similar
> to the one reported here:
> http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01314.html
> 
> In xc_domain_dumpcore_via_callback in the file xc_core.c the function
> xc_core_arch_map_p2m is called to map P2M_FL_ENTRIES pages to the variable p2m.
> But to unmap the pages later, the dinfo->p2m_size has to be set accordingly.
> This was not done, instead a variable named p2m_size was set.
> This way P2M_FL_ENTRIES was always zero and the pages were left mapped.
> 
> The following patch fixes this.
> 
> Best regards,
> Markus
>  
> diff -r 7c7ef1b6f4e5 tools/libxc/xc_core.c
> --- a/tools/libxc/xc_core.c     Tue Apr 26 14:11:18 2011 +0100
> +++ b/tools/libxc/xc_core.c     Mon May 23 13:36:23 2011 +0200
> @@ -468,7 +468,6 @@
>  
>      int auto_translated_physmap;
>      xen_pfn_t *p2m = NULL;
> -    unsigned long p2m_size = 0;
>      struct xen_dumpcore_p2m *p2m_array = NULL;
>  
>      uint64_t *pfn_array = NULL;
> @@ -569,7 +568,7 @@
>          }
>  
>          sts = xc_core_arch_map_p2m(xch, dinfo->guest_width, &info, live_shinfo,
> -                                   &p2m, &p2m_size);
> +                                   &p2m, &dinfo->p2m_size);
>          if ( sts != 0 )
>              goto out;
>      }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 

Re: libxc: correctly unmap pages after core-dumping a pv guest

[Ian Campbell]

On Mon, 2011-05-23 at 13:24 +0100, Markus Groß wrote:
> I forgot to put [PATCH] in the subject, sorry about that.

That doesn't matter too much (it just increases the chance of attracting
a maintainers attention). What does matter though is that we need your
Signed-off-by per the DCO, [0]. Please can you resend with that?

Thanks!
Ian.

[0] http://lwn.net/Articles/437739/

> 
> Am Montag 23 Mai 2011 13:42:28 schrieb Markus Groß:
> > Hi,
> > 
> > while implementing core dumping functionality for the libxl driver
> > of libvirt, I discovered an issue with mapping pages of a pv guest.
> > 
> > After dumping the core of a pv guest the domain was not cleared up
> > properly and some pages were not unmapped. This issue is similar
> > to the one reported here:
> > http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01314.html
> > 
> > In xc_domain_dumpcore_via_callback in the file xc_core.c the function
> > xc_core_arch_map_p2m is called to map P2M_FL_ENTRIES pages to the variable p2m.
> > But to unmap the pages later, the dinfo->p2m_size has to be set accordingly.
> > This was not done, instead a variable named p2m_size was set.
> > This way P2M_FL_ENTRIES was always zero and the pages were left mapped.
> > 
> > The following patch fixes this.
> > 
> > Best regards,
> > Markus
> >  
> > diff -r 7c7ef1b6f4e5 tools/libxc/xc_core.c
> > --- a/tools/libxc/xc_core.c     Tue Apr 26 14:11:18 2011 +0100
> > +++ b/tools/libxc/xc_core.c     Mon May 23 13:36:23 2011 +0200
> > @@ -468,7 +468,6 @@
> >  
> >      int auto_translated_physmap;
> >      xen_pfn_t *p2m = NULL;
> > -    unsigned long p2m_size = 0;
> >      struct xen_dumpcore_p2m *p2m_array = NULL;
> >  
> >      uint64_t *pfn_array = NULL;
> > @@ -569,7 +568,7 @@
> >          }
> >  
> >          sts = xc_core_arch_map_p2m(xch, dinfo->guest_width, &info, live_shinfo,
> > -                                   &p2m, &p2m_size);
> > +                                   &p2m, &dinfo->p2m_size);
> >          if ( sts != 0 )
> >              goto out;
> >      }
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> > 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] libxc: correctly unmap pages after core-dumping a pv guest

[Markus Groß]

Hi,

while implementing core dumping functionality for the libxl driver
of libvirt, I discovered an issue with mapping pages of a pv guest.

After dumping the core of a pv guest the domain was not cleared up
properly and some pages were not unmapped. This issue is similar
to the one reported here:
http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01314.html

In xc_domain_dumpcore_via_callback in the file xc_core.c the function
xc_core_arch_map_p2m is called to map P2M_FL_ENTRIES pages to the  
variable p2m.
But to unmap the pages later, the dinfo->p2m_size has to be set accordingly.
This was not done, instead a variable named p2m_size was set.
This way P2M_FL_ENTRIES was always zero and the pages were left mapped.

The following patch fixes this.

Best regards,
Markus

Signed-off-by: Markus Groß <gross <at> univention.de>

diff -r 7c7ef1b6f4e5 tools/libxc/xc_core.c
--- a/tools/libxc/xc_core.c     Tue Apr 26 14:11:18 2011 +0100
+++ b/tools/libxc/xc_core.c     Mon May 23 13:36:23 2011 +0200
@@ -468,7 +468,6 @@

      int auto_translated_physmap;
      xen_pfn_t *p2m = NULL;
-    unsigned long p2m_size = 0;
      struct xen_dumpcore_p2m *p2m_array = NULL;

      uint64_t *pfn_array = NULL;
@@ -569,7 +568,7 @@
          }

          sts = xc_core_arch_map_p2m(xch, dinfo->guest_width, &info,  
live_shinfo,
-                                   &p2m, &p2m_size);
+                                   &p2m, &dinfo->p2m_size);
          if ( sts != 0 )
              goto out;
      }

Re: libxc: correctly unmap pages after core-dumping a pv guest

[Ian Jackson]

Markus Groß writes ("[Xen-devel] libxc: correctly unmap pages after core-dumping a pv guest"):
> But to unmap the pages later, the dinfo->p2m_size has to be set accordingly.
> This was not done, instead a variable named p2m_size was set.
> This way P2M_FL_ENTRIES was always zero and the pages were left mapped.
> 
> The following patch fixes this.

Thanks.  The existing code here is pretty unfortunate.

In xc_domain_dumpcore_via_callback where you make your change, "dinfo"
refers to a struct domain_info_context which is local to that
function.  But in the called function xc_core_arch_map_p2m_rw, "dinfo"
refers to another identical structure allocated locally to the small
wrapper function xc_core_arch_map_p2m.  All rather tangled.

But, anyway, your change is correct so I have applied it.

Next time, though, can you please be sure to add a Signed-off-by line,
to signify your certification in accordance with the Developer's
Certificate of Origin ?  In this case I'll go ahead as it's only a
couple of lines.

Thanks,
Ian.

>From Documentation/SubmittingPatches in the Linux kernel tree:
 
        Developer's Certificate of Origin 1.1
 
        By making a contribution to this project, I certify that:
 
        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or
 
        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or
 
        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.
 
        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

Re: libxc: correctly unmap pages after core-dumping a pv guest

[Markus Groß]

Quoting Ian Jackson <Ian.Jackson@eu.citrix.com>:

> Markus Groß writes ("[Xen-devel] libxc: correctly unmap pages after  
> core-dumping a pv guest"):
>> But to unmap the pages later, the dinfo->p2m_size has to be set accordingly.
>> This was not done, instead a variable named p2m_size was set.
>> This way P2M_FL_ENTRIES was always zero and the pages were left mapped.
>>
>> The following patch fixes this.
>
> Thanks.  The existing code here is pretty unfortunate.
>
> In xc_domain_dumpcore_via_callback where you make your change, "dinfo"
> refers to a struct domain_info_context which is local to that
> function.  But in the called function xc_core_arch_map_p2m_rw, "dinfo"
> refers to another identical structure allocated locally to the small
> wrapper function xc_core_arch_map_p2m.  All rather tangled.
>
> But, anyway, your change is correct so I have applied it.
>
> Next time, though, can you please be sure to add a Signed-off-by line,
> to signify your certification in accordance with the Developer's
> Certificate of Origin ?  In this case I'll go ahead as it's only a
> couple of lines.
>
> Thanks,
> Ian.

Thanks, although I did repost the patch with a Signed-off-by line here:
http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01469.html

Cheers,
Markus

>
> From Documentation/SubmittingPatches in the Linux kernel tree:
>
>         Developer's Certificate of Origin 1.1
>
>         By making a contribution to this project, I certify that:
>
>         (a) The contribution was created in whole or in part by me and I
>             have the right to submit it under the open source license
>             indicated in the file; or
>
>         (b) The contribution is based upon previous work that, to the best
>             of my knowledge, is covered under an appropriate open source
>             license and I have the right under that license to submit that
>             work with modifications, whether created in whole or in part
>             by me, under the same open source license (unless I am
>             permitted to submit under a different license), as indicated
>             in the file; or
>
>         (c) The contribution was provided directly to me by some other
>             person who certified (a), (b) or (c) and I have not modified
>             it.
>
>         (d) I understand and agree that this project and the contribution
>             are public and that a record of the contribution (including all
>             personal information I submit with it, including my sign-off) is
>             maintained indefinitely and may be redistributed consistent with
>             this project or the open source license(s) involved.
>

Re: libxc: correctly unmap pages after core-dumping a pv guest

[Ian Jackson]

Markus Groß writes ("Re: [Xen-devel] libxc: correctly unmap pages after core-dumping a pv guest"):
> Thanks, although I did repost the patch with a Signed-off-by line here:
> http://lists.xensource.com/archives/html/xen-devel/2011-05/msg01469.html

Sorry, I must have missed that in my backlog.

Regards,
Ian.