Xen devel - May 2013 - xc_map_foreign_bulk() memory leak in ARM version?

Hi Xen folks!

I''ve faced with one strange thing in ARM version of Xen: when I use
xc_map_foreign_bulk() to map some memory from domU to dom0, after unmap()
for previous returned address - memory is not freed at all.

Let''s look at call stack:

xc_map_foreign() ->
  linux_privcmd_map_foreign_bulk() ->
    {
    addr = mmap(fd);
    ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2 );
    }  ->
      alloc_empty_pages() ->
        alloc_xenballoned_pages();

So, I think that unmap(addr) must call free_xenballoned_pages(), but this
doesn''t happen. =(
Let me note, that mmap() knows about privcmd_close() function, and it is
the place where free_xenballoned_pages() is called, So we have that unmap()
doesn''t call privcmd_close() at all. It''s something strange
for me.

Can somebody show me the place of my misunderstanding, or is it a real bug?

Best regards,
Nyashka


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Thu, 2013-05-16 at 19:36 +0400, Nyashka Surovski
wrote:
> Hi Xen folks!
> 
> 
> I''ve faced with one strange thing in ARM version of Xen: when I
use
> xc_map_foreign_bulk() to map some memory from domU to dom0, after
> unmap() for previous returned address - memory is not freed at all.
> 
> 
> Let''s look at call stack:
> 
> 
> xc_map_foreign() -> 
>   linux_privcmd_map_foreign_bulk() -> 
>     { 
>     addr = mmap(fd); 
>     ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2 ); 
>     }  ->
>       alloc_empty_pages() ->
>         alloc_xenballoned_pages();
> 
> So, I think that unmap(addr) must call free_xenballoned_pages(), but
> this doesn''t happen. =(
> Let me note, that mmap() knows about privcmd_close() function, and it
> is the place where free_xenballoned_pages() is called, So we have that
> unmap() doesn''t call privcmd_close() at all. It''s
something strange
> for me.
> 
> Can somebody show me the place of my misunderstanding, or is it a real
> bug?
Do you mean munmap()?

I think munmap will eventually end up calling close, when the references
to the vma etc are gone. Since the code path is a bit twisty I''d be
tempted to throw in a debug printk to confirm though.

Can you share your usercode?

Ian.

On Fri, 17 May 2013 11:14:00 +0100
Ian Campbell <ian.campbell@citrix.com> wrote:
> On Thu, 2013-05-16 at 19:36 +0400, Nyashka Surovski wrote:
> > Hi Xen folks!
> > 
> > 
> > I''ve faced with one strange thing in ARM version of Xen: when
I use
> > xc_map_foreign_bulk() to map some memory from domU to dom0, after
> > unmap() for previous returned address - memory is not freed at all.
> > 
> > 
> > Let''s look at call stack:
> > 
> > 
> > xc_map_foreign() -> 
> >   linux_privcmd_map_foreign_bulk() -> 
> >     { 
> >     addr = mmap(fd); 
> >     ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2 ); 
> >     }  ->
> >       alloc_empty_pages() ->
> >         alloc_xenballoned_pages();
> > 
> > So, I think that unmap(addr) must call free_xenballoned_pages(), but
> > this doesn''t happen. =(
> > Let me note, that mmap() knows about privcmd_close() function, and
> > it is the place where free_xenballoned_pages() is called, So we
> > have that unmap() doesn''t call privcmd_close() at all.
It''s
> > something strange for me.
> > 
> > Can somebody show me the place of my misunderstanding, or is it a
> > real bug?
> 
> Do you mean munmap()?
> 
> I think munmap will eventually end up calling close, when the
> references to the vma etc are gone. Since the code path is a bit
> twisty I''d be tempted to throw in a debug printk to confirm
though.
> 
> Can you share your usercode?
I dealt with that a lot during PVH debug. Yes, munmap will call close.
If the process exits without calling munmap, then do_exit -> exit_mm will
result in call to privcmd_close.

hope that helps.
Mukesh

Oh.. We have found out the root of the problem.
In our version of code there was a mistake in condition inside
privcmd_close().

It was:
  if (!xen_feature(XENFEAT_auto_translated_physmap || !numpgs || !pages))

But the right one is:
  if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)

Looks like typo.

git blame shows that this version of code was added in commit
d71f513985c22f1050295d1a7e4327cf9fb060da on Oct 17 2012, so you can check
it.

Thanks for your replies!

Best regards,
Nyashka


