[PATCH v2 -mm 0/6] revert the commit 22a9189f (cdrom: use kmalloced buffers instead of buffers on stack)

[PATCH v2 -mm 0/6] revert the commit 22a9189f (cdrom: use kmalloced buffers instead of buffers on stack)

[FUJITA Tomonori]

This is an updated version of the patchset to revert the commit
22a9189fd073db3d03a4cf8b8c098aa207602de1 (cdrom: use kmalloced buffers
instead of buffers on stack):

http://marc.info/?l=linux-scsi&m=121125981930673&w=2

The major change is that this version avoids DMA on the stack on all
the platforms (for simplicity) while the previous version uses the
dma_pad_mask on a non-coherent platform (in a bit hacky way).

With thic patchset, blk_rq_map_kern() always uses the bounce buffers
if a caller passes a stack buffer.

ide-cd has a mechanism to handle DMA alignment and padding for
SG_IO. This patchset extends the mechanism with regard to a stack
buffer.

This patchset is against -mm, replaces the following patches in -mm:

cdrom-revert-commit-22a9189-cdrom-use-kmalloced-buffers-instead-of-buffers-on-stack.patch
ide-use-the-dma-safe-check-for-req_type_ata_pc.patch 
block-add-blk_queue_update_dma_pad.patch 
block-use-arch_kmalloc_minalign-as-the-default-dma-pad-mask.patch

The diffstat is:

 block/blk-map.c           |    5 +
 block/blk-settings.c      |   24 ++++-
 drivers/ata/libata-scsi.c |    3 +-
 drivers/cdrom/cdrom.c     |  274 +++++++++++++++------------------------------
 drivers/ide/ide-cd.c      |   22 +++-
 drivers/scsi/sr.c         |   20 +---
 include/linux/blkdev.h    |    1 +
 7 files changed, 143 insertions(+), 206 deletions(-)

[PATCH v2 -mm 1/6] block: add blk_queue_update_dma_pad

[FUJITA Tomonori]

This adds blk_queue_update_dma_pad to prevent LLDs from overwriting
the dma pad mask wrongly (we added blk_queue_update_dma_alignment due
to the same reason).

This also converts libata to use blk_queue_update_dma_pad instead of
blk_queue_dma_pad.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Tejun Heo <htejun@gmail.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-settings.c      |   24 ++++++++++++++++++++----
 drivers/ata/libata-scsi.c |    3 ++-
 include/linux/blkdev.h    |    1 +
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 8dd8641..dfc7701 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -302,11 +302,10 @@ EXPORT_SYMBOL(blk_queue_stack_limits);
  * @q:     the request queue for the device
  * @mask:  pad mask
  *
- * Set pad mask.  Direct IO requests are padded to the mask specified.
+ * Set dma pad mask.
  *
