Patchwork [Bug,49151] NULL pointer dereference in pata_acpi

login
register
mail settings
Submitter bugzilla-daemon@bugzilla.kernel.org
Date Nov. 16, 2012, 8:39 a.m.
Message ID <20121116083934.B4F9711FEE3@bugzilla.kernel.org>
Download mbox | patch
Permalink /patch/199518/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

bugzilla-daemon@bugzilla.kernel.org - Nov. 16, 2012, 8:39 a.m.
https://bugzilla.kernel.org/show_bug.cgi?id=49151





--- Comment #41 from Aaron Lu <aaron.lu@intel.com>  2012-11-16 08:39:33 ---
The problem I see is:
During init time, identify command needs to be sent;
pacpi_qc_issue is invoked, and if the chipset can not set timing independently
for each drive, and the qc is not for the current drive, dma mode will be set
for the target device if ata_dma_enabled returns true;
At this time, ata_device->dma_mode is still un-initialized as 0, but
ata_dma_enabled would treat this as valid, and the ata_timing table doesn't
handle mode 0, so we get NULL. This problem only occurs when processing
identify command.

So I think we should init ata_device->dma_mode to 0xff when we are to reset the
drive, and the real value will get set in ata_set_mode afterwards. This way,
when pacpi_qc_issue is invoked, it will not attempt to set dma mode for the
device.

And for Szymon's bisect, I think the 'offending' commit actually fixed a
problem of pata_acpi(maybe a long standing problem). And due to this fix,
pata_acpi module triggered this bug.

In previous kernels(pre the 'offending' commit Szymon bisected), the
acpi_port_start function will always fail due to ap->acpi_handle is NULL, the
reason is showed in the following call sequence:

ata_host_start
  ap->ops->port_start -> pacpi_port_start -> where ap->acpi_handle is used
ata_host_register
  ata_associate_acpi -> where the ap->acpi_handle is assigned

So in previous kernels, pata_acpi module will always fail to init the
controller, effectively hiding this bug.

Please someone test the following patch, as I do not have a system to reproduce
this:

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3cc7096..e04cdc2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2560,6 +2560,7 @@  int ata_bus_probe(struct ata_port *ap)
          * bus as we may be talking too fast.
          */
         dev->pio_mode = XFER_PIO_0;
+        dev->dma_mode = 0xff;

         /* If the controller has a pio mode setup function
          * then use it to set the chipset to rights. Don't
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index e60437c..bf039b0 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2657,6 +2657,7 @@  int ata_eh_reset(struct ata_link *link, int classify,
          * bus as we may be talking too fast.
          */
         dev->pio_mode = XFER_PIO_0;
+        dev->dma_mode = 0xff;

         /* If the controller has a pio mode setup function
          * then use it to set the chipset to rights. Don't