[PATCH] libata: fix probe_ent free in ata_sas_port_alloc()

[PATCH] libata: fix probe_ent free in ata_sas_port_alloc()

[Tejun Heo]

probe_ent is allocated using devm_kzalloc() and thus should be freed
using devm_kfree().  ata_sas_port_alloc() freed its probe_ent using
kfree() thus causing double free later.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
James, does this fix the bug you mentioned on IRC?

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0009818..e5e19e3 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3234,7 +3234,7 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
 
 	ata_port_init(ap, host, ent, 0);
 	ap->lock = shost->host_lock;
-	kfree(ent);
+	devm_kfree(host->dev, ent);
 	return ap;
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_alloc);

Re: [PATCH] libata: fix probe_ent free in ata_sas_port_alloc()

[James Bottomley]

On Sat, 2007-02-17 at 23:27 +0900, Tejun Heo wrote:
> probe_ent is allocated using devm_kzalloc() and thus should be freed
> using devm_kfree().  ata_sas_port_alloc() freed its probe_ent using
> kfree() thus causing double free later.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> James, does this fix the bug you mentioned on IRC?

Yes and no.  I actually have two devices in this sas setup: a SATA disk
and a SATAPI DVD burner.  Originally, I got the bug I reported here

                           Subject: 
BUG in libata from
ata_sas_port_alloc

On my SATA disk.  However, the DVD was fine.  Now the disk shows up
fine, but I get this from the DVD:

BUG: at drivers/base/devres.c:642 devm_kfree()
 [<c0103c0a>] show_trace_log_lvl+0x1a/0x30
 [<c0104282>] show_trace+0x12/0x20
 [<c0104336>] dump_stack+0x16/0x20
 [<c023f09a>] devm_kfree+0x4a/0x50
 [<f892eea2>] ata_sas_port_alloc+0x62/0x80 [libata]
 [<f897869e>] sas_ata_init_host_and_port+0x5e/0xa0 [libsas]
 [<f897832d>] sas_target_alloc+0x4d/0x60 [libsas]
[...]

This time, it's the opposite problem: the SATAPI DVD was kmalloc
allocated.  The fault all seems to be in this code:

