xen_disk: fix memory leak

[Qemu-devel] [PATCH v1] xen_disk: fix memory leak

Posted by Olaf Hering 5 years, 4 months ago

There are some code paths that clobber ioreq->buf, which leads to a huge
memory leak after a few hours of runtime. One code path is
qemu_aio_complete, which might be called recursive. Another one is
ioreq_reset, which might clobber ioreq->buf as well.

Add wrappers to free ioreq->buf before reassignment.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 hw/block/xen_disk.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 36eff94f84..e15eefe625 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -103,12 +103,24 @@ struct XenBlkDev {
 
 /* ------------------------------------------------------------- */
 
+static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)
+{
+    if (ioreq->buf)
+        qemu_vfree(ioreq->buf);
+    ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
+}
+static void ioreq_buf_free(struct ioreq *ioreq)
+{
+    if (ioreq->buf)
+        qemu_vfree(ioreq->buf);
+    ioreq->buf = NULL;
+}
 static void ioreq_reset(struct ioreq *ioreq)
 {
+    ioreq_buf_free(ioreq);
     memset(&ioreq->req, 0, sizeof(ioreq->req));
     ioreq->status = 0;
     ioreq->start = 0;
-    ioreq->buf = NULL;
     ioreq->size = 0;
     ioreq->presync = 0;
 
@@ -315,14 +327,14 @@ static void qemu_aio_complete(void *opaque, int ret)
         if (ret == 0) {
             ioreq_grant_copy(ioreq);
         }
-        qemu_vfree(ioreq->buf);
+        ioreq_buf_free(ioreq);
         break;
     case BLKIF_OP_WRITE:
     case BLKIF_OP_FLUSH_DISKCACHE:
         if (!ioreq->req.nr_segments) {
             break;
         }
-        qemu_vfree(ioreq->buf);
+        ioreq_buf_free(ioreq);
         break;
     default:
         break;
@@ -390,12 +402,12 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq)
 {
     struct XenBlkDev *blkdev = ioreq->blkdev;
 
-    ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
+    ioreq_buf_alloc(ioreq, XC_PAGE_SIZE);
     if (ioreq->req.nr_segments &&
         (ioreq->req.operation == BLKIF_OP_WRITE ||
          ioreq->req.operation == BLKIF_OP_FLUSH_DISKCACHE) &&
         ioreq_grant_copy(ioreq)) {
-        qemu_vfree(ioreq->buf);
+        ioreq_buf_free(ioreq);
         goto err;
     }

Re: [Qemu-devel] [PATCH v1] xen_disk: fix memory leak

Posted by no-reply@patchew.org 5 years, 4 months ago

Patchew URL: https://patchew.org/QEMU/20181211160224.22181-1-olaf@aepfle.de/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20181211160224.22181-1-olaf@aepfle.de
Subject: [Qemu-devel] [PATCH v1] xen_disk: fix memory leak
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
aaf3499 xen_disk: fix memory leak

=== OUTPUT BEGIN ===
Checking PATCH 1/1: xen_disk: fix memory leak...
ERROR: braces {} are necessary for all arms of this statement
#26: FILE: hw/block/xen_disk.c:108:
+    if (ioreq->buf)
[...]

ERROR: braces {} are necessary for all arms of this statement
#32: FILE: hw/block/xen_disk.c:114:
+    if (ioreq->buf)
[...]

total: 2 errors, 0 warnings, 55 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181211160224.22181-1-olaf@aepfle.de/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v1] xen_disk: fix memory leak

Posted by Anthony PERARD 5 years, 4 months ago

On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> There are some code paths that clobber ioreq->buf, which leads to a huge
> memory leak after a few hours of runtime. One code path is
> qemu_aio_complete, which might be called recursive. Another one is

I think it's s/recursive/recursively/.

> ioreq_reset, which might clobber ioreq->buf as well.
> 
> Add wrappers to free ioreq->buf before reassignment.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

That patch seems fine, with a few coding style issues, and is going to
be needed to be forward ported to Paul's reimplementation (not yet
merged).

> ---
>  hw/block/xen_disk.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> index 36eff94f84..e15eefe625 100644
> --- a/hw/block/xen_disk.c
> +++ b/hw/block/xen_disk.c
> @@ -103,12 +103,24 @@ struct XenBlkDev {
>  
>  /* ------------------------------------------------------------- */
>  
> +static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)

You have the parameter `alignment` but don't actually use it, I don't
think it's needed.

> +{
> +    if (ioreq->buf)
> +        qemu_vfree(ioreq->buf);

You could call ioreq_buf_free here instead of duplicating the code.

> +    ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
> +}
> +static void ioreq_buf_free(struct ioreq *ioreq)
> +{
> +    if (ioreq->buf)
> +        qemu_vfree(ioreq->buf);
> +    ioreq->buf = NULL;
> +}

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [Xen-devel] [PATCH v1] xen_disk: fix memory leak

Posted by Paul Durrant 5 years, 4 months ago

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Anthony PERARD
> Sent: 13 December 2018 11:34
> To: Olaf Hering <olaf@aepfle.de>
> Cc: Kevin Wolf <kwolf@redhat.com>; Stefano Stabellini
> <sstabellini@kernel.org>; open list:Block layer core <qemu-
> block@nongnu.org>; qemu-devel@nongnu.org; Max Reitz <mreitz@redhat.com>;
> open list:X86 <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH v1] xen_disk: fix memory leak
> 
> On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> > There are some code paths that clobber ioreq->buf, which leads to a huge
> > memory leak after a few hours of runtime. One code path is
> > qemu_aio_complete, which might be called recursive. Another one is
> 
> I think it's s/recursive/recursively/.
> 
> > ioreq_reset, which might clobber ioreq->buf as well.
> >
> > Add wrappers to free ioreq->buf before reassignment.
> >
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> That patch seems fine, with a few coding style issues, and is going to
> be needed to be forward ported to Paul's reimplementation (not yet
> merged).

I already posted a patch from Tim Smith (re-based to the new xen-block datapath) that should fix this issue.

  Paul

> 
> > ---
> >  hw/block/xen_disk.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 36eff94f84..e15eefe625 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -103,12 +103,24 @@ struct XenBlkDev {
> >
> >  /* ------------------------------------------------------------- */
> >
> > +static void ioreq_buf_alloc(struct ioreq *ioreq, size_t alignment)
> 
> You have the parameter `alignment` but don't actually use it, I don't
> think it's needed.
> 
> > +{
> > +    if (ioreq->buf)
> > +        qemu_vfree(ioreq->buf);
> 
> You could call ioreq_buf_free here instead of duplicating the code.
> 
> > +    ioreq->buf = qemu_memalign(XC_PAGE_SIZE, ioreq->size);
> > +}
> > +static void ioreq_buf_free(struct ioreq *ioreq)
> > +{
> > +    if (ioreq->buf)
> > +        qemu_vfree(ioreq->buf);
> > +    ioreq->buf = NULL;
> > +}
> 
> Thanks,
> 
> --
> Anthony PERARD
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel