Skip to contentSkip to conversation

libata: fix probe_ent free in ata_sas_port_alloc()

PATCH In review Tejun Heo 4 participants 9 messages Feb 17, 2007 – Feb 20, 2007

Subsystems (2)
SCSI libata
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 libata: fix probe_ent free in ata_sas_port_alloc()

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 <[email protected]>

---
James, does this fix the bug you mentioned on IRC?

drivers/ata/libata-scsi.c +0 -0
index 0009818..e5e19e3 100644
@@ -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);

#1 lore raw
Quoted 7 lines · 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 <[email protected]>

---
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

author
#2 lore raw

PATCH 1/1 libata: fix probe_ent alloc/free bugs

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 <[email protected]>
Cc James Bottomley <[email protected]>

---
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.

drivers/ata/libata-core.c +0 -0
index 2cf8251..c34e0b4 100644
@@ -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)));
drivers/ata/libata-scsi.c +0 -0
index 0009818..e5e19e3 100644
@@ -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);

#3 lore raw

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

Hello.

Quoted 9 lines · 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().

[...]

Quoted 17 lines
drivers/ata/libata-core.c +1 -5
index 2cf8251..c34e0b4 100644
@@ -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",
Quoted 1 line
               kobject_name(&(dev->kobj)));

[...]

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

MBR, Sergei

#4 lore raw

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

Quoted 7 lines · Sergei Shtylyov
Quoted 4 lines
+    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

#5 lore raw

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

Hello.

Quoted 11 lines · James Bottomley
Quoted 8 lines
Quoted 4 lines
+    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)...

Quoted 1 line

James

MBR, Sergei

#6 lore raw

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

In reply to Tejun Heo
Quoted 7 lines · 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.

Quoted 8 lines

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

Signed-off-by Tejun Heo <[email protected]>
Cc James Bottomley <[email protected]>

---
James, thanks for the diagnosis and please ack if this fixes the problem.

Yes, it fixes all the problems.

James

author
#7 lore raw

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

In reply to Tejun Heo
Quoted 12 lines · 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 <[email protected]>
Cc James Bottomley <[email protected]>
Quoted 8 lines

---
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.

Signature · 1 line
tejun
Applied
#8 lore raw

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

In reply to Tejun Heo
Quoted 16 lines · 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 <[email protected]>
Cc James Bottomley <[email protected]>

---
James, thanks for the diagnosis and please ack if this fixes the problem.

Quoted 6 lines

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