struct ata_probe_ent *
ata_probe_ent_alloc(struct device *dev, const struct ata_port_info *port)
{
	struct ata_probe_ent *probe_ent;

	/* XXX - the following if can go away once all LLDs are managed */
	if (!list_empty(&dev->devres_head))
		probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
	else
		probe_ent = kzalloc(sizeof(*probe_ent), GFP_KERNEL);

So we can't tell how the memory was obtained.

To fix it, it looks like we might have to mark it in some way and then
call a freeing function (ata_probe_ent_free?) to release it via the
correct method.

James

[PATCH] libata: fix probe_ent alloc/free bugs

[Tejun Heo]

ata_probe_ent_alloc() had a temporary hack such that devm_kzalloc()
was used for allocation if devres had been previously initialized on
the device; otherwise, plain kzalloc() was used.  This was to make the
code useable from both the old and devres-aware libata drivers during
transition.  This hack made ata_sas_port_alloc() unable to determine
how the probe_ent is allocated, causing double free in some cases.

Remove the now-unneeded hack and make ata_sas_port_alloc() use
devm_kfree().

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: James Bottomley <James.Bottomley@SteelEye.com>
---
James, thanks for the diagnosis and please ack if this fixes the
problem.

Jeff, after James' ack, can you please verify this works with a libata
driver?  Just loading and unloading a libata LLD should be enough.
I'm visiting my hometown for lunar new year's day, so I can't do it
till Tuesday.

Thanks.

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2cf8251..c34e0b4 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5891,11 +5891,7 @@ ata_probe_ent_alloc(struct device *dev, const struct ata_port_info *port)
 {
 	struct ata_probe_ent *probe_ent;
 
-	/* XXX - the following if can go away once all LLDs are managed */
-	if (!list_empty(&dev->devres_head))
-		probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
-	else
-		probe_ent = kzalloc(sizeof(*probe_ent), GFP_KERNEL);
+	probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
 	if (!probe_ent) {
 		printk(KERN_ERR DRV_NAME "(%s): out of memory\n",
 		       kobject_name(&(dev->kobj)));
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0009818..e5e19e3 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3234,7 +3234,7 @@ struct ata_port *ata_sas_port_alloc(struct ata_host *host,
 
 	ata_port_init(ap, host, ent, 0);
 	ap->lock = shost->host_lock;
-	kfree(ent);
+	devm_kfree(host->dev, ent);
 	return ap;
 }
 EXPORT_SYMBOL_GPL(ata_sas_port_alloc);

Re: [PATCH] libata: fix probe_ent alloc/free bugs

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:
> ata_probe_ent_alloc() had a temporary hack such that devm_kzalloc()
> was used for allocation if devres had been previously initialized on
> the device; otherwise, plain kzalloc() was used.  This was to make the
> code useable from both the old and devres-aware libata drivers during
> transition.  This hack made ata_sas_port_alloc() unable to determine
> how the probe_ent is allocated, causing double free in some cases.
> 
> Remove the now-unneeded hack and make ata_sas_port_alloc() use
> devm_kfree().

[...]

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2cf8251..c34e0b4 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5891,11 +5891,7 @@ ata_probe_ent_alloc(struct device *dev, const 
> struct ata_port_info *port)
> {
>     struct ata_probe_ent *probe_ent;
> 
> -    /* XXX - the following if can go away once all LLDs are managed */
> -    if (!list_empty(&dev->devres_head))
> -        probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
> -    else
> -        probe_ent = kzalloc(sizeof(*probe_ent), GFP_KERNEL);
> +    probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
>     if (!probe_ent) {
>         printk(KERN_ERR DRV_NAME "(%s): out of memory\n",
>                kobject_name(&(dev->kobj)));
[...]

    The patch certainly looks mangled tab-wise. :-)

MBR, Sergei

Re: [PATCH] libata: fix probe_ent alloc/free bugs

[James Bottomley]

On Sun, 2007-02-18 at 02:24 +0900, Tejun Heo wrote:
> ata_probe_ent_alloc() had a temporary hack such that devm_kzalloc()
> was used for allocation if devres had been previously initialized on
> the device; otherwise, plain kzalloc() was used.  This was to make the
> code useable from both the old and devres-aware libata drivers during
> transition.  This hack made ata_sas_port_alloc() unable to determine
> how the probe_ent is allocated, causing double free in some cases.
> 
> Remove the now-unneeded hack and make ata_sas_port_alloc() use
> devm_kfree().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: James Bottomley <James.Bottomley@SteelEye.com>
> ---
> James, thanks for the diagnosis and please ack if this fixes the
> problem.

Yes, it fixes all the problems.

James

Re: [PATCH] libata: fix probe_ent alloc/free bugs

[James Bottomley]

On Sat, 2007-02-17 at 20:43 +0300, Sergei Shtylyov wrote:
> > +    probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
> >     if (!probe_ent) {
> >         printk(KERN_ERR DRV_NAME "(%s): out of memory\n",
> >                kobject_name(&(dev->kobj)));
> [...]
> 
>     The patch certainly looks mangled tab-wise. :-)

It isn't, though, so I think you'll find it's a problem with your mail
client.

James

Re: [PATCH] libata: fix probe_ent alloc/free bugs

[Sergei Shtylyov]

Hello.

James Bottomley wrote:

>>>+    probe_ent = devm_kzalloc(dev, sizeof(*probe_ent), GFP_KERNEL);
>>>    if (!probe_ent) {
>>>        printk(KERN_ERR DRV_NAME "(%s): out of memory\n",
>>>               kobject_name(&(dev->kobj)));

>>[...]

>>    The patch certainly looks mangled tab-wise. :-)

> It isn't, though, so I think you'll find it's a problem with your mail
> client.

    You're right. :-<
    It seems to "dislike" format=flowed in this message (that's what is 
different from other mails where tabs are shown as is)...

> James

MBR, Sergei

Re: [PATCH] libata: fix probe_ent alloc/free bugs

[Tejun Heo]

Tejun Heo wrote:
> ata_probe_ent_alloc() had a temporary hack such that devm_kzalloc()
> was used for allocation if devres had been previously initialized on
> the device; otherwise, plain kzalloc() was used.  This was to make the
> code useable from both the old and devres-aware libata drivers during
> transition.  This hack made ata_sas_port_alloc() unable to determine
> how the probe_ent is allocated, causing double free in some cases.
> 
> Remove the now-unneeded hack and make ata_sas_port_alloc() use
> devm_kfree().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: James Bottomley <James.Bottomley@SteelEye.com>
> ---
> James, thanks for the diagnosis and please ack if this fixes the
> problem.
> 
> Jeff, after James' ack, can you please verify this works with a libata
> driver?  Just loading and unloading a libata LLD should be enough.
> I'm visiting my hometown for lunar new year's day, so I can't do it
> till Tuesday.

Okay, verified.  Please apply.

-- 
tejun

Re: [PATCH] libata: fix probe_ent alloc/free bugs

[Jeff Garzik]

Tejun Heo wrote:
> ata_probe_ent_alloc() had a temporary hack such that devm_kzalloc()
> was used for allocation if devres had been previously initialized on
> the device; otherwise, plain kzalloc() was used.  This was to make the
> code useable from both the old and devres-aware libata drivers during
> transition.  This hack made ata_sas_port_alloc() unable to determine
> how the probe_ent is allocated, causing double free in some cases.
> 
> Remove the now-unneeded hack and make ata_sas_port_alloc() use
> devm_kfree().
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> Cc: James Bottomley <James.Bottomley@SteelEye.com>
> ---
> James, thanks for the diagnosis and please ack if this fixes the
> problem.
> 
> Jeff, after James' ack, can you please verify this works with a libata
> driver?  Just loading and unloading a libata LLD should be enough.
> I'm visiting my hometown for lunar new year's day, so I can't do it
> till Tuesday.
> 
> Thanks.

applied