2006-12-09 22:54:45

by Catalin Marinas

[permalink] [raw]
Subject: Possible memory leak in ata_piix.c

Hi,

Kmemleak found a possible memory leak in piix_init_one() in
drivers/ata/ata_piix.c. This only appeared after 2.6.19, maybe caused
by the recent patches to this area. Kmemleak cannot find any track of
the kzalloc'ed piix_host_priv structure allocated in the above
function and reports it. The allocation stack trace is below:

unreferenced object 0xde9bca60 (size 4):
[<c018d85d>] __kmalloc_track_caller
[<c0179249>] __kzalloc
[<c02cf33f>] piix_init_one
[<c023dc2d>] pci_call_probe
[<c023dc81>] __pci_device_probe
[<c023dcb9>] pci_device_probe
[<c029b5fc>] really_probe
[<c029b728>] driver_probe_device
[<c029b8c1>] __driver_attach
[<c029a879>] bus_for_each_dev
[<c029b8e9>] driver_attach
[<c029ae9c>] bus_add_driver
[<c029bd27>] driver_register
[<c023e035>] __pci_register_driver
[<c02cf4ef>] piix_init

Thanks.

--
Catalin


2006-12-11 13:26:40

by Tejun Heo

[permalink] [raw]
Subject: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

piix_init_one() allocates host private data which should be freed by
piix_host_stop(). ich_pata_ops wasn't converted to piix_host_stop()
while merging, leaking 4 bytes on driver detach. Fix it.

This was spotted using Kmemleak by Catalin Marinas.

Signed-off-by: Tejun Heo <[email protected]>
Cc: Catalin Marinas <[email protected]>
---
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index c7de0bb..dfe17e1 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -330,7 +330,7 @@ static const struct ata_port_operations ich_pata_ops = {

.port_start = ata_port_start,
.port_stop = ata_port_stop,
- .host_stop = ata_host_stop,
+ .host_stop = piix_host_stop,
};

static const struct ata_port_operations piix_sata_ops = {

2006-12-11 14:13:13

by Alan

[permalink] [raw]
Subject: Re: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

On Mon, 11 Dec 2006 22:26:25 +0900
Tejun Heo <[email protected]> wrote:

> piix_init_one() allocates host private data which should be freed by
> piix_host_stop(). ich_pata_ops wasn't converted to piix_host_stop()
> while merging, leaking 4 bytes on driver detach. Fix it.
>
> This was spotted using Kmemleak by Catalin Marinas.
>
> Signed-off-by: Tejun Heo <[email protected]>
> Cc: Catalin Marinas <[email protected]>

Acked-by: Alan Cox <[email protected]>

2006-12-13 10:41:18

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

Tejun,

On 11/12/06, Tejun Heo <[email protected]> wrote:
> piix_init_one() allocates host private data which should be freed by
> piix_host_stop(). ich_pata_ops wasn't converted to piix_host_stop()
> while merging, leaking 4 bytes on driver detach. Fix it.

I tried your patch last night but the leak is still reported. I need
to investigate further and put some printk's in the piix_host_stop
function to check whether the freeing really takes place.

What I can't follow is where the ata_port_info.private_data
(port_info[] or ppinfo[]) in piix_init_one gets transfered to
ata_host.private_data (the "host" argument) that piix_host_stop tries
to free.

--
Catalin

2006-12-14 23:42:50

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

On 13/12/06, Catalin Marinas <[email protected]> wrote:
> On 11/12/06, Tejun Heo <[email protected]> wrote:
> > piix_init_one() allocates host private data which should be freed by
> > piix_host_stop(). ich_pata_ops wasn't converted to piix_host_stop()
> > while merging, leaking 4 bytes on driver detach. Fix it.
>
> I tried your patch last night but the leak is still reported. I need
> to investigate further and put some printk's in the piix_host_stop
> function to check whether the freeing really takes place.

The piix_host_stop() isn't actually called on my machine, so this is
not the cause of the leak.

What causes the leak seem to be the error returned by
ata_pci_init_one() called from piix_init_one(). These are the related
kernel messages:

ata_piix 0000:00:1f.1: version 2.00ac7
ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 16 (level, low) -> IRQ 18
PCI: Unable to reserve I/O region #1:8@1f0 for device 0000:00:1f.1
ata_piix: probe of 0000:00:1f.1 failed with error -16

I think the call to ata_pci_init_one() should be followed by some
clean-up in case it fails. There is also another return without
clean-up in piix_init_one() after the call to piix_disable_ahci(). I
don't have time to try this now.

--
Catalin