Re: CDROM_SEND_PACKET oddity

[Jens Axboe <axboe@suse.de>]

On Sat, Sep 27 2003, Derek Foreman wrote:
> On Sat, 27 Sep 2003, Jens Axboe wrote:
> 
> > On Sat, Sep 27 2003, Jens Axboe wrote:
> > > On Sat, Sep 27 2003, Jens Axboe wrote:
> > > Actually, try this patch against current bk, it kills the
> > > CDROM_SEND_PACKET setup and use SG_IO internally instead. Should be much
> > > much better than what we have now. It's not tested here at all though,
> > > I'd appreciate it if you could give it a go.
> > 
> > This has a better chance of working. Changes:
> > 
> > - Don't export sg_io() anyways (leftover)
> > - Actually set ->cmdp and ->cmd_len
> > 
> > still untested.
> 
> [...]
> 
> > +			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> 
> This breaks because hdr.cmdp is a pointer, not an array.

Duh, yes.

> I think there has to be a hdr.mx_sb_len = sizeof(struct request_sense) in 
> there too?

Yes

> Also, this changes the semantics of CDROM_SEND_PACKET, currently if 
> the command fails, it returns EIO, after the patch it succeeds.

Ok, that needs to be fixed.

> how's this incremental patch?  It seems to work as I expect it to.
> 
> --- scsi_ioctl.c.orig	2003-09-27 22:45:40.708105384 -0500
> +++ scsi_ioctl.c	2003-09-27 22:46:23.490917249 -0500
> @@ -479,10 +479,14 @@
>  			hdr.dxferp = cgc.buffer;
>  			hdr.sbp = (char *) cgc.sense;
>  			hdr.timeout = cgc.timeout;
> -			memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
> +			hdr.cmdp = (unsigned char *)arg
> +			         + offsetof(struct cdrom_generic_command, cmd);

No that's buggy, arg is a user pointer. It needs to read:

			hdr.cmdp = cgc.cmd;

> +			hdr.mx_sb_len = sizeof(struct request_sense);
>  			hdr.cmd_len = sizeof(cgc.cmd);
>  			err = sg_io(q, bdev, &hdr);
>  
> +			if (hdr.status)
> +				err = -EIO;

Looks fine.

===== drivers/block/scsi_ioctl.c 1.34 vs edited =====
--- 1.34/drivers/block/scsi_ioctl.c	Fri Sep  5 12:22:31 2003
+++ edited/drivers/block/scsi_ioctl.c	Sun Sep 28 10:50:50 2003
@@ -25,6 +25,7 @@
 #include <linux/cdrom.h>
 #include <linux/slab.h>
 #include <linux/bio.h>
+#include <linux/times.h>
 #include <asm/uaccess.h>
 
 #include <scsi/scsi.h>
