Patchwork libata: fix DMA to stack in reading devslp_timing parameters

login
register
mail settings
Submitter David Woodhouse
Date March 29, 2013, 11:54 a.m.
Message ID <1364558095.14860.59.camel@i7.infradead.org>
Download mbox | patch
Permalink /patch/232375/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

David Woodhouse - March 29, 2013, 11:54 a.m.
Commit 803739d25c2343da6d2f95eebdcbc08bf67097d4 ("[libata] replace
sata_settings with devslp_timing"), which was also Cc: stable, used a
stack buffer to receive data from ata_read_log_page(), which triggers
the following warning:
 ahci 0000:00:1f.2: DMA-API: device driver maps memory fromstack [addr=ffff880140469948]

Fix this by using ap->sector_buf instead of a stack buffer.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Cc: stable@kernel.org

---
I'm somewhat ignorant, but it looks like this is kind of thing is what
ap->sector_buf is *for*, right? And in ata_dev_configure() nobody else
is likely to be using it?
Shane Huang - April 1, 2013, 9:51 a.m.
Hi David,

> Fix this by using ap->sector_buf instead of a stack buffer.

> 

> ---

> I'm somewhat ignorant, but it looks like this is kind of thing is what

> ap->sector_buf is *for*, right? And in ata_dev_configure() nobody else

> is likely to be using it?


Your fix is fine with me, thanks. I didn't notice this warning since
CONFIG_DMA_API_DEBUG was not set here.

Regards,
Shane
David Woodhouse - April 3, 2013, 5:02 p.m.
On Fri, 2013-03-29 at 11:54 +0000, David Woodhouse wrote:
> Commit 803739d25c2343da6d2f95eebdcbc08bf67097d4 ("[libata] replace
> sata_settings with devslp_timing"), which was also Cc: stable, used a
> stack buffer to receive data from ata_read_log_page()...

That commit also changes from using ata_id_has_ncq() to
ata_id_has_devslp() as the condition for reading the SATA settings log
page (log 0x30 page 0x08), because of compatibility issues with some
drives which *do* implement NCQ but not DEVSLP.

It's not entirely clear to me that this change was correct. Even if the
drive does support DEVSLP, the timings in the SATA settings log page are
optional, and we're supposed to use default values if they are not
provided. I can imagine some DEVSLP-capable drives not having that page
either.

Shouldn't we be reading page zero of the 0x30 log, which *is* mandatory,
and looking there to see which pages are supported?
Jeff Garzik - April 3, 2013, 11:46 p.m.
On 03/29/2013 07:54 AM, David Woodhouse wrote:
> Commit 803739d25c2343da6d2f95eebdcbc08bf67097d4 ("[libata] replace
> sata_settings with devslp_timing"), which was also Cc: stable, used a
> stack buffer to receive data from ata_read_log_page(), which triggers
> the following warning:
>   ahci 0000:00:1f.2: DMA-API: device driver maps memory fromstack [addr=ffff880140469948]
>
> Fix this by using ap->sector_buf instead of a stack buffer.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Cc: stable@kernel.org

applied


> I'm somewhat ignorant, but it looks like this is kind of thing is what
> ap->sector_buf is *for*, right? And in ata_dev_configure() nobody else
> is likely to be using it?

yep



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 497adea..b1160ee 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2329,7 +2329,7 @@  int ata_dev_configure(struct ata_device *dev)
 		 * from SATA Settings page of Identify Device Data Log.
 		 */
 		if (ata_id_has_devslp(dev->id)) {
-			u8 sata_setting[ATA_SECT_SIZE];
+			u8 *sata_setting = ap->sector_buf;
 			int i, j;
 
 			dev->flags |= ATA_DFLAG_DEVSLP;