Possible memory leak in ata_piix.c

Possible memory leak in ata_piix.c

[Catalin Marinas]

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

[PATCH] ata_piix: use piix_host_stop() in ich_pata_ops

[Tejun Heo]

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 <htejun@gmail.com>
Cc: Catalin Marinas <catalin.marinas@gmail.com>
---
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 = {

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

[Alan]

On Mon, 11 Dec 2006 22:26:25 +0900
Tejun Heo <htejun@gmail.com> 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 <htejun@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@gmail.com>

Acked-by: Alan Cox <alan@redhat.com>

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

[Catalin Marinas]

Tejun,

On 11/12/06, Tejun Heo <htejun@gmail.com> 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

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

[Catalin Marinas]

On 13/12/06, Catalin Marinas <catalin.marinas@gmail.com> wrote:
> On 11/12/06, Tejun Heo <htejun@gmail.com> 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