- * Appending pad buffer to a request modifies ->data_len such that it
- * includes the pad buffer.  The original requested data length can be
- * obtained using blk_rq_raw_data_len().
+ * Appending pad buffer to a request modifies the last entry of a
+ * scatter list such that it includes the pad buffer.
  **/
 void blk_queue_dma_pad(struct request_queue *q, unsigned int mask)
 {
@@ -315,6 +314,23 @@ void blk_queue_dma_pad(struct request_queue *q, unsigned int mask)
 EXPORT_SYMBOL(blk_queue_dma_pad);
 
 /**
+ * blk_queue_update_dma_pad - update pad mask
+ * @q:     the request queue for the device
+ * @mask:  pad mask
+ *
+ * Update dma pad mask.
+ *
+ * Appending pad buffer to a request modifies the last entry of a
+ * scatter list such that it includes the pad buffer.
+ **/
+void blk_queue_update_dma_pad(struct request_queue *q, unsigned int mask)
+{
+	if (mask > q->dma_pad_mask)
+		q->dma_pad_mask = mask;
+}
+EXPORT_SYMBOL(blk_queue_update_dma_pad);
+
+/**
  * blk_queue_dma_drain - Set up a drain buffer for excess dma.
  * @q:  the request queue for the device
  * @dma_drain_needed: fn which returns non-zero if drain is necessary
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index aeb6e01..60c0e28 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -885,7 +885,8 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		/* set the min alignment and padding */
 		blk_queue_update_dma_alignment(sdev->request_queue,
 					       ATA_DMA_PAD_SZ - 1);
-		blk_queue_dma_pad(sdev->request_queue, ATA_DMA_PAD_SZ - 1);
+		blk_queue_update_dma_pad(sdev->request_queue,
+					 ATA_DMA_PAD_SZ - 1);
 
 		/* configure draining */
 		buf = kmalloc(ATAPI_MAX_DRAIN, q->bounce_gfp | GFP_KERNEL);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1171abd..5d8c339 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -748,6 +748,7 @@ extern void blk_queue_max_segment_size(struct request_queue *, unsigned int);
 extern void blk_queue_hardsect_size(struct request_queue *, unsigned short);
 extern void blk_queue_stack_limits(struct request_queue *t, struct request_queue *b);
 extern void blk_queue_dma_pad(struct request_queue *, unsigned int);
+extern void blk_queue_update_dma_pad(struct request_queue *, unsigned int);
 extern int blk_queue_dma_drain(struct request_queue *q,
 			       dma_drain_needed_fn *dma_drain_needed,
 			       void *buf, unsigned int size);
-- 
1.5.4.2

[PATCH v2 -mm 2/6] ide: use the dma safe check for REQ_TYPE_ATA_PC

[FUJITA Tomonori]

This uses the dma safe check for REQ_TYPE_ATA_PC. The dma safe check
is used for only sg requests but it should be used for other non fs
commands.

This uses blk_queue_update_dma_pad to make the intention clear though
ide don't use the blk APIs so it doesn't change anything.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Acked-by: Borislav Petkov <petkovbb@gmail.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index d998471..e3f085c 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1191,10 +1191,15 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 	info->dma = 0;
 
 	/* sg request */
-	if (rq->bio) {
-		int mask = drive->queue->dma_alignment;
-		unsigned long addr =
-			(unsigned long)page_address(bio_page(rq->bio));
+	if (rq->bio || ((rq->cmd_type == REQ_TYPE_ATA_PC) && rq->data_len)) {
+		struct request_queue *q = drive->queue;
+		unsigned int alignment;
+		unsigned long addr;
+
+		if (rq->bio)
+			addr = (unsigned long)bio_data(rq->bio);
+		else
+			addr = (unsigned long)rq->data;
 
 		info->dma = drive->using_dma;
 
@@ -1204,7 +1209,8 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 		 * NOTE! The "len" and "addr" checks should possibly have
 		 * separate masks.
 		 */
-		if ((rq->data_len & 15) || (addr & mask))
+		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
+		if (addr & alignment || rq->data_len & alignment)
 			info->dma = 0;
 	}
 
@@ -1872,6 +1878,7 @@ static int ide_cdrom_setup(ide_drive_t *drive)
 
 	blk_queue_prep_rq(drive->queue, ide_cdrom_prep_fn);
 	blk_queue_dma_alignment(drive->queue, 31);
+	blk_queue_update_dma_pad(drive->queue, 15);
 	drive->queue->unplug_delay = (1 * HZ) / 1000;
 	if (!drive->queue->unplug_delay)
 		drive->queue->unplug_delay = 1;
-- 
1.5.4.2

[PATCH v2 -mm 3/6] block: blk_rq_map_kern uses the bounce buffers for stack buffers

[FUJITA Tomonori]

blk_rq_map_kern is used for kernel internal I/Os. Some callers use
this function with stack buffers but DMA to/from the stack buffers
leads to memory corruption on a non-coherent platform.

This patch make blk_rq_map_kern uses the bounce buffers if a caller
passes a stack buffer (on the all platforms for simplicity).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-map.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 0b1af5a..6b0142b 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -268,6 +268,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	int reading = rq_data_dir(rq) == READ;
 	int do_copy = 0;
 	struct bio *bio;
+	unsigned long stack_mask = ~(THREAD_SIZE - 1);
 
 	if (len > (q->max_hw_sectors << 9))
 		return -EINVAL;
@@ -278,6 +279,10 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	alignment = queue_dma_alignment(q) | q->dma_pad_mask;
 	do_copy = ((kaddr & alignment) || (len & alignment));
 
+	if (!((kaddr & stack_mask) ^
+	      ((unsigned long)current->stack & stack_mask)))
+		do_copy = 1;
+
 	if (do_copy)
 		bio = bio_copy_kern(q, kbuf, len, gfp_mask, reading);
 	else
-- 
1.5.4.2

[PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[FUJITA Tomonori]

Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
leads to memory corruption on a non-coherent platform.

With regard to alignment and padding, ide-cd has the the dma safe
check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
check to that check.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Borislav Petkov <petkovbb@gmail.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-cd.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index e3f085c..e12d602 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 		struct request_queue *q = drive->queue;
 		unsigned int alignment;
 		unsigned long addr;
+		unsigned long stack_mask = ~(THREAD_SIZE - 1);
 
 		if (rq->bio)
 			addr = (unsigned long)bio_data(rq->bio);
@@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
 		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
 		if (addr & alignment || rq->data_len & alignment)
 			info->dma = 0;
+
+		if (!((addr & stack_mask) ^
+		      ((unsigned long)current->stack & stack_mask)))
+			info->dma = 0;
 	}
 
 	/* start sending the command to the drive */
-- 
1.5.4.2

[PATCH v2 -mm 5/6] scsi: sr avoids useless buffer allocation

[FUJITA Tomonori]

blk_rq_map_kern can handle the stack buffers correctly (avoid DMA
from/to the stack buffers by using the bounce buffer) so we don't need
to complicate the code by allocating just 8 bytes.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/sr.c |   20 +++++---------------
 1 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7ee86d4..c53701e 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -670,24 +670,20 @@ fail:
 static void get_sectorsize(struct scsi_cd *cd)
 {
 	unsigned char cmd[10];
-	unsigned char *buffer;
+	unsigned char buffer[8];
 	int the_result, retries = 3;
 	int sector_size;
 	struct request_queue *queue;
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
-	if (!buffer)
-		goto Enomem;
-
 	do {
 		cmd[0] = READ_CAPACITY;
 		memset((void *) &cmd[1], 0, 9);
-		memset(buffer, 0, 8);
+		memset(buffer, 0, sizeof(buffer));
 
 		/* Do the command and wait.. */
 		the_result = scsi_execute_req(cd->device, cmd, DMA_FROM_DEVICE,
-					      buffer, 8, NULL, SR_TIMEOUT,
-					      MAX_RETRIES);
+					      buffer, sizeof(buffer), NULL,
+					      SR_TIMEOUT, MAX_RETRIES);
 
 		retries--;
 
@@ -742,14 +738,8 @@ static void get_sectorsize(struct scsi_cd *cd)
 
 	queue = cd->device->request_queue;
 	blk_queue_hardsect_size(queue, sector_size);
-out:
-	kfree(buffer);
-	return;
 
-Enomem:
-	cd->capacity = 0x1fffff;
-	cd->device->sector_size = 2048;	/* A guess, just in case */
-	goto out;
+	return;
 }
 
 static void get_capabilities(struct scsi_cd *cd)
-- 
1.5.4.2

[PATCH v2 -mm 6/6] cdrom: revert commit 22a9189 (cdrom: use kmalloced buffers instead of buffers on stack)

[FUJITA Tomonori]

The commit 22a9189fd073db3d03a4cf8b8c098aa207602de1 (cdrom: use
kmalloced buffers instead of buffers on stack) is introduced to use
kmalloced buffers for packet commands to avoid stack corruption on non
coherent platforms.

SCSI cdrom uses blk_rq_map_kern, which properly avoids DMA on the
stack by using the bounce buffers. IDE cdrom also has the mechnism to
avoids DMA on the stack. So we don't need this extra complexitiy in
cdrom.c, such as allocating just 8 bytes. The lower layers can handle
it.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 drivers/cdrom/cdrom.c |  274 +++++++++++++++++--------------------------------
 1 files changed, 93 insertions(+), 181 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 69f26eb..a5da356 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -461,37 +461,27 @@ int cdrom_get_media_event(struct cdrom_device_info *cdi,
 			  struct media_event_desc *med)
 {
 	struct packet_command cgc;
-	unsigned char *buffer;
-	struct event_header *eh;
-	int ret = 1;
-
-	buffer = kmalloc(8, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
+	unsigned char buffer[8];
+	struct event_header *eh = (struct event_header *) buffer;
 
-	eh = (struct event_header *)buffer;
-
-	init_cdrom_command(&cgc, buffer, 8, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 	cgc.cmd[0] = GPCMD_GET_EVENT_STATUS_NOTIFICATION;
 	cgc.cmd[1] = 1;		/* IMMED */
 	cgc.cmd[4] = 1 << 4;	/* media event */
-	cgc.cmd[8] = 8;
+	cgc.cmd[8] = sizeof(buffer);
 	cgc.quiet = 1;
 
 	if (cdi->ops->generic_packet(cdi, &cgc))
-		goto err;
+		return 1;
 
 	if (be16_to_cpu(eh->data_len) < sizeof(*med))
-		goto err;
+		return 1;
 
 	if (eh->nea || eh->notification_class != 0x4)
-		goto err;
+		return 1;
 
-	memcpy(med, buffer + sizeof(*eh), sizeof(*med));
-	ret = 0;
-err:
-	kfree(buffer);
-	return ret;
+	memcpy(med, &buffer[sizeof(*eh)], sizeof(*med));
+	return 0;
 }
 
 /*
@@ -501,82 +491,68 @@ err:
 static int cdrom_mrw_probe_pc(struct cdrom_device_info *cdi)
 {
 	struct packet_command cgc;
-	char *buffer;
-	int ret = 1;
-
-	buffer = kmalloc(16, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
+	char buffer[16];
 
-	init_cdrom_command(&cgc, buffer, 16, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 
 	cgc.timeout = HZ;
 	cgc.quiet = 1;
 
 	if (!cdrom_mode_sense(cdi, &cgc, MRW_MODE_PC, 0)) {
 		cdi->mrw_mode_page = MRW_MODE_PC;
-		ret = 0;
+		return 0;
 	} else if (!cdrom_mode_sense(cdi, &cgc, MRW_MODE_PC_PRE1, 0)) {
 		cdi->mrw_mode_page = MRW_MODE_PC_PRE1;
-		ret = 0;
+		return 0;
 	}
-	kfree(buffer);
-	return ret;
+
+	return 1;
 }
 
 static int cdrom_is_mrw(struct cdrom_device_info *cdi, int *write)
 {
 	struct packet_command cgc;
 	struct mrw_feature_desc *mfd;
-	unsigned char *buffer;
+	unsigned char buffer[16];
 	int ret;
 
 	*write = 0;
-	buffer = kmalloc(16, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
 
-	init_cdrom_command(&cgc, buffer, 16, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 
 	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
 	cgc.cmd[3] = CDF_MRW;
-	cgc.cmd[8] = 16;
+	cgc.cmd[8] = sizeof(buffer);
 	cgc.quiet = 1;
 
 	if ((ret = cdi->ops->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	mfd = (struct mrw_feature_desc *)&buffer[sizeof(struct feature_header)];
-	if (be16_to_cpu(mfd->feature_code) != CDF_MRW) {
-		ret = 1;
-		goto err;
-	}
+	if (be16_to_cpu(mfd->feature_code) != CDF_MRW)
+		return 1;
 	*write = mfd->write;
 
 	if ((ret = cdrom_mrw_probe_pc(cdi))) {
 		*write = 0;
+		return ret;
 	}
-err:
-	kfree(buffer);
-	return ret;
+
+	return 0;
 }
 
 static int cdrom_mrw_bgformat(struct cdrom_device_info *cdi, int cont)
 {
 	struct packet_command cgc;
-	unsigned char *buffer;
+	unsigned char buffer[12];
 	int ret;
 
 	printk(KERN_INFO "cdrom: %sstarting format\n", cont ? "Re" : "");
 
-	buffer = kmalloc(12, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
 	/*
 	 * FmtData bit set (bit 4), format type is 1
 	 */
-	init_cdrom_command(&cgc, buffer, 12, CGC_DATA_WRITE);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_WRITE);
 	cgc.cmd[0] = GPCMD_FORMAT_UNIT;
 	cgc.cmd[1] = (1 << 4) | 1;
 
@@ -603,7 +579,6 @@ static int cdrom_mrw_bgformat(struct cdrom_device_info *cdi, int cont)
 	if (ret)
 		printk(KERN_INFO "cdrom: bgformat failed\n");
 
-	kfree(buffer);
 	return ret;
 }
 
@@ -663,17 +638,16 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info *cdi, int space)
 {
 	struct packet_command cgc;
 	struct mode_page_header *mph;
-	char *buffer;
+	char buffer[16];
 	int ret, offset, size;
 
-	buffer = kmalloc(16, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 
-	init_cdrom_command(&cgc, buffer, 16, CGC_DATA_READ);
+	cgc.buffer = buffer;
+	cgc.buflen = sizeof(buffer);
 
 	if ((ret = cdrom_mode_sense(cdi, &cgc, cdi->mrw_mode_page, 0)))
-		goto err;
+		return ret;
 
 	mph = (struct mode_page_header *) buffer;
 	offset = be16_to_cpu(mph->desc_length);
@@ -683,70 +657,55 @@ static int cdrom_mrw_set_lba_space(struct cdrom_device_info *cdi, int space)
 	cgc.buflen = size;
 
 	if ((ret = cdrom_mode_select(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	printk(KERN_INFO "cdrom: %s: mrw address space %s selected\n", cdi->name, mrw_address_space[space]);
-	ret = 0;
-err:
-	kfree(buffer);
-	return ret;
+	return 0;
 }
 
 static int cdrom_get_random_writable(struct cdrom_device_info *cdi,
 			      struct rwrt_feature_desc *rfd)
 {
 	struct packet_command cgc;
-	char *buffer;
+	char buffer[24];
 	int ret;
 
-	buffer = kmalloc(24, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	init_cdrom_command(&cgc, buffer, 24, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 
 	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;	/* often 0x46 */
 	cgc.cmd[3] = CDF_RWRT;			/* often 0x0020 */
-	cgc.cmd[8] = 24;		        /* often 0x18 */
+	cgc.cmd[8] = sizeof(buffer);		/* often 0x18 */
 	cgc.quiet = 1;
 
 	if ((ret = cdi->ops->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	memcpy(rfd, &buffer[sizeof(struct feature_header)], sizeof (*rfd));
-	ret = 0;
-err:
-	kfree(buffer);
-	return ret;
+	return 0;
 }
 
 static int cdrom_has_defect_mgt(struct cdrom_device_info *cdi)
 {
 	struct packet_command cgc;
-	char *buffer;
+	char buffer[16];
 	__be16 *feature_code;
 	int ret;
 
-	buffer = kmalloc(16, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	init_cdrom_command(&cgc, buffer, 16, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 
 	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
 	cgc.cmd[3] = CDF_HWDM;
-	cgc.cmd[8] = 16;
+	cgc.cmd[8] = sizeof(buffer);
 	cgc.quiet = 1;
 
 	if ((ret = cdi->ops->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	feature_code = (__be16 *) &buffer[sizeof(struct feature_header)];
 	if (be16_to_cpu(*feature_code) == CDF_HWDM)
-		ret = 0;
-err:
-	kfree(buffer);
-	return ret;
+		return 0;
+
+	return 1;
 }
 
 
@@ -837,14 +796,10 @@ static int cdrom_mrw_open_write(struct cdrom_device_info *cdi)
 static int mo_open_write(struct cdrom_device_info *cdi)
 {
 	struct packet_command cgc;
-	char *buffer;
+	char buffer[255];
 	int ret;
 
-	buffer = kmalloc(255, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	init_cdrom_command(&cgc, buffer, 4, CGC_DATA_READ);
+	init_cdrom_command(&cgc, &buffer, 4, CGC_DATA_READ);
 	cgc.quiet = 1;
 
 	/*
@@ -861,15 +816,10 @@ static int mo_open_write(struct cdrom_device_info *cdi)
 	}
 
 	/* drive gave us no info, let the user go ahead */
-	if (ret) {
-		ret = 0;
-		goto err;
-	}
+	if (ret)
+		return 0;
 
-	ret = buffer[3] & 0x80;
-err:
-	kfree(buffer);
-	return ret;
+	return buffer[3] & 0x80;
 }
 
 static int cdrom_ram_open_write(struct cdrom_device_info *cdi)
@@ -892,19 +842,15 @@ static int cdrom_ram_open_write(struct cdrom_device_info *cdi)
 static void cdrom_mmc3_profile(struct cdrom_device_info *cdi)
 {
 	struct packet_command cgc;
-	char *buffer;
+	char buffer[32];
 	int ret, mmc3_profile;
 
-	buffer = kmalloc(32, GFP_KERNEL);
-	if (!buffer)
-		return;
-
-	init_cdrom_command(&cgc, buffer, 32, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buffer, sizeof(buffer), CGC_DATA_READ);
 
 	cgc.cmd[0] = GPCMD_GET_CONFIGURATION;
 	cgc.cmd[1] = 0;
 	cgc.cmd[2] = cgc.cmd[3] = 0;		/* Starting Feature Number */
-	cgc.cmd[8] = 32;		        /* Allocation Length */
+	cgc.cmd[8] = sizeof(buffer);		/* Allocation Length */
 	cgc.quiet = 1;
 
 	if ((ret = cdi->ops->generic_packet(cdi, &cgc)))
@@ -913,7 +859,6 @@ static void cdrom_mmc3_profile(struct cdrom_device_info *cdi)
 		mmc3_profile = (buffer[6] << 8) | buffer[7];
 
 	cdi->mmc3_profile = mmc3_profile;
-	kfree(buffer);
 }
 
 static int cdrom_is_dvd_rw(struct cdrom_device_info *cdi)
@@ -1628,15 +1573,12 @@ static void setup_send_key(struct packet_command *cgc, unsigned agid, unsigned t
 static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 {
 	int ret;
-	u_char *buf;
+	u_char buf[20];
 	struct packet_command cgc;
 	struct cdrom_device_ops *cdo = cdi->ops;
-	rpc_state_t *rpc_state;
-
-	buf = kzalloc(20, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
+	rpc_state_t rpc_state;
 
+	memset(buf, 0, sizeof(buf));
 	init_cdrom_command(&cgc, buf, 0, CGC_DATA_READ);
 
 	switch (ai->type) {
@@ -1647,7 +1589,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		setup_report_key(&cgc, ai->lsa.agid, 0);
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
 		ai->lsa.agid = buf[7] >> 6;
 		/* Returning data, let host change state */
@@ -1658,7 +1600,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		setup_report_key(&cgc, ai->lsk.agid, 2);
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
 		copy_key(ai->lsk.key, &buf[4]);
 		/* Returning data, let host change state */
@@ -1669,7 +1611,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		setup_report_key(&cgc, ai->lsc.agid, 1);
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
 		copy_chal(ai->lsc.chal, &buf[4]);
 		/* Returning data, let host change state */
@@ -1686,7 +1628,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		cgc.cmd[2] = ai->lstk.lba >> 24;
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
 		ai->lstk.cpm = (buf[4] >> 7) & 1;
 		ai->lstk.cp_sec = (buf[4] >> 6) & 1;
@@ -1700,7 +1642,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		setup_report_key(&cgc, ai->lsasf.agid, 5);
 		
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
 		ai->lsasf.asf = buf[7] & 1;
 		break;
@@ -1713,7 +1655,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		copy_chal(&buf[4], ai->hsc.chal);
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
 		ai->type = DVD_LU_SEND_KEY1;
 		break;
@@ -1726,7 +1668,7 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 
 		if ((ret = cdo->generic_packet(cdi, &cgc))) {
 			ai->type = DVD_AUTH_FAILURE;
-			goto err;
+			return ret;
 		}
 		ai->type = DVD_AUTH_ESTABLISHED;
 		break;
@@ -1737,23 +1679,24 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		cdinfo(CD_DVD, "entering DVD_INVALIDATE_AGID\n"); 
 		setup_report_key(&cgc, ai->lsa.agid, 0x3f);
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 		break;
 
 	/* Get region settings */
 	case DVD_LU_SEND_RPC_STATE:
 		cdinfo(CD_DVD, "entering DVD_LU_SEND_RPC_STATE\n");
 		setup_report_key(&cgc, 0, 8);
+		memset(&rpc_state, 0, sizeof(rpc_state_t));
+		cgc.buffer = (char *) &rpc_state;
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 
-		rpc_state = (rpc_state_t *)buf;
-		ai->lrpcs.type = rpc_state->type_code;
-		ai->lrpcs.vra = rpc_state->vra;
-		ai->lrpcs.ucca = rpc_state->ucca;
-		ai->lrpcs.region_mask = rpc_state->region_mask;
-		ai->lrpcs.rpc_scheme = rpc_state->rpc_scheme;
+		ai->lrpcs.type = rpc_state.type_code;
+		ai->lrpcs.vra = rpc_state.vra;
+		ai->lrpcs.ucca = rpc_state.ucca;
+		ai->lrpcs.region_mask = rpc_state.region_mask;
+		ai->lrpcs.rpc_scheme = rpc_state.rpc_scheme;
 		break;
 
 	/* Set region settings */
@@ -1764,23 +1707,20 @@ static int dvd_do_auth(struct cdrom_device_info *cdi, dvd_authinfo *ai)
 		buf[4] = ai->hrpcs.pdrc;
 
 		if ((ret = cdo->generic_packet(cdi, &cgc)))
-			goto err;
+			return ret;
 		break;
 
 	default:
 		cdinfo(CD_WARNING, "Invalid DVD key ioctl (%d)\n", ai->type);
-		ret = -ENOTTY;
-		goto err;
+		return -ENOTTY;
 	}
-	ret = 0;
-err:
-	kfree(buf);
-	return ret;
+
+	return 0;
 }
 
 static int dvd_read_physical(struct cdrom_device_info *cdi, dvd_struct *s)
 {
-	unsigned char *buf, *base;
+	unsigned char buf[21], *base;
 	struct dvd_layer *layer;
 	struct packet_command cgc;
 	struct cdrom_device_ops *cdo = cdi->ops;
@@ -1789,11 +1729,7 @@ static int dvd_read_physical(struct cdrom_device_info *cdi, dvd_struct *s)
 	if (layer_num >= DVD_LAYERS)
 		return -EINVAL;
 
-	buf = kmalloc(21, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	init_cdrom_command(&cgc, buf, 21, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
 	cgc.cmd[0] = GPCMD_READ_DVD_STRUCTURE;
 	cgc.cmd[6] = layer_num;
 	cgc.cmd[7] = s->type;
@@ -1805,7 +1741,7 @@ static int dvd_read_physical(struct cdrom_device_info *cdi, dvd_struct *s)
 	cgc.quiet = 1;
 
 	if ((ret = cdo->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	base = &buf[4];
 	layer = &s->physical.layer[layer_num];
@@ -1829,24 +1765,17 @@ static int dvd_read_physical(struct cdrom_device_info *cdi, dvd_struct *s)
 	layer->end_sector_l0 = base[13] << 16 | base[14] << 8 | base[15];
 	layer->bca = base[16] >> 7;
 
-	ret = 0;
-err:
-	kfree(buf);
-	return ret;
+	return 0;
 }
 
 static int dvd_read_copyright(struct cdrom_device_info *cdi, dvd_struct *s)
 {
 	int ret;
-	u_char *buf;
+	u_char buf[8];
 	struct packet_command cgc;
 	struct cdrom_device_ops *cdo = cdi->ops;
 
-	buf = kmalloc(8, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	init_cdrom_command(&cgc, buf, 8, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
 	cgc.cmd[0] = GPCMD_READ_DVD_STRUCTURE;
 	cgc.cmd[6] = s->copyright.layer_num;
 	cgc.cmd[7] = s->type;
@@ -1854,15 +1783,12 @@ static int dvd_read_copyright(struct cdrom_device_info *cdi, dvd_struct *s)
 	cgc.cmd[9] = cgc.buflen & 0xff;
 
 	if ((ret = cdo->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	s->copyright.cpst = buf[4];
 	s->copyright.rmi = buf[5];
 
-	ret = 0;
-err:
-	kfree(buf);
-	return ret;
+	return 0;
 }
 
 static int dvd_read_disckey(struct cdrom_device_info *cdi, dvd_struct *s)
@@ -1894,33 +1820,26 @@ static int dvd_read_disckey(struct cdrom_device_info *cdi, dvd_struct *s)
 static int dvd_read_bca(struct cdrom_device_info *cdi, dvd_struct *s)
 {
 	int ret;
-	u_char *buf;
+	u_char buf[4 + 188];
 	struct packet_command cgc;
 	struct cdrom_device_ops *cdo = cdi->ops;
 
-	buf = kmalloc(4 + 188, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	init_cdrom_command(&cgc, buf, 4 + 188, CGC_DATA_READ);
+	init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
 	cgc.cmd[0] = GPCMD_READ_DVD_STRUCTURE;
 	cgc.cmd[7] = s->type;
 	cgc.cmd[9] = cgc.buflen & 0xff;
 
 	if ((ret = cdo->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	s->bca.len = buf[0] << 8 | buf[1];
 	if (s->bca.len < 12 || s->bca.len > 188) {
 		cdinfo(CD_WARNING, "Received invalid BCA length (%d)\n", s->bca.len);
-		ret = -EIO;
-		goto err;
+		return -EIO;
 	}
 	memcpy(s->bca.value, &buf[4], s->bca.len);
-	ret = 0;
-err:
-	kfree(buf);
-	return ret;
+
+	return 0;
 }
 
 static int dvd_read_manufact(struct cdrom_device_info *cdi, dvd_struct *s)
@@ -2020,13 +1939,9 @@ static int cdrom_read_subchannel(struct cdrom_device_info *cdi,
 {
 	struct cdrom_device_ops *cdo = cdi->ops;
 	struct packet_command cgc;
-	char *buffer;
+	char buffer[32];
 	int ret;
 
-	buffer = kmalloc(32, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
 	init_cdrom_command(&cgc, buffer, 16, CGC_DATA_READ);
 	cgc.cmd[0] = GPCMD_READ_SUBCHANNEL;
 	cgc.cmd[1] = 2;     /* MSF addressing */
@@ -2035,7 +1950,7 @@ static int cdrom_read_subchannel(struct cdrom_device_info *cdi,
 	cgc.cmd[8] = 16;
 
 	if ((ret = cdo->generic_packet(cdi, &cgc)))
-		goto err;
+		return ret;
 
 	subchnl->cdsc_audiostatus = cgc.buffer[1];
 	subchnl->cdsc_format = CDROM_MSF;
@@ -2050,10 +1965,7 @@ static int cdrom_read_subchannel(struct cdrom_device_info *cdi,
 	subchnl->cdsc_absaddr.msf.second = cgc.buffer[10];
 	subchnl->cdsc_absaddr.msf.frame = cgc.buffer[11];
 
-	ret = 0;
-err:
-	kfree(buffer);
-	return ret;
+	return 0;
 }
 
 /*
-- 
1.5.4.2

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[Borislav Petkov]

On Mon, Jun 02, 2008 at 03:57:30PM +0900, FUJITA Tomonori wrote:
> Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> leads to memory corruption on a non-coherent platform.
> 
> With regard to alignment and padding, ide-cd has the the dma safe
> check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> check to that check.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Borislav Petkov <petkovbb@gmail.com>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-cd.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index e3f085c..e12d602 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  		struct request_queue *q = drive->queue;
>  		unsigned int alignment;
>  		unsigned long addr;
> +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
>  
>  		if (rq->bio)
>  			addr = (unsigned long)bio_data(rq->bio);
> @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
>  		if (addr & alignment || rq->data_len & alignment)
>  			info->dma = 0;
> +
> +		if (!((addr & stack_mask) ^
> +		      ((unsigned long)current->stack & stack_mask)))
> +			info->dma = 0;
>  	}
>  
>  	/* start sending the command to the drive */
> -- 
> 1.5.4.2

Looks good, thanks.

Acked-by: Borislav Petkov <petkovbb@gmail.com>

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

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[James Bottomley]

On Mon, 2008-06-02 at 15:57 +0900, FUJITA Tomonori wrote:
> Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> leads to memory corruption on a non-coherent platform.
> 
> With regard to alignment and padding, ide-cd has the the dma safe
> check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> check to that check.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Borislav Petkov <petkovbb@gmail.com>
> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-cd.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> index e3f085c..e12d602 100644
> --- a/drivers/ide/ide-cd.c
> +++ b/drivers/ide/ide-cd.c
> @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  		struct request_queue *q = drive->queue;
>  		unsigned int alignment;
>  		unsigned long addr;
> +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
>  
>  		if (rq->bio)
>  			addr = (unsigned long)bio_data(rq->bio);
> @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
>  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
>  		if (addr & alignment || rq->data_len & alignment)
>  			info->dma = 0;
> +
> +		if (!((addr & stack_mask) ^
> +		      ((unsigned long)current->stack & stack_mask)))

That can basically become

if ((addr & stack_mask)  == ((unsigned long)current->stack & stack_mask))

to be a bit clearer, can't it?

I'm also not keen on the use of current->stack.  It looks like this
commit:

commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
Author: Roman Zippel <zippel@linux-m68k.org>
Date:   Wed May 9 02:35:17 2007 -0700

    rename thread_info to stack

Introduced a task_stack_page() accessor to get this instead, so perhaps
we should use it (I've cc'd Roman and linux-arch for opinions).

> +			info->dma = 0;
>  	}
>  
>  	/* start sending the command to the drive */

James

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[Borislav Petkov]

On Thu, Jun 05, 2008 at 11:10:48AM -0500, James Bottomley wrote:
> On Mon, 2008-06-02 at 15:57 +0900, FUJITA Tomonori wrote:
> > Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> > leads to memory corruption on a non-coherent platform.
> > 
> > With regard to alignment and padding, ide-cd has the the dma safe
> > check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> > check to that check.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Borislav Petkov <petkovbb@gmail.com>
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> >  drivers/ide/ide-cd.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index e3f085c..e12d602 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		struct request_queue *q = drive->queue;
> >  		unsigned int alignment;
> >  		unsigned long addr;
> > +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
> >  
> >  		if (rq->bio)
> >  			addr = (unsigned long)bio_data(rq->bio);
> > @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
> >  		if (addr & alignment || rq->data_len & alignment)
> >  			info->dma = 0;
> > +
> > +		if (!((addr & stack_mask) ^
> > +		      ((unsigned long)current->stack & stack_mask)))
> 
> That can basically become
> 
> if ((addr & stack_mask)  == ((unsigned long)current->stack & stack_mask))
> 
> to be a bit clearer, can't it?

yep, yep. Clearer it is :).
> 
> I'm also not keen on the use of current->stack.  It looks like this
> commit:
> 
> commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
> Author: Roman Zippel <zippel@linux-m68k.org>
> Date:   Wed May 9 02:35:17 2007 -0700
> 
>     rename thread_info to stack
> 
> Introduced a task_stack_page() accessor to get this instead, so perhaps
> we should use it (I've cc'd Roman and linux-arch for opinions).
> 
> > +			info->dma = 0;
> >  	}
> >  
> >  	/* start sending the command to the drive */
> 
> James
> 

-- 
Regards/Gruß,
    Boris.

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[FUJITA Tomonori]

On Thu, 05 Jun 2008 11:10:48 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Mon, 2008-06-02 at 15:57 +0900, FUJITA Tomonori wrote:
> > Some REQ_TYPE_ATA_PC commands uses the stack buffers for DMA, which
> > leads to memory corruption on a non-coherent platform.
> > 
> > With regard to alignment and padding, ide-cd has the the dma safe
> > check for sg requests and REQ_TYPE_ATA_PC. This adds the stack buffer
> > check to that check.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Cc: Borislav Petkov <petkovbb@gmail.com>
> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > ---
> >  drivers/ide/ide-cd.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
> > index e3f085c..e12d602 100644
> > --- a/drivers/ide/ide-cd.c
> > +++ b/drivers/ide/ide-cd.c
> > @@ -1195,6 +1195,7 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		struct request_queue *q = drive->queue;
> >  		unsigned int alignment;
> >  		unsigned long addr;
> > +		unsigned long stack_mask = ~(THREAD_SIZE - 1);
> >  
> >  		if (rq->bio)
> >  			addr = (unsigned long)bio_data(rq->bio);
> > @@ -1212,6 +1213,10 @@ static ide_startstop_t cdrom_do_block_pc(ide_drive_t *drive, struct request *rq)
> >  		alignment = queue_dma_alignment(q) | q->dma_pad_mask;
> >  		if (addr & alignment || rq->data_len & alignment)
> >  			info->dma = 0;
> > +
> > +		if (!((addr & stack_mask) ^
> > +		      ((unsigned long)current->stack & stack_mask)))
> 
> That can basically become
> 
> if ((addr & stack_mask)  == ((unsigned long)current->stack & stack_mask))
> 
> to be a bit clearer, can't it?

Ok, I'll do next time.


> I'm also not keen on the use of current->stack.  It looks like this
> commit:
> 
> commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
> Author: Roman Zippel <zippel@linux-m68k.org>
> Date:   Wed May 9 02:35:17 2007 -0700
> 
>     rename thread_info to stack
> 
> Introduced a task_stack_page() accessor to get this instead, so perhaps
> we should use it (I've cc'd Roman and linux-arch for opinions).

Thanks. Seems that we should use it here (though current->stack look
like work now).

lib/debugobjects.c has the same code (that uses current->stack) to
detect an address on the stack.

Next time, I'll add a helper function to detect an address on the
stack, which is useful for debugobjects.c, blk-map.c and ide-cd.c.

Re: [PATCH v2 -mm 4/6] ide: avoid DMA on the stack for REQ_TYPE_ATA_PC

[Roman Zippel]

Hi,

On Thu, 5 Jun 2008, James Bottomley wrote:

> I'm also not keen on the use of current->stack.  It looks like this
> commit:
> 
> commit f7e4217b007d1f73e7e3cf10ba4fea4a608c603f
> Author: Roman Zippel <zippel@linux-m68k.org>
> Date:   Wed May 9 02:35:17 2007 -0700
> 
>     rename thread_info to stack
> 
> Introduced a task_stack_page() accessor to get this instead, so perhaps
> we should use it (I've cc'd Roman and linux-arch for opinions).

This helper was mainly introduced to help with the transition from direct 
thread_info access. I don't see this field go away or change somehow in 
the near future, so direct access of stack is IMO ok, but we already have 
the helper, so using it to improve readability is fine too.

bye, Roman