lkml.org 
[lkml]   [2003]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: CDROM_SEND_PACKET oddity
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

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

\
 
 \ /
  Last update: 2005-03-22 13:48    [W:0.045 / U:0.924 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site