libata crashes on incorrectly initialised ports

PATCH NAK Hannes Reinecke 2 participants 4 messages Mar 13, 2006

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Hannes Reinecke

author

2006-03-13 09:14 UTC #0 lore raw

PATCH 1/1 libata crashes on incorrectly initialised ports

Hi all,

Randy found an interesting usage of the label 'err_out' in libata-core.c:ata_device_add().

'err_out' is meant to be called to teardown existing sysfs entries. As such is it clearly wrong to call it if the sysfs registration fails.

Please apply.

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke			[email protected]
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

From: Hannes Reinecke <[email protected]>
Subject: libata crashes on incorrectly initialized ports

Randy Dunlap noted:
With the update ahci I am getting these messages (typed by me, no serial port for console), but ata2 drive is not present (!?):

ata2: could not start DMA engine

BUG: unable to handle kernel NULL pointer dereference at virtual address 00000000

plus a Call Trace like so (names only transcribed here):
class_device_del
class_device_unregister
scsi_remove_host
ata_host_remove
ata_device_add
ahci_init_one
... normal pci driver init/register functions ...

The label 'err_out' is used twice; the first usage of which is wrong as there is not host registered in sysfs which we could deregister. In fact, we haven't done anything (yet) so we might as well return here.

Signed-off-by Hannes Reinecke <[email protected]>

drivers/scsi/libata-core.c +1 -1

