Jeff Garzik wrote: > Hannes Reinecke wrote: >> 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 <hare@xxxxxxx> >> 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 <hare@xxxxxxx> >> >> diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c >> index ab3257a..42e5c40 100644 >> --- a/drivers/scsi/libata-core.c >> +++ b/drivers/scsi/libata-core.c >> @@ -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 ;-) > Hmm. Okay. > 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 -- Dr. Hannes Reinecke hare@xxxxxxx SuSE Linux Products GmbH S390 & zSeries Maxfeldstraße 5 +49 911 74053 688 90409 Nürnberg http://www.suse.de - : send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
