2005-05-26 19:30:06

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH] libata: fix use-after-free during driver unload/unplug

diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -289,6 +289,8 @@ static void ahci_host_stop(struct ata_ho
{
struct ahci_host_priv *hpriv = host_set->private_data;
kfree(hpriv);
+
+ ata_host_stop(host_set);
}

static int ahci_port_start(struct ata_port *ap)
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -153,6 +153,7 @@ static struct ata_port_operations piix_p

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_operations piix_sata_ops = {
@@ -180,6 +181,7 @@ static struct ata_port_operations piix_s

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info piix_port_info[] = {
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3321,6 +3321,13 @@ void ata_port_stop (struct ata_port *ap)
dma_free_coherent(dev, ATA_PRD_TBL_SZ, ap->prd, ap->prd_dma);
}

+void ata_host_stop (struct ata_host_set *host_set)
+{
+ if (host_set->mmio_base)
+ iounmap(host_set->mmio_base);
+}
+
+
/**
* ata_host_remove - Unregister SCSI host structure with upper layers
* @ap: Port to unregister
@@ -3877,10 +3884,9 @@ void ata_pci_remove_one (struct pci_dev
}

free_irq(host_set->irq, host_set);
- if (host_set->ops->host_stop)
- host_set->ops->host_stop(host_set);
- if (host_set->mmio_base)
- iounmap(host_set->mmio_base);
+
+ if (host_set->ops->host_stop_prewalk)
+ host_set->ops->host_stop_prewalk(host_set);

for (i = 0; i < host_set->n_ports; i++) {
ap = host_set->ports[i];
@@ -3899,6 +3905,9 @@ void ata_pci_remove_one (struct pci_dev
scsi_host_put(ap->host);
}

+ if (host_set->ops->host_stop)
+ host_set->ops->host_stop(host_set);
+
kfree(host_set);

pci_release_regions(pdev);
@@ -3996,6 +4005,7 @@ EXPORT_SYMBOL_GPL(ata_chk_err);
EXPORT_SYMBOL_GPL(ata_exec_command);
EXPORT_SYMBOL_GPL(ata_port_start);
EXPORT_SYMBOL_GPL(ata_port_stop);
+EXPORT_SYMBOL_GPL(ata_host_stop);
EXPORT_SYMBOL_GPL(ata_interrupt);
EXPORT_SYMBOL_GPL(ata_qc_prep);
EXPORT_SYMBOL_GPL(ata_bmdma_setup);
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -329,6 +329,8 @@ static void nv_host_stop (struct ata_hos
host->host_desc->disable_hotplug(host_set);

kfree(host);
+
+ ata_host_stop(host_set);
}

static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -122,6 +122,7 @@ static struct ata_port_operations pdc_at
.scr_write = pdc_sata_scr_write,
.port_start = pdc_port_start,
.port_stop = pdc_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info pdc_port_info[] = {
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -111,7 +111,7 @@ static void qs_scr_write (struct ata_por
static int qs_ata_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
static irqreturn_t qs_intr (int irq, void *dev_instance, struct pt_regs *regs);
static int qs_port_start(struct ata_port *ap);
-static void qs_host_stop(struct ata_host_set *host_set);
+static void qs_host_stop_prewalk(struct ata_host_set *host_set);
static void qs_port_stop(struct ata_port *ap);
static void qs_phy_reset(struct ata_port *ap);
static void qs_qc_prep(struct ata_queued_cmd *qc);
@@ -160,7 +160,8 @@ static struct ata_port_operations qs_ata
.scr_write = qs_scr_write,
.port_start = qs_port_start,
.port_stop = qs_port_stop,
- .host_stop = qs_host_stop,
+ .host_stop = ata_host_stop,
+ .host_stop_prewalk = qs_host_stop_prewalk,
.bmdma_stop = qs_bmdma_stop,
.bmdma_status = qs_bmdma_status,
};
@@ -530,7 +531,7 @@ static void qs_port_stop(struct ata_port
ata_port_stop(ap);
}

-static void qs_host_stop(struct ata_host_set *host_set)
+static void qs_host_stop_prewalk(struct ata_host_set *host_set)
{
void __iomem *mmio_base = host_set->mmio_base;

diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -161,6 +161,7 @@ static struct ata_port_operations sil_op
.scr_write = sil_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info sil_port_info[] = {
diff --git a/drivers/scsi/sata_sis.c b/drivers/scsi/sata_sis.c
--- a/drivers/scsi/sata_sis.c
+++ b/drivers/scsi/sata_sis.c
@@ -114,6 +114,7 @@ static struct ata_port_operations sis_op
.scr_write = sis_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info sis_port_info = {
diff --git a/drivers/scsi/sata_svw.c b/drivers/scsi/sata_svw.c
--- a/drivers/scsi/sata_svw.c
+++ b/drivers/scsi/sata_svw.c
@@ -313,6 +313,7 @@ static struct ata_port_operations k2_sat
.scr_write = k2_sata_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static void k2_sata_setup_port(struct ata_ioports *port, unsigned long base)
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -245,6 +245,8 @@ static void pdc20621_host_stop(struct at

iounmap(dimm_mmio);
kfree(hpriv);
+
+ ata_host_stop(host_set);
}

static int pdc_port_start(struct ata_port *ap)
diff --git a/drivers/scsi/sata_uli.c b/drivers/scsi/sata_uli.c
--- a/drivers/scsi/sata_uli.c
+++ b/drivers/scsi/sata_uli.c
@@ -113,6 +113,7 @@ static struct ata_port_operations uli_op

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info uli_port_info = {
diff --git a/drivers/scsi/sata_via.c b/drivers/scsi/sata_via.c
--- a/drivers/scsi/sata_via.c
+++ b/drivers/scsi/sata_via.c
@@ -134,6 +134,7 @@ static struct ata_port_operations svia_s

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info svia_port_info = {
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -230,6 +230,7 @@ static struct ata_port_operations vsc_sa
.scr_write = vsc_sata_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static void __devinit vsc_sata_setup_port(struct ata_ioports *port, unsigned long base)
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -361,6 +361,7 @@ struct ata_port_operations {
int (*port_start) (struct ata_port *ap);
void (*port_stop) (struct ata_port *ap);

+ void (*host_stop_prewalk) (struct ata_host_set *host_set);
void (*host_stop) (struct ata_host_set *host_set);

void (*bmdma_stop) (struct ata_port *ap);
@@ -410,6 +411,7 @@ extern u8 ata_chk_err(struct ata_port *a
extern void ata_exec_command(struct ata_port *ap, struct ata_taskfile *tf);
extern int ata_port_start (struct ata_port *ap);
extern void ata_port_stop (struct ata_port *ap);
+extern void ata_host_stop (struct ata_host_set *host_set);
extern irqreturn_t ata_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
extern void ata_qc_prep(struct ata_queued_cmd *qc);
extern int ata_qc_issue_prot(struct ata_queued_cmd *qc);


Attachments:
patch (7.56 kB)

2005-05-26 21:33:50

by Mark Lord

[permalink] [raw]
Subject: Re: [PATCH] libata: fix use-after-free during driver unload/unplug

Hi Jeff!

Jeff Garzik wrote:
>
> * add ->host_stop_prewalk() hook, use it in sata_qstor.c (hi Mark).
> sata_qstor appears to require the host-stop-before-port-stop ordering
> that existed prior to applying the attached patch.

Mmm.. I'm a little bit rusty here, but I don't think qstor
cares about the order, so long as ports are marked with
ATA_FLAG_PORT_DISABLED before invoking port_stop().

I've tried to allow disabling/enabling individual ports
on-the-fly as needed, even though it never really happens
in practice. So host_stop() kills the whole chip, whereas
port_stop() I took to mean just one of the four SATA ports.

Cheers
--
Mark Lord
Real-Time Remedies Inc.
[email protected]

2005-05-27 01:27:05

by Jeff Garzik

[permalink] [raw]
Subject: Re: [PATCH] libata: fix use-after-free during driver unload/unplug

Mark Lord wrote:
> Hi Jeff!
>
> Jeff Garzik wrote:
>
>>
>> * add ->host_stop_prewalk() hook, use it in sata_qstor.c (hi Mark).
>> sata_qstor appears to require the host-stop-before-port-stop ordering
>> that existed prior to applying the attached patch.
>
>
> Mmm.. I'm a little bit rusty here, but I don't think qstor
> cares about the order, so long as ports are marked with
> ATA_FLAG_PORT_DISABLED before invoking port_stop().
>
> I've tried to allow disabling/enabling individual ports
> on-the-fly as needed, even though it never really happens
> in practice. So host_stop() kills the whole chip, whereas
> port_stop() I took to mean just one of the four SATA ports.

qstor's ->host_stop() disables global interrupts, and I didn't know if
you really wanted to do that prior to ->port_stop().

I would much prefer to eliminate the ->host_stop_prewalk() hook, and
simply call ->host_stop after all ->port_stop() calls complete, if qstor
doesn't need the pre-walk.

Jeff



2005-05-27 01:39:04

by Mark Lord

[permalink] [raw]
Subject: Re: [PATCH] libata: fix use-after-free during driver unload/unplug

Jeff Garzik wrote:
>
> qstor's ->host_stop() disables global interrupts, and I didn't know if
> you really wanted to do that prior to ->port_stop().
>
> I would much prefer to eliminate the ->host_stop_prewalk() hook, and
> simply call ->host_stop after all ->port_stop() calls complete, if qstor
> doesn't need the pre-walk.

Ahh.. of course. I must have misread your original.

Yes, sata_qstor generally assumes that host_stop should completely
stop the host from operating.. and thereby expects any port_stop()
calls to happen before host_stop(). So, yup, the changes you have
seem to be useful here.

If we didn't want to have a prewalk() method, then we could hack
sata_qstor to keep a count of active ("non stopped") ports,
and automatically do the global (chip) interrupt disable from
port_stop() whenever the count hits zero.

Cheers

2005-05-27 02:00:54

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH][RFT] libata: fix use-after-free during driver unload/unplug

Attached is the patch I committed, please test.

2005-05-27 04:18:55

by Jeff Garzik

[permalink] [raw]
Subject: [PATCH][RFT] libata: fix use-after-free during driver unload/unplug

diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
--- a/drivers/scsi/ahci.c
+++ b/drivers/scsi/ahci.c
@@ -289,6 +289,8 @@ static void ahci_host_stop(struct ata_ho
{
struct ahci_host_priv *hpriv = host_set->private_data;
kfree(hpriv);
+
+ ata_host_stop(host_set);
}

static int ahci_port_start(struct ata_port *ap)
diff --git a/drivers/scsi/ata_piix.c b/drivers/scsi/ata_piix.c
--- a/drivers/scsi/ata_piix.c
+++ b/drivers/scsi/ata_piix.c
@@ -153,6 +153,7 @@ static struct ata_port_operations piix_p

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_operations piix_sata_ops = {
@@ -180,6 +181,7 @@ static struct ata_port_operations piix_s

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info piix_port_info[] = {
diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -3321,6 +3321,13 @@ void ata_port_stop (struct ata_port *ap)
dma_free_coherent(dev, ATA_PRD_TBL_SZ, ap->prd, ap->prd_dma);
}

+void ata_host_stop (struct ata_host_set *host_set)
+{
+ if (host_set->mmio_base)
+ iounmap(host_set->mmio_base);
+}
+
+
/**
* ata_host_remove - Unregister SCSI host structure with upper layers
* @ap: Port to unregister
@@ -3877,10 +3884,6 @@ void ata_pci_remove_one (struct pci_dev
}

free_irq(host_set->irq, host_set);
- if (host_set->ops->host_stop)
- host_set->ops->host_stop(host_set);
- if (host_set->mmio_base)
- iounmap(host_set->mmio_base);

for (i = 0; i < host_set->n_ports; i++) {
ap = host_set->ports[i];
@@ -3899,6 +3902,9 @@ void ata_pci_remove_one (struct pci_dev
scsi_host_put(ap->host);
}

+ if (host_set->ops->host_stop)
+ host_set->ops->host_stop(host_set);
+
kfree(host_set);

pci_release_regions(pdev);
@@ -3996,6 +4002,7 @@ EXPORT_SYMBOL_GPL(ata_chk_err);
EXPORT_SYMBOL_GPL(ata_exec_command);
EXPORT_SYMBOL_GPL(ata_port_start);
EXPORT_SYMBOL_GPL(ata_port_stop);
+EXPORT_SYMBOL_GPL(ata_host_stop);
EXPORT_SYMBOL_GPL(ata_interrupt);
EXPORT_SYMBOL_GPL(ata_qc_prep);
EXPORT_SYMBOL_GPL(ata_bmdma_setup);
diff --git a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
--- a/drivers/scsi/sata_nv.c
+++ b/drivers/scsi/sata_nv.c
@@ -329,6 +329,8 @@ static void nv_host_stop (struct ata_hos
host->host_desc->disable_hotplug(host_set);

kfree(host);
+
+ ata_host_stop(host_set);
}

static int nv_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
diff --git a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
--- a/drivers/scsi/sata_promise.c
+++ b/drivers/scsi/sata_promise.c
@@ -122,6 +122,7 @@ static struct ata_port_operations pdc_at
.scr_write = pdc_sata_scr_write,
.port_start = pdc_port_start,
.port_stop = pdc_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info pdc_port_info[] = {
diff --git a/drivers/scsi/sata_qstor.c b/drivers/scsi/sata_qstor.c
--- a/drivers/scsi/sata_qstor.c
+++ b/drivers/scsi/sata_qstor.c
@@ -536,6 +536,8 @@ static void qs_host_stop(struct ata_host

writeb(0, mmio_base + QS_HCT_CTRL); /* disable host interrupts */
writeb(QS_CNFG3_GSRST, mmio_base + QS_HCF_CNFG3); /* global reset */
+
+ ata_host_stop(host_set);
}

static void qs_host_init(unsigned int chip_id, struct ata_probe_ent *pe)
diff --git a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c
+++ b/drivers/scsi/sata_sil.c
@@ -161,6 +161,7 @@ static struct ata_port_operations sil_op
.scr_write = sil_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info sil_port_info[] = {
diff --git a/drivers/scsi/sata_sis.c b/drivers/scsi/sata_sis.c
--- a/drivers/scsi/sata_sis.c
+++ b/drivers/scsi/sata_sis.c
@@ -114,6 +114,7 @@ static struct ata_port_operations sis_op
.scr_write = sis_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info sis_port_info = {
diff --git a/drivers/scsi/sata_svw.c b/drivers/scsi/sata_svw.c
--- a/drivers/scsi/sata_svw.c
+++ b/drivers/scsi/sata_svw.c
@@ -313,6 +313,7 @@ static struct ata_port_operations k2_sat
.scr_write = k2_sata_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static void k2_sata_setup_port(struct ata_ioports *port, unsigned long base)
diff --git a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
--- a/drivers/scsi/sata_sx4.c
+++ b/drivers/scsi/sata_sx4.c
@@ -245,6 +245,8 @@ static void pdc20621_host_stop(struct at

iounmap(dimm_mmio);
kfree(hpriv);
+
+ ata_host_stop(host_set);
}

static int pdc_port_start(struct ata_port *ap)
diff --git a/drivers/scsi/sata_uli.c b/drivers/scsi/sata_uli.c
--- a/drivers/scsi/sata_uli.c
+++ b/drivers/scsi/sata_uli.c
@@ -113,6 +113,7 @@ static struct ata_port_operations uli_op

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info uli_port_info = {
diff --git a/drivers/scsi/sata_via.c b/drivers/scsi/sata_via.c
--- a/drivers/scsi/sata_via.c
+++ b/drivers/scsi/sata_via.c
@@ -134,6 +134,7 @@ static struct ata_port_operations svia_s

.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static struct ata_port_info svia_port_info = {
diff --git a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c
+++ b/drivers/scsi/sata_vsc.c
@@ -230,6 +230,7 @@ static struct ata_port_operations vsc_sa
.scr_write = vsc_sata_scr_write,
.port_start = ata_port_start,
.port_stop = ata_port_stop,
+ .host_stop = ata_host_stop,
};

static void __devinit vsc_sata_setup_port(struct ata_ioports *port, unsigned long base)
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -410,6 +410,7 @@ extern u8 ata_chk_err(struct ata_port *a
extern void ata_exec_command(struct ata_port *ap, struct ata_taskfile *tf);
extern int ata_port_start (struct ata_port *ap);
extern void ata_port_stop (struct ata_port *ap);
+extern void ata_host_stop (struct ata_host_set *host_set);
extern irqreturn_t ata_interrupt (int irq, void *dev_instance, struct pt_regs *regs);
extern void ata_qc_prep(struct ata_queued_cmd *qc);
extern int ata_qc_issue_prot(struct ata_queued_cmd *qc);


Attachments:
patch (6.37 kB)