Re: [Bugme-new] [Bug 7667] New: BUG at drivers/scsi/scsi_lib.c:1118 caused by "pktsetup dvd /dev/sr0"

[Boaz Harrosh <bharrosh@panasas.com>]

Christoph Hellwig wrote:
> On Tue, Dec 12, 2006 at 11:38:42AM +0100, Christoph Hellwig wrote:
>> This is because the packet driver tries to send down read/write
>> BLOCK_PC commands that don't use a bio and do not use sg lists.
>> As part of the patch you mentioned I added strict assertations for that
>> case because the scsi layer doesn't handle those anymore.
>>
>> The right fix is to replace all the packet_command stuff in the packet
>> driver by scsi_execute() which needs to be lifted from scsi code to
>> the block code for that.  I'll prepare a patch this weekend unless
>> someone beets me in doing that work.
> 
> Please try the patch below to fix the bug for now.  It's not the
> full way to a generic execute block pc infrastcuture but should fix
> the bug for the time beeing:
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/drivers/block/pktcdvd.c
> ===================================================================
> --- linux-2.6.orig/drivers/block/pktcdvd.c	2006-12-12 11:30:45.000000000 +0100
> +++ linux-2.6/drivers/block/pktcdvd.c	2006-12-12 14:23:37.000000000 +0100
> @@ -765,47 +765,34 @@
>   */
>  static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *cgc)
>  {
> -	char sense[SCSI_SENSE_BUFFERSIZE];
> -	request_queue_t *q;
> +	request_queue_t *q = bdev_get_queue(pd->bdev);
>  	struct request *rq;
> -	DECLARE_COMPLETION_ONSTACK(wait);
> -	int err = 0;
> +	int ret = 0;
>  
> -	q = bdev_get_queue(pd->bdev);
> +	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ?
> +			     WRITE : READ, __GFP_WAIT);
> +
> +	if (cgc->buflen) {
> +		if (blk_rq_map_kern(q, rq, cgc->buffer, cgc->buflen, __GFP_WAIT))
> +			goto out;
> +	}
> +
> +	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
> +	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
> +	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
> +		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
>  
> -	rq = blk_get_request(q, (cgc->data_direction == CGC_DATA_WRITE) ? WRITE : READ,
> -			     __GFP_WAIT);
> -	rq->errors = 0;
> -	rq->rq_disk = pd->bdev->bd_disk;
> -	rq->bio = NULL;
> -	rq->buffer = NULL;
>  	rq->timeout = 60*HZ;
> -	rq->data = cgc->buffer;
> -	rq->data_len = cgc->buflen;
> -	rq->sense = sense;
> -	memset(sense, 0, sizeof(sense));
> -	rq->sense_len = 0;
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->cmd_flags |= REQ_HARDBARRIER;
>  	if (cgc->quiet)
>  		rq->cmd_flags |= REQ_QUIET;
> -	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
> -	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
> -		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
> -	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
> -
> -	rq->ref_count++;
> -	rq->end_io_data = &wait;
> -	rq->end_io = blk_end_sync_rq;
> -	elv_add_request(q, rq, ELEVATOR_INSERT_BACK, 1);
> -	generic_unplug_device(q);
> -	wait_for_completion(&wait);
> -
> -	if (rq->errors)
> -		err = -EIO;
>  
> +	blk_execute_rq(rq->q, pd->bdev->bd_disk, rq, 0);
> +	ret = rq->errors;
> +out:
>  	blk_put_request(rq);
> -	return err;
> +	return ret;
>  }
>  
>  /*
> -
> 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
I'm afraid this might not be enough because of code in drivers/ide/ide-cd.c. It does IO off of request->data.

[background]
pkt_generic_packet and ton of other places mainly cd(s), floppy(s), and other ide code paths,
are using what I call BLACK requests. They put some data on request->buffer or request->data
stick it in the Q and than advance on them later down the ladder. Remove of "buffer" and "data" from
struct request will reveal all these places. At one time I had plans to do just that. But 1/2 way through I gave
up, it is too risky, too much Hardware that I don't have, that needs checking.

below patch combined with your patch might get a bit closer for this code path.
At struct request I have changed the name of "data" member to "user_data". than changed the code paths that used
"data" as IO to use request->buffer instead. This is just as bad but is a more common practice.

I suspect there is a problem with what I did in scsi_lib.c Christoph please check me out with the new BUG_ON.
Mainly what you need from below is only the code in ide-cd.c.
(And there are 3-4 places that do exactly like pkt_generic_packet, though I'm not sure they end up through SCSI.
At first I thought this code doesn't either)

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 9eaee66..f52a4f2 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -257,7 +257,7 @@ static void rq_init(request_queue_t *q,
 	rq->q = q;
 	rq->special = NULL;
 	rq->data_len = 0;
-	rq->data = NULL;
+	rq->user_data = NULL;
 	rq->nr_phys_segments = 0;
 	rq->sense = NULL;
 	rq->end_io = NULL;
@@ -1199,7 +1199,7 @@ void blk_dump_rq_flags(struct request *r
 	printk("\nsector %llu, nr/cnr %lu/%u\n", (unsigned long long)rq->sector,
 						       rq->nr_sectors,
 						       rq->current_nr_sectors);
-	printk("bio %p, biotail %p, buffer %p, data %p, len %u\n", rq->bio, rq->biotail, rq->buffer, rq->data, rq->data_len);
+	printk("bio %p, biotail %p, buffer %p, user_data %p, len %u\n", rq->bio, rq->biotail, rq->buffer, rq->user_data, rq->data_len);

 	if (blk_pc_request(rq)) {
 		printk("cdb: ");
@@ -2370,7 +2370,7 @@ int blk_rq_map_user(request_queue_t *q,
 		rq->bio = rq->biotail = bio;
 		blk_rq_bio_prep(q, rq, bio);

-		rq->buffer = rq->data = NULL;
+		rq->buffer = NULL;
 		rq->data_len = len;
 		return 0;
 	}
@@ -2420,7 +2420,7 @@ int blk_rq_map_user_iov(request_queue_t

 	rq->bio = rq->biotail = bio;
 	blk_rq_bio_prep(q, rq, bio);
-	rq->buffer = rq->data = NULL;
+	rq->buffer = NULL;
 	rq->data_len = bio->bi_size;
 	return 0;
 }
@@ -2479,7 +2479,7 @@ int blk_rq_map_kern(request_queue_t *q,
 	rq->bio = rq->biotail = bio;
 	blk_rq_bio_prep(q, rq, bio);

-	rq->buffer = rq->data = NULL;
+	rq->buffer = NULL;
 	rq->data_len = len;
 	return 0;
 }
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index dcd9c71..d163a2c 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -502,7 +502,6 @@ static int __blk_send_generic(request_qu

 	rq = blk_get_request(q, WRITE, __GFP_WAIT);
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
-	rq->data = NULL;
 	rq->data_len = 0;
 	rq->timeout = BLK_DEFAULT_TIMEOUT;
 	memset(rq->cmd, 0, sizeof(rq->cmd));
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index f2904f6..b72758c 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -360,9 +360,8 @@ static int pkt_generic_packet(struct pkt
 	rq->errors = 0;
 	rq->rq_disk = pd->bdev->bd_disk;
 	rq->bio = NULL;
-	rq->buffer = NULL;
 	rq->timeout = 60*HZ;
-	rq->data = cgc->buffer;
+	rq->buffer = cgc->buffer;
 	rq->data_len = cgc->buflen;
 	rq->sense = sense;
 	memset(sense, 0, sizeof(sense));
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 8821494..00fce03 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -613,14 +613,14 @@ static void cdrom_queue_request_sense(id
 	/* stuff the sense request in front of our current request */
 	cdrom_prepare_request(drive, rq);