2013/5/17 Mukesh Rathor <mukesh.rathor@oracle.com>
> On Fri, 17 May 2013 11:14:00 +0100
> Ian Campbell <ian.campbell@citrix.com> wrote:
>
> > On Thu, 2013-05-16 at 19:36 +0400, Nyashka Surovski wrote:
> > > Hi Xen folks!
> > >
> > >
> > > I''ve faced with one strange thing in ARM version of Xen:
when I use
> > > xc_map_foreign_bulk() to map some memory from domU to dom0, after
> > > unmap() for previous returned address - memory is not freed at
all.
> > >
> > >
> > > Let''s look at call stack:
> > >
> > >
> > > xc_map_foreign() ->
> > >   linux_privcmd_map_foreign_bulk() ->
> > >     {
> > >     addr = mmap(fd);
> > >     ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2 );
> > >     }  ->
> > >       alloc_empty_pages() ->
> > >         alloc_xenballoned_pages();
> > >
> > > So, I think that unmap(addr) must call free_xenballoned_pages(),
but
> > > this doesn''t happen. =(
> > > Let me note, that mmap() knows about privcmd_close() function,
and
> > > it is the place where free_xenballoned_pages() is called, So we
> > > have that unmap() doesn''t call privcmd_close() at all.
It''s
> > > something strange for me.
> > >
> > > Can somebody show me the place of my misunderstanding, or is it a
> > > real bug?
> >
> > Do you mean munmap()?
> >
> > I think munmap will eventually end up calling close, when the
> > references to the vma etc are gone. Since the code path is a bit
> > twisty I''d be tempted to throw in a debug printk to confirm
though.
> >
> > Can you share your usercode?
>
> I dealt with that a lot during PVH debug. Yes, munmap will call close.
> If the process exits without calling munmap, then do_exit -> exit_mm
will
> result in call to privcmd_close.
>
> hope that helps.
> Mukesh
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On Mon, 2013-05-20 at 11:55 +0400, Nyashka Surovski
wrote:
> Oh.. We have found out the root of the problem.
> In our version of code there was a mistake in condition inside
> privcmd_close().
> 
> 
> It was: 
>   if (!xen_feature(XENFEAT_auto_translated_physmap || !numpgs || !
> pages))
> 
> 
> 
> But the right one is:
>   if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !
> pages)
> 
> 
> Looks like typo.
Absolutely!

This was picked up by Dan Carpenter with "xen/privcmd: fix condition in
privcmd_close()" ages ago but accidentally got dropped. Luckily he sent
a ping about it last week and I think it has now been picked up into
Konrad''s tree and will no doubt be going to Linus soon.

Thanks for tracking this down.

Ian.

On Mon, May 20, 2013 at 11:55:48AM +0400, Nyashka Surovski
wrote:
> Oh.. We have found out the root of the problem.
> In our version of code there was a mistake in condition inside
> privcmd_close().
> 
> It was:
>   if (!xen_feature(XENFEAT_auto_translated_physmap || !numpgs || !pages))
> 
> But the right one is:
>   if (!xen_feature(XENFEAT_auto_translated_physmap) || !numpgs || !pages)
> 
> Looks like typo.
> 
> git blame shows that this version of code was added in commit
> d71f513985c22f1050295d1a7e4327cf9fb060da on Oct 17 2012, so you can check
> it.
the fix for that is going to be sent to Linus shortly.
> 
> Thanks for your replies!
> 
> Best regards,
> Nyashka
> 
> 
> 2013/5/17 Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> > On Fri, 17 May 2013 11:14:00 +0100
> > Ian Campbell <ian.campbell@citrix.com> wrote:
> >
> > > On Thu, 2013-05-16 at 19:36 +0400, Nyashka Surovski wrote:
> > > > Hi Xen folks!
> > > >
> > > >
> > > > I''ve faced with one strange thing in ARM version of
Xen: when I use
> > > > xc_map_foreign_bulk() to map some memory from domU to dom0,
after
> > > > unmap() for previous returned address - memory is not freed
at all.
> > > >
> > > >
> > > > Let''s look at call stack:
> > > >
> > > >
> > > > xc_map_foreign() ->
> > > >   linux_privcmd_map_foreign_bulk() ->
> > > >     {
> > > >     addr = mmap(fd);
> > > >     ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2 );
> > > >     }  ->
> > > >       alloc_empty_pages() ->
> > > >         alloc_xenballoned_pages();
> > > >
> > > > So, I think that unmap(addr) must call
free_xenballoned_pages(), but
> > > > this doesn''t happen. =(
> > > > Let me note, that mmap() knows about privcmd_close()
function, and
> > > > it is the place where free_xenballoned_pages() is called, So
we
> > > > have that unmap() doesn''t call privcmd_close() at
all. It''s
> > > > something strange for me.
> > > >
> > > > Can somebody show me the place of my misunderstanding, or is
it a
> > > > real bug?
> > >
> > > Do you mean munmap()?
> > >
> > > I think munmap will eventually end up calling close, when the
> > > references to the vma etc are gone. Since the code path is a bit
> > > twisty I''d be tempted to throw in a debug printk to
confirm though.
> > >
> > > Can you share your usercode?
> >
> > I dealt with that a lot during PVH debug. Yes, munmap will call close.
> > If the process exits without calling munmap, then do_exit ->
exit_mm will
> > result in call to privcmd_close.
> >
> > hope that helps.
> > Mukesh
> >
> >
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel