2010-11-06 09:07:03

by Jiri Slaby

[permalink] [raw]
Subject: [PATCH] x86: pci/xen, fix memory leak

Stanse found that xen_setup_msi_irqs leaks memory when
xen_allocate_pirq fails. Free the memory in that fail path.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: [email protected]
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
---
arch/x86/pci/xen.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
index 117f5b8..d7b5109 100644
--- a/arch/x86/pci/xen.c
+++ b/arch/x86/pci/xen.c
@@ -147,8 +147,10 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
irq = xen_allocate_pirq(v[i], 0, /* not sharable */
(type == PCI_CAP_ID_MSIX) ?
"pcifront-msi-x" : "pcifront-msi");
- if (irq < 0)
- return -1;
+ if (irq < 0) {
+ ret = -1;
+ goto free;
+ }

ret = set_irq_msi(irq, msidesc);
if (ret)
@@ -164,7 +166,7 @@ error:
if (ret == -ENODEV)
dev_err(&dev->dev, "Xen PCI frontend has not registered" \
" MSI/MSI-X support!\n");
-
+free:
kfree(v);
return ret;
}
--
1.7.3.1


2010-11-06 09:07:04

by Jiri Slaby

[permalink] [raw]
Subject: [PATCH] Char: virtio_console, fix memory leak

Stanse found that in init_vqs, memory is leaked under certain
circumstanses (the fail path order is incorrect). Fix that by checking
allocations in one turn and free all of them at once if some fails
(some may be NULL, but this is OK).

Signed-off-by: Jiri Slaby <[email protected]>
Cc: Amit Shah <[email protected]>
Cc: [email protected]
Cc: Greg Kroah-Hartman <[email protected]>
---
drivers/char/virtio_console.c | 37 +++++++++----------------------------
1 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 6c1b676..896a2ce 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1547,31 +1547,16 @@ static int init_vqs(struct ports_device *portdev)
nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;

vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
- if (!vqs) {
- err = -ENOMEM;
- goto fail;
- }
io_callbacks = kmalloc(nr_queues * sizeof(vq_callback_t *), GFP_KERNEL);
- if (!io_callbacks) {
- err = -ENOMEM;
- goto free_vqs;
- }
io_names = kmalloc(nr_queues * sizeof(char *), GFP_KERNEL);
- if (!io_names) {
- err = -ENOMEM;
- goto free_callbacks;
- }
portdev->in_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
GFP_KERNEL);
- if (!portdev->in_vqs) {
- err = -ENOMEM;
- goto free_names;
- }
portdev->out_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
GFP_KERNEL);
- if (!portdev->out_vqs) {
+ if (!vqs || !io_callbacks || !io_names || !portdev->in_vqs ||
+ !portdev->out_vqs) {
err = -ENOMEM;
- goto free_invqs;
+ goto free;
}

/*
@@ -1605,7 +1590,7 @@ static int init_vqs(struct ports_device *portdev)
io_callbacks,
(const char **)io_names);
if (err)
- goto free_outvqs;
+ goto free;

j = 0;
portdev->in_vqs[0] = vqs[0];
@@ -1621,23 +1606,19 @@ static int init_vqs(struct ports_device *portdev)
portdev->out_vqs[i] = vqs[j + 1];
}
}
- kfree(io_callbacks);
kfree(io_names);
+ kfree(io_callbacks);
kfree(vqs);

return 0;

-free_names:
- kfree(io_names);
-free_callbacks:
- kfree(io_callbacks);
-free_outvqs:
+free:
kfree(portdev->out_vqs);
-free_invqs:
kfree(portdev->in_vqs);
-free_vqs:
+ kfree(io_names);
+ kfree(io_callbacks);
kfree(vqs);
-fail:
+
return err;
}

--
1.7.3.1

2010-11-06 09:07:38

by Jiri Slaby

[permalink] [raw]
Subject: [PATCH] IDE: ide-floppy, remove unnecessary NULL check

Stanse founf that rq in ide_floppy_callback cannot be NULL, because it
is dereferenced all around. So remove the superfluous check.

This appeared after blk_* macors removal.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
---
drivers/ide/ide-floppy.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 5406b6e..536ff68 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -73,7 +73,7 @@ static int ide_floppy_callback(ide_drive_t *drive, int dsc)
drive->failed_pc = NULL;

if (pc->c[0] == GPCMD_READ_10 || pc->c[0] == GPCMD_WRITE_10 ||
- (rq && rq->cmd_type == REQ_TYPE_BLOCK_PC))
+ rq->cmd_type == REQ_TYPE_BLOCK_PC)
uptodate = 1; /* FIXME */
else if (pc->c[0] == GPCMD_REQUEST_SENSE) {

--
1.7.3.1

2010-11-06 09:07:36

by Jiri Slaby

[permalink] [raw]
Subject: [PATCH] ocfs2: fix memory leak

Stanse found that o2hb_heartbeat_group_make_item leaks some memory on
fail paths. Fix the paths by adding a new label and jump there.

Signed-off-by: Jiri Slaby <[email protected]>
Cc: Mark Fasheh <[email protected]>
Cc: Joel Becker <[email protected]>
Cc: [email protected]
Cc: Alexander Viro <[email protected]>
Cc: [email protected]
---
fs/ocfs2/cluster/heartbeat.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
index 52c7557..9f26ac9 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -1964,8 +1964,10 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
if (reg == NULL)
return ERR_PTR(-ENOMEM);

- if (strlen(name) > O2HB_MAX_REGION_NAME_LEN)
- return ERR_PTR(-ENAMETOOLONG);
+ if (strlen(name) > O2HB_MAX_REGION_NAME_LEN) {
+ ret = -ENAMETOOLONG;
+ goto free;
+ }

spin_lock(&o2hb_live_lock);
reg->hr_region_num = 0;
@@ -1974,7 +1976,8 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
O2NM_MAX_REGIONS);
if (reg->hr_region_num >= O2NM_MAX_REGIONS) {
spin_unlock(&o2hb_live_lock);
- return ERR_PTR(-EFBIG);
+ ret = -EFBIG;
+ goto free;
}
set_bit(reg->hr_region_num, o2hb_region_bitmap);
}
@@ -1986,10 +1989,13 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
ret = o2hb_debug_region_init(reg, o2hb_debug_dir);
if (ret) {
config_item_put(&reg->hr_item);
- return ERR_PTR(ret);
+ goto free;
}

return &reg->hr_item;
+free:
+ kfree(reg);
+ return ERR_PTR(ret);
}

static void o2hb_heartbeat_group_drop_item(struct config_group *group,
--
1.7.3.1

2010-11-06 12:01:52

by Sergei Shtylyov

[permalink] [raw]
Subject: Re: [PATCH] IDE: ide-floppy, remove unnecessary NULL check

Hello.

On 06-11-2010 12:06, Jiri Slaby wrote:

> Stanse founf that rq in ide_floppy_callback cannot be NULL, because it

found?

> is dereferenced all around. So remove the superfluous check.

> This appeared after blk_* macors removal.

macros?

> Signed-off-by: Jiri Slaby<[email protected]>
> Cc: Christoph Hellwig<[email protected]>
> Cc: "David S. Miller"<[email protected]>
> Cc: [email protected]

MBR, Sergei

2010-11-06 12:10:23

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] IDE: ide-floppy, remove unnecessary NULL check