-	rq->data = sense;
+	rq->buffer = sense;
 	rq->cmd[0] = GPCMD_REQUEST_SENSE;
 	rq->cmd[4] = rq->data_len = 18;

 	rq->cmd_type = REQ_TYPE_SENSE;

-	/* NOTE! Save the failed command in "rq->buffer" */
-	rq->buffer = (void *) failed_command;
+	/* NOTE! Save the failed command in "rq->user_data" */
+	rq->user_data = failed_command;

 	(void) ide_do_drive_cmd(drive, rq, ide_preempt);
 }
@@ -632,10 +632,10 @@ static void cdrom_end_request (ide_drive

 	if (blk_sense_request(rq) && uptodate) {
 		/*
-		 * For REQ_TYPE_SENSE, "rq->buffer" points to the original
+		 * For REQ_TYPE_SENSE, "rq->user_data" points to the original
 		 * failed request
 		 */
-		struct request *failed = (struct request *) rq->buffer;
+		struct request *failed = rq->user_data;
 		struct cdrom_info *info = drive->driver_data;
 		void *sense = &info->sense_data;
 		unsigned long flags;
@@ -1441,7 +1441,7 @@ static ide_startstop_t cdrom_pc_intr (id
 		    rq->data_len > 0 &&
 		    rq->data_len <= 5) {
 			while (rq->data_len > 0) {
-				*(unsigned char *)rq->data++ = 0;
+				*(unsigned char *)rq->buffer++ = 0;
 				--rq->data_len;
 			}
 		}
@@ -1468,12 +1468,12 @@ static ide_startstop_t cdrom_pc_intr (id

 	/* The drive wants to be written to. */
 	if ((ireason & 3) == 0) {
-		if (!rq->data) {
+		if (!rq->buffer) {
 			blk_dump_rq_flags(rq, "cdrom_pc_intr, write");
 			goto confused;
 		}
 		/* Transfer the data. */
-		HWIF(drive)->atapi_output_bytes(drive, rq->data, thislen);
+		HWIF(drive)->atapi_output_bytes(drive, rq->buffer, thislen);

 		/* If we haven't moved enough data to satisfy the drive,
 		   add some padding. */
@@ -1484,18 +1484,18 @@ static ide_startstop_t cdrom_pc_intr (id
 		}

 		/* Keep count of how much data we've moved. */
-		rq->data += thislen;
+		rq->buffer += thislen;
 		rq->data_len -= thislen;
 	}

 	/* Same drill for reading. */
 	else if ((ireason & 3) == 2) {
-		if (!rq->data) {
+		if (!rq->buffer) {
 			blk_dump_rq_flags(rq, "cdrom_pc_intr, write");
 			goto confused;
 		}
 		/* Transfer the data. */
-		HWIF(drive)->atapi_input_bytes(drive, rq->data, thislen);
+		HWIF(drive)->atapi_input_bytes(drive, rq->buffer, thislen);

 		/* If we haven't moved enough data to satisfy the drive,
 		   add some padding. */
@@ -1506,7 +1506,7 @@ static ide_startstop_t cdrom_pc_intr (id
 		}

 		/* Keep count of how much data we've moved. */
-		rq->data += thislen;
+		rq->buffer += thislen;
 		rq->data_len -= thislen;

 		if (blk_sense_request(rq))
@@ -1644,7 +1644,7 @@ static void post_transform_command(struc
 	if (req->bio)
 		ibuf = bio_data(req->bio);
 	else
-		ibuf = req->data;
+		ibuf = req->buffer;

 	if (!ibuf)
 		return;
@@ -1662,7 +1662,7 @@ typedef void (xfer_func_t)(ide_drive_t *

 /*
  * best way to deal with dma that is not sector aligned right now... note
- * that in this path we are not using ->data or ->buffer at all. this irs
+ * that in this path we are not using ->buffer at all. this irs
  * can replace cdrom_pc_intr, cdrom_read_intr, and cdrom_write_intr in the
  * future.
  */
@@ -1745,7 +1745,7 @@ static ide_startstop_t cdrom_newpc_intr(
 	 */
 	while (thislen > 0) {
 		int blen = blen = rq->data_len;
-		char *ptr = rq->data;
+		char *ptr = rq->buffer;

 		/*
 		 * bio backed?
@@ -1772,7 +1772,7 @@ static ide_startstop_t cdrom_newpc_intr(
 		if (rq->bio)
 			end_that_request_chunk(rq, 1, blen);
 		else
-			rq->data += blen;
+			rq->buffer += blen;
 	}

 	/*
@@ -2206,7 +2206,7 @@ static int cdrom_read_capacity(ide_drive

 	req.sense = sense;
 	req.cmd[0] = GPCMD_READ_CDVD_CAPACITY;
-	req.data = (char *)&capbuf;
+	req.buffer = (char *)&capbuf;
 	req.data_len = sizeof(capbuf);
 	req.cmd_flags |= REQ_QUIET;

@@ -2229,7 +2229,7 @@ static int cdrom_read_tocentry(ide_drive
 	cdrom_prepare_request(drive, &req);

 	req.sense = sense;
-	req.data =  buf;
+	req.buffer =  buf;
 	req.data_len = buflen;
 	req.cmd_flags |= REQ_QUIET;
 	req.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
@@ -2324,7 +2324,7 @@ #endif  /* not STANDARD_ATAPI */
 		   If we get an error for the regular case, we assume
 		   a CDI without additional audio tracks. In this case
 		   the readable TOC is empty (CDI tracks are not included)
-		   and only holds the Leadout entry. Heiko Ei�eldt */
+		   and only holds the Leadout entry. Heiko Ei�eldt */
 		ntracks = 0;
 		stat = cdrom_read_tocentry(drive, CDROM_LEADOUT, 1, 0,
 					   (char *)&toc->hdr,
@@ -2426,7 +2426,7 @@ static int cdrom_read_subchannel(ide_dri
 	cdrom_prepare_request(drive, &req);

 	req.sense = sense;
-	req.data = buf;
+	req.buffer = buf;
 	req.data_len = buflen;
 	req.cmd[0] = GPCMD_READ_SUBCHANNEL;
 	req.cmd[1] = 2;     /* MSF addressing */
@@ -2527,7 +2527,7 @@ static int ide_cdrom_packet(struct cdrom
 	memcpy(req.cmd, cgc->cmd, CDROM_PACKET_SIZE);
 	if (cgc->sense)
 		memset(cgc->sense, 0, sizeof(struct request_sense));
-	req.data = cgc->buffer;
+	req.buffer = cgc->buffer;
 	req.data_len = cgc->buflen;
 	req.timeout = cgc->timeout;

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 2614f41..dbf0295 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -141,7 +141,7 @@ enum {

 static void ide_complete_power_step(ide_drive_t *drive, struct request *rq, u8 stat, u8 error)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->user_data;

 	if (drive->media != ide_disk)
 		return;
@@ -167,7 +167,7 @@ static void ide_complete_power_step(ide_

 static ide_startstop_t ide_start_power_step(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->user_data;
 	ide_task_t *args = rq->special;

 	memset(args, 0, sizeof(*args));
@@ -433,7 +433,7 @@ void ide_end_drive_cmd (ide_drive_t *dri
 			}
 		}
 	} else if (blk_pm_request(rq)) {
-		struct request_pm_state *pm = rq->data;
+		struct request_pm_state *pm = rq->user_data;
 #ifdef DEBUG_PM
 		printk("%s: complete_power_step(step: %d, stat: %x, err: %x)\n",
 			drive->name, rq->pm->pm_step, stat, err);
@@ -945,7 +945,7 @@ #endif

 static void ide_check_pm_state(ide_drive_t *drive, struct request *rq)
 {
-	struct request_pm_state *pm = rq->data;
+	struct request_pm_state *pm = rq->user_data;

 	if (blk_pm_suspend_request(rq) &&
 	    pm->pm_step == ide_pm_state_start_suspend)
@@ -1030,7 +1030,7 @@ #endif
 		    rq->cmd_type == REQ_TYPE_ATA_TASKFILE)
 			return execute_drive_cmd(drive, rq);
 		else if (blk_pm_request(rq)) {
-			struct request_pm_state *pm = rq->data;
+			struct request_pm_state *pm = rq->user_data;
 #ifdef DEBUG_PM
 			printk("%s: start_power_step(step: %d)\n",
 				drive->name, rq->pm->pm_step);
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 287a662..47a6110 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -1223,7 +1223,7 @@ static int generic_ide_suspend(struct de
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_SUSPEND;
 	rq.special = &args;
-	rq.data = &rqpm;
+	rq.user_data = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_suspend;
 	if (mesg.event == PM_EVENT_PRETHAW)
 		mesg.event = PM_EVENT_FREEZE;
@@ -1244,7 +1244,7 @@ static int generic_ide_resume(struct dev
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_RESUME;
 	rq.special = &args;
-	rq.data = &rqpm;
+	rq.user_data = &rqpm;
 	rqpm.pm_step = ide_pm_state_start_resume;
 	rqpm.pm_state = PM_EVENT_ON;

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fb616c6..bea6e9e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -351,7 +351,7 @@ static int scsi_req_map_sg(struct reques
 		}
 	}

-	rq->buffer = rq->data = NULL;
+	rq->buffer = NULL;
 	rq->data_len = data_len;
 	return 0;

@@ -1116,12 +1116,11 @@ static int scsi_setup_blk_pc_cmnd(struct
 			return ret;
 	} else {
 		BUG_ON(req->data_len);
-		BUG_ON(req->data);
+		BUG_ON(req->buffer);

 		cmd->request_bufflen = 0;
 		cmd->request_buffer = NULL;
 		cmd->use_sg = 0;
-		req->buffer = NULL;
 	}

 	BUILD_BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd));
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7bfcde2..beb1f8d 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -288,7 +288,8 @@ struct request {
 	unsigned short ioprio;

 	void *special;
-	char *buffer;
+	char *buffer;			/* FIXME: Deprecated */
+	void *user_data;		/* FIXME: used to be *data. Deprecated this allready */

 	int tag;
 	int errors;
@@ -303,7 +304,6 @@ struct request {

 	unsigned int data_len;
 	unsigned int sense_len;
-	void *data;
 	void *sense;

 	unsigned int timeout;




-
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