index ab3257a..42e5c40 100644
@@ -4578,7 +4578,7 @@ int ata_device_add(const struct ata_prob
 
 		ap = ata_host_add(ent, host_set, i);
 		if (!ap)
-			goto err_out;
+			return 0;
 
 		host_set->ports[i] = ap;
 		xfer_mode_mask =(ap->udma_mask << ATA_SHIFT_UDMA) |

Jeff Garzik

NAK

2006-03-13 09:26 UTC #1 lore raw

Quoted 49 lines · Hannes Reinecke

Hi all,
Randy found an interesting usage of the label 'err_out' in libata-core.c:ata_device_add().
'err_out' is meant to be called to teardown existing sysfs entries. As such is it clearly wrong to call it if the sysfs registration fails.
Please apply.
Cheers,
Hannes
------------------------------------------------------------------------
From: Hannes Reinecke <[email protected]>
Subject: libata crashes on incorrectly initialized ports
Randy Dunlap noted:
With the update ahci I am getting these messages (typed by me, no serial port for console), but ata2 drive is not present (!?):
ata2: could not start DMA engine
BUG: unable to handle kernel NULL pointer dereference at virtual address 00000000
plus a Call Trace like so (names only transcribed here):
class_device_del
class_device_unregister
scsi_remove_host
ata_host_remove
ata_device_add
ahci_init_one
... normal pci driver init/register functions ...
The label 'err_out' is used twice; the first usage of which is wrong as there is not host registered in sysfs which we could deregister. In fact, we haven't done anything (yet) so we might as well return here.
Signed-off-by Hannes Reinecke <[email protected]>
drivers/scsi/libata-core.c +0 -0
index ab3257a..42e5c40 100644

drivers/scsi/libata-core.c int ata_device_add(const struct ata_prob · line 4581

Quoted 5 lines

drivers/scsi/libata-core.c +1 -1

@@ -4578,7 +4578,7 @@ int ata_device_add(const struct ata_prob
 
 		ap = ata_host_add(ent, host_set, i);
 		if (!ap)
-			goto err_out;
+			return 0;

NAK: This patch adds memory leaks.

The clear intent of the error handling was to clean up host_set and the ports allocated so far. 'return 0' just leaks all that stuff, rather than performing the incorrect error handling ;-)

AFAICT, all the error handling at the err_out label is correct, save for one detail: do_unregister argument passed to ata_host_remove() should be zero for the err_out callsite we are discussing.

Jeff

Hannes Reinecke

author

2006-03-13 10:00 UTC #2 lore raw

Quoted 59 lines · Jeff Garzik

Quoted 50 lines · Hannes Reinecke
Hi all,
Randy found an interesting usage of the label 'err_out' in libata-core.c:ata_device_add().
'err_out' is meant to be called to teardown existing sysfs entries. As such is it clearly wrong to call it if the sysfs registration fails.
Please apply.
Cheers,
Hannes
------------------------------------------------------------------------
From: Hannes Reinecke <[email protected]>
Subject: libata crashes on incorrectly initialized ports
Randy Dunlap noted:
With the update ahci I am getting these messages (typed by me, no serial port for console), but ata2 drive is not present (!?):
ata2: could not start DMA engine
BUG: unable to handle kernel NULL pointer dereference at virtual
address 00000000
plus a Call Trace like so (names only transcribed here):
class_device_del
class_device_unregister
scsi_remove_host
ata_host_remove
ata_device_add
ahci_init_one
... normal pci driver init/register functions ...
The label 'err_out' is used twice; the first usage of which is wrong as there is not host registered in sysfs which we could deregister. In fact, we haven't done anything (yet) so we might as well return here.
Signed-off-by Hannes Reinecke <[email protected]>
drivers/scsi/libata-core.c +0 -0
index ab3257a..42e5c40 100644
drivers/scsi/libata-core.c int ata_device_add(const struct ata_prob · line 4581
Quoted 5 lines
drivers/scsi/libata-core.c +1 -1
@@ -4578,7 +4578,7 @@ int ata_device_add(const struct ata_prob
 
         ap = ata_host_add(ent, host_set, i);
         if (!ap)
-            goto err_out;
+            return 0;
NAK: This patch adds memory leaks.

Quoted 4 lines

The clear intent of the error handling was to clean up host_set and the ports allocated so far. 'return 0' just leaks all that stuff, rather than performing the incorrect error handling ;-)

Hmm. Okay.

Quoted 4 lines

AFAICT, all the error handling at the err_out label is correct, save for one detail: do_unregister argument passed to ata_host_remove() should be zero for the err_out callsite we are discussing.

Not quite: ata_host_remove can _not_ be called with the first argument being NULL.

This leads to another interesting question:
Do we allow for non-consecutive ports?
Ie should it be possible for host_ent->port[1] to fail, but host_ent->port[2] to be present and useable?

The current code doesn't allow for that either; but if it's disallowed we can just tear down all ports after the first failed one. Which isn't handled currently, too :-)
The current code will fail if not all ports found in host_ent->ports are useable. Which seems to be the case here.

Awaiting insight.

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke			[email protected]
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

Jeff Garzik

2006-03-13 10:09 UTC #3 lore raw

Quoted 82 lines · Hannes Reinecke

Quoted 60 lines · Jeff Garzik
Quoted 50 lines · Hannes Reinecke
Hi all,
Randy found an interesting usage of the label 'err_out' in libata-core.c:ata_device_add().
'err_out' is meant to be called to teardown existing sysfs entries. As such is it clearly wrong to call it if the sysfs registration fails.
Please apply.
Cheers,
Hannes
------------------------------------------------------------------------
From: Hannes Reinecke <[email protected]>
Subject: libata crashes on incorrectly initialized ports
Randy Dunlap noted:
With the update ahci I am getting these messages (typed by me, no serial port for console), but ata2 drive is not present (!?):
ata2: could not start DMA engine
BUG: unable to handle kernel NULL pointer dereference at virtual
address 00000000
plus a Call Trace like so (names only transcribed here):
class_device_del
class_device_unregister
scsi_remove_host
ata_host_remove
ata_device_add
ahci_init_one
... normal pci driver init/register functions ...
The label 'err_out' is used twice; the first usage of which is wrong as there is not host registered in sysfs which we could deregister. In fact, we haven't done anything (yet) so we might as well return here.
Signed-off-by Hannes Reinecke <[email protected]>
drivers/scsi/libata-core.c +0 -0
index ab3257a..42e5c40 100644
drivers/scsi/libata-core.c int ata_device_add(const struct ata_prob · line 4581
Quoted 5 lines
drivers/scsi/libata-core.c +1 -1
@@ -4578,7 +4578,7 @@ int ata_device_add(const struct ata_prob
 
        ap = ata_host_add(ent, host_set, i);
        if (!ap)
-            goto err_out;
+            return 0;
NAK: This patch adds memory leaks.
Quoted 4 lines
The clear intent of the error handling was to clean up host_set and the ports allocated so far. 'return 0' just leaks all that stuff, rather than performing the incorrect error handling ;-)
Hmm. Okay.
Quoted 4 lines
AFAICT, all the error handling at the err_out label is correct, save for one detail: do_unregister argument passed to ata_host_remove() should be zero for the err_out callsite we are discussing.
Not quite: ata_host_remove can _not_ be called with the first argument being NULL.
This leads to another interesting question:
Do we allow for non-consecutive ports?
Ie should it be possible for host_ent->port[1] to fail, but host_ent->port[2] to be present and useable?

Quoted 5 lines

The current code doesn't allow for that either; but if it's disallowed we can just tear down all ports after the first failed one. Which isn't handled currently, too :-)
The current code will fail if not all ports found in host_ent->ports are useable. Which seems to be the case here.

Currently, that's intentional. ata_host_add() failure indicates a serious error such as OOM or failure to get DMA memory, and as such everything should be torn down.

Most problems are not so serious, and fall into the category of "mark port disabled". That "not so serious" category includes everything from malfunctioning hardware to ports disabled in BIOS to ports without attached devices.

So the current code already supports port[1] failing, but port[2] being usable.

Jeff