On Sat, Nov 06, 2010 at 10:06:51AM +0100, Jiri Slaby wrote:
> Stanse founf that rq in ide_floppy_callback cannot be NULL, because it
> is dereferenced all around. So remove the superfluous check.
>
> This appeared after blk_* macors removal.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: [email protected]
> ---
> drivers/ide/ide-floppy.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
> index 5406b6e..536ff68 100644
> --- a/drivers/ide/ide-floppy.c
> +++ b/drivers/ide/ide-floppy.c
> @@ -73,7 +73,7 @@ static int ide_floppy_callback(ide_drive_t *drive, int dsc)
> drive->failed_pc = NULL;
>
> if (pc->c[0] == GPCMD_READ_10 || pc->c[0] == GPCMD_WRITE_10 ||
> - (rq && rq->cmd_type == REQ_TYPE_BLOCK_PC))
> + rq->cmd_type == REQ_TYPE_BLOCK_PC)
> uptodate = 1; /* FIXME */
> else if (pc->c[0] == GPCMD_REQUEST_SENSE) {

If this isn't a fix for a real bug, then there's no need for it since
IDE is deprecated and thus in bugfix mode only.

--
Regards/Gruss,
Boris.

2010-11-07 23:28:31

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH] Char: virtio_console, fix memory leak

On Sat, 6 Nov 2010 07:36:50 pm Jiri Slaby wrote:
> Stanse found that in init_vqs, memory is leaked under certain
> circumstanses (the fail path order is incorrect). Fix that by checking
> allocations in one turn and free all of them at once if some fails
> (some may be NULL, but this is OK).

Good catch, thanks!

I generally prefer the static rather than lazy method of cleanup, but
in this case it's ugly both before and after...

Applied,
Rusty.

2010-11-08 15:35:33

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH] x86: pci/xen, fix memory leak

On Sat, Nov 06, 2010 at 10:06:49AM +0100, Jiri Slaby wrote:
> Stanse found that xen_setup_msi_irqs leaks memory when
> xen_allocate_pirq fails. Free the memory in that fail path.

Duh! Yes, sticking it in the queue for bug-fixes. Thank you
for finding it.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: [email protected]
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> ---
> arch/x86/pci/xen.c | 8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 117f5b8..d7b5109 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -147,8 +147,10 @@ static int xen_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> irq = xen_allocate_pirq(v[i], 0, /* not sharable */
> (type == PCI_CAP_ID_MSIX) ?
> "pcifront-msi-x" : "pcifront-msi");
> - if (irq < 0)
> - return -1;
> + if (irq < 0) {
> + ret = -1;
> + goto free;
> + }
>
> ret = set_irq_msi(irq, msidesc);
> if (ret)
> @@ -164,7 +166,7 @@ error:
> if (ret == -ENODEV)
> dev_err(&dev->dev, "Xen PCI frontend has not registered" \
> " MSI/MSI-X support!\n");
> -
> +free:
> kfree(v);
> return ret;
> }
> --
> 1.7.3.1
>

2010-11-18 23:43:51

by Joel Becker

[permalink] [raw]
Subject: Re: [PATCH] ocfs2: fix memory leak

On Sat, Nov 06, 2010 at 10:06:52AM +0100, Jiri Slaby wrote:
> Stanse found that o2hb_heartbeat_group_make_item leaks some memory on
> fail paths. Fix the paths by adding a new label and jump there.
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Mark Fasheh <[email protected]>
> Cc: Joel Becker <[email protected]>
> Cc: [email protected]
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]

This patch is now in the fixes branch of ocfs2.git.

Joel


--

"In a crisis, don't hide behind anything or anybody. They're going
to find you anyway."
- Paul "Bear" Bryant

Joel Becker
Senior Development Manager
Oracle
E-mail: [email protected]
Phone: (650) 506-8127

2010-11-22 19:39:22

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] IDE: ide-floppy, remove unnecessary NULL check

From: Jiri Slaby <[email protected]>
Date: Sat, 6 Nov 2010 10:06:51 +0100

> Stanse founf that rq in ide_floppy_callback cannot be NULL, because it
> is dereferenced all around. So remove the superfluous check.
>
> This appeared after blk_* macors removal.
>
> Signed-off-by: Jiri Slaby <[email protected]>

Applied with commit message typos fixed.

Thanks.