@@ -140,40 +141,36 @@
 }
 
 static int sg_io(request_queue_t *q, struct block_device *bdev,
-		 struct sg_io_hdr *uptr)
+		 struct sg_io_hdr *hdr)
 {
 	unsigned long start_time;
 	int reading, writing;
-	struct sg_io_hdr hdr;
 	struct request *rq;
 	struct bio *bio;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	void *buffer;
 
-	if (copy_from_user(&hdr, uptr, sizeof(*uptr)))
-		return -EFAULT;
-
-	if (hdr.interface_id != 'S')
+	if (hdr->interface_id != 'S')
 		return -EINVAL;
-	if (hdr.cmd_len > sizeof(rq->cmd))
+	if (hdr->cmd_len > sizeof(rq->cmd))
 		return -EINVAL;
 
 	/*
 	 * we'll do that later
 	 */
-	if (hdr.iovec_count)
+	if (hdr->iovec_count)
 		return -EOPNOTSUPP;
 
-	if (hdr.dxfer_len > (q->max_sectors << 9))
+	if (hdr->dxfer_len > (q->max_sectors << 9))
 		return -EIO;
 
 	reading = writing = 0;
 	buffer = NULL;
 	bio = NULL;
-	if (hdr.dxfer_len) {
-		unsigned int bytes = (hdr.dxfer_len + 511) & ~511;
+	if (hdr->dxfer_len) {
+		unsigned int bytes = (hdr->dxfer_len + 511) & ~511;
 
-		switch (hdr.dxfer_direction) {
+		switch (hdr->dxfer_direction) {
 		default:
 			return -EINVAL;
 		case SG_DXFER_TO_FROM_DEV:
@@ -191,8 +188,8 @@
 		 * first try to map it into a bio. reading from device will
 		 * be a write to vm.
 		 */
-		bio = bio_map_user(bdev, (unsigned long) hdr.dxferp,
-				   hdr.dxfer_len, reading);
+		bio = bio_map_user(bdev, (unsigned long) hdr->dxferp,
+				   hdr->dxfer_len, reading);
 
 		/*
 		 * if bio setup failed, fall back to slow approach
@@ -203,11 +200,11 @@
 				return -ENOMEM;
 
 			if (writing) {
-				if (copy_from_user(buffer, hdr.dxferp,
-						   hdr.dxfer_len))
+				if (copy_from_user(buffer, hdr->dxferp,
+						   hdr->dxfer_len))
 					goto out_buffer;
 			} else
-				memset(buffer, 0, hdr.dxfer_len);
+				memset(buffer, 0, hdr->dxfer_len);
 		}
 	}
 
@@ -216,11 +213,11 @@
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr.cmd_len;
-	if (copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len))
+	rq->cmd_len = hdr->cmd_len;
+	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
 		goto out_request;
-	if (sizeof(rq->cmd) != hdr.cmd_len)
-		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
+	if (sizeof(rq->cmd) != hdr->cmd_len)
+		memset(rq->cmd + hdr->cmd_len, 0, sizeof(rq->cmd) - hdr->cmd_len);
 
 	memset(sense, 0, sizeof(sense));
 	rq->sense = sense;
@@ -234,9 +231,9 @@
 		blk_rq_bio_prep(q, rq, bio);
 
 	rq->data = buffer;
-	rq->data_len = hdr.dxfer_len;
+	rq->data_len = hdr->dxfer_len;
 
-	rq->timeout = (hdr.timeout * HZ) / 1000;
+	rq->timeout = (hdr->timeout * HZ) / 1000;
 	if (!rq->timeout)
 		rq->timeout = q->sg_timeout;
 	if (!rq->timeout)
@@ -254,33 +251,30 @@
 		bio_unmap_user(bio, reading);
 
 	/* write to all output members */
-	hdr.status = rq->errors;	
-	hdr.masked_status = (hdr.status >> 1) & 0x1f;
-	hdr.msg_status = 0;
-	hdr.host_status = 0;
-	hdr.driver_status = 0;
-	hdr.info = 0;
-	if (hdr.masked_status || hdr.host_status || hdr.driver_status)
-		hdr.info |= SG_INFO_CHECK;
-	hdr.resid = rq->data_len;
-	hdr.duration = ((jiffies - start_time) * 1000) / HZ;
-	hdr.sb_len_wr = 0;
+	hdr->status = rq->errors;	
+	hdr->masked_status = (hdr->status >> 1) & 0x1f;
+	hdr->msg_status = 0;
+	hdr->host_status = 0;
+	hdr->driver_status = 0;
+	hdr->info = 0;
+	if (hdr->masked_status || hdr->host_status || hdr->driver_status)
+		hdr->info |= SG_INFO_CHECK;
+	hdr->resid = rq->data_len;
+	hdr->duration = ((jiffies - start_time) * 1000) / HZ;
+	hdr->sb_len_wr = 0;
 
-	if (rq->sense_len && hdr.sbp) {
-		int len = min((unsigned int) hdr.mx_sb_len, rq->sense_len);
+	if (rq->sense_len && hdr->sbp) {
+		int len = min((unsigned int) hdr->mx_sb_len, rq->sense_len);
 
-		if (!copy_to_user(hdr.sbp, rq->sense, len))
-			hdr.sb_len_wr = len;
+		if (!copy_to_user(hdr->sbp, rq->sense, len))
+			hdr->sb_len_wr = len;
 	}
 
 	blk_put_request(rq);
 
-	if (copy_to_user(uptr, &hdr, sizeof(*uptr)))
-		goto out_buffer;
-
 	if (buffer) {
 		if (reading)
-			if (copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len))
+			if (copy_to_user(hdr->dxferp, buffer, hdr->dxfer_len))
 				goto out_buffer;
 
 		kfree(buffer);
@@ -437,9 +431,71 @@
 		case SG_EMULATED_HOST:
 			err = sg_emulated_host(q, (int *) arg);
 			break;
-		case SG_IO:
-			err = sg_io(q, bdev, (struct sg_io_hdr *) arg);
+		case SG_IO: {
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&hdr, (struct sg_io_hdr *) arg, sizeof(hdr))) {
+				err = -EFAULT;
+				break;
+			}
+			err = sg_io(q, bdev, &hdr);
+			if (copy_to_user((struct sg_io_hdr *) arg, &hdr, sizeof(hdr)))
+				err = -EFAULT;
+			break;
+		}
+		case CDROM_SEND_PACKET: {
+			struct cdrom_generic_command cgc;
+			struct sg_io_hdr hdr;
+
+			if (copy_from_user(&cgc, (struct cdrom_generic_command *) arg, sizeof(cgc))) {
+				err = -EFAULT;
+				break;
+			}
+			cgc.timeout = clock_t_to_jiffies(cgc.timeout);
+			memset(&hdr, 0, sizeof(hdr));
+			hdr.interface_id = 'S';
+			hdr.cmd_len = sizeof(cgc.cmd);
+			hdr.dxfer_len = cgc.buflen;
+			err = 0;
+			switch (cgc.data_direction) {
+				case CGC_DATA_UNKNOWN:
+					hdr.dxfer_direction = SG_DXFER_UNKNOWN;
+					break;
+				case CGC_DATA_WRITE:
+					hdr.dxfer_direction = SG_DXFER_TO_DEV;
+					break;
+				case CGC_DATA_READ:
+					hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+					break;
+				case CGC_DATA_NONE:
+					hdr.dxfer_direction = SG_DXFER_NONE;
+					break;
+				default:
+					err = -EINVAL;
+			}
+			if (err)
+				break;
+
+			hdr.dxferp = cgc.buffer;
+			hdr.sbp = (char *) cgc.sense;
+			if (hdr.sbp)
+				hdr.mx_sb_len = sizeof(struct request_sense);
+			hdr.timeout = cgc.timeout;
+			hdr.cmdp = cgc.cmd;
+			hdr.cmd_len = sizeof(cgc.cmd);
+			err = sg_io(q, bdev, &hdr);
+
+			if (hdr.status)
+				err = -EIO;
+
+			cgc.stat = err;
+			cgc.buflen = hdr.resid;
+			if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))
+				err = -EFAULT;
+
 			break;
+		}
+
 		/*
 		 * old junk scsi send command ioctl
 		 */
===== drivers/cdrom/cdrom.c 1.40 vs edited =====
--- 1.40/drivers/cdrom/cdrom.c	Tue Sep 23 21:18:06 2003
+++ edited/drivers/cdrom/cdrom.c	Sat Sep 27 14:24:59 2003
@@ -1856,57 +1856,6 @@
 	return cdo->generic_packet(cdi, &cgc);
 }
 
-static int cdrom_do_cmd(struct cdrom_device_info *cdi,
-			struct cdrom_generic_command *cgc)
-{
-	struct request_sense *usense, sense;
-	unsigned char *ubuf;
-	int ret;
-
-	if (cgc->data_direction == CGC_DATA_UNKNOWN)
-		return -EINVAL;
-
-	if (cgc->buflen < 0 || cgc->buflen >= 131072)
-		return -EINVAL;
-
-	usense = cgc->sense;
-	cgc->sense = &sense;
-	if (usense && !access_ok(VERIFY_WRITE, usense, sizeof(*usense))) {
-		return -EFAULT;
-	}
-
-	ubuf = cgc->buffer;
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		cgc->buffer = kmalloc(cgc->buflen, GFP_KERNEL);
-		if (cgc->buffer == NULL)
-			return -ENOMEM;
-	}
-
-
-	if (cgc->data_direction == CGC_DATA_READ) {
-		if (!access_ok(VERIFY_READ, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	} else if (cgc->data_direction == CGC_DATA_WRITE) {
-		if (copy_from_user(cgc->buffer, ubuf, cgc->buflen)) {
-			kfree(cgc->buffer);
-			return -EFAULT;
-		}
-	}
-
-	ret = cdi->ops->generic_packet(cdi, cgc);
-	__copy_to_user(usense, cgc->sense, sizeof(*usense));
-	if (!ret && cgc->data_direction == CGC_DATA_READ)
-		__copy_to_user(ubuf, cgc->buffer, cgc->buflen);
-	if (cgc->data_direction == CGC_DATA_READ ||
-	    cgc->data_direction == CGC_DATA_WRITE) {
-		kfree(cgc->buffer);
-	}
-	return ret;
-}
-
 static int mmc_ioctl(struct cdrom_device_info *cdi, unsigned int cmd,
 		     unsigned long arg)
 {		
@@ -2176,14 +2125,6 @@
 		return 0;
 		}
 
-	case CDROM_SEND_PACKET: {
-		if (!CDROM_CAN(CDC_GENERIC_PACKET))
-			return -ENOSYS;
-		cdinfo(CD_DO_IOCTL, "entering CDROM_SEND_PACKET\n"); 
-		IOCTL_IN(arg, struct cdrom_generic_command, cgc);
-		cgc.timeout = clock_t_to_jiffies(cgc.timeout);
-		return cdrom_do_cmd(cdi, &cgc);
-		}
 	case CDROM_NEXT_WRITABLE: {
 		long next = 0;
 		cdinfo(CD_DO_IOCTL, "entering CDROM_NEXT_WRITABLE\n"); 

-- 
Jens Axboe