The example code from
http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
Does not behave as expected on my 2.6.0-test5 system. While the command
seems to be successfully sent - 2 of my drives report it as an invalid
opcode - for the other 2 drives, the buffer comes back all zeros.
(actually, the buffer's contents will remain in whatever state they're in
before the ioctl is called)
Sending the same command to those 2 drives with SG_IO results in the
expected behaviour.
On Fri, Sep 26 2003, Derek Foreman wrote:
> The example code from
> http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
>
> Does not behave as expected on my 2.6.0-test5 system. While the command
> seems to be successfully sent - 2 of my drives report it as an invalid
> opcode - for the other 2 drives, the buffer comes back all zeros.
> (actually, the buffer's contents will remain in whatever state they're in
> before the ioctl is called)
>
> Sending the same command to those 2 drives with SG_IO results in the
> expected behaviour.
Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
However, cd_poll should be rewritten to use SG_IO. Pretty trivial
exercise.
--
Jens Axboe
On Sat, Sep 27 2003, Jens Axboe wrote:
> On Fri, Sep 26 2003, Derek Foreman wrote:
> > The example code from
> > http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> >
> > Does not behave as expected on my 2.6.0-test5 system. While the command
> > seems to be successfully sent - 2 of my drives report it as an invalid
> > opcode - for the other 2 drives, the buffer comes back all zeros.
> > (actually, the buffer's contents will remain in whatever state they're in
> > before the ioctl is called)
> >
> > Sending the same command to those 2 drives with SG_IO results in the
> > expected behaviour.
>
> Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
>
> However, cd_poll should be rewritten to use SG_IO. Pretty trivial
> exercise.
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.
===== 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 Sat Sep 27 14:24:42 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>
@@ -139,41 +140,36 @@
return put_user(1, p);
}
-static int sg_io(request_queue_t *q, struct block_device *bdev,
- struct sg_io_hdr *uptr)
+int sg_io(request_queue_t *q, struct block_device *bdev, 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 +187,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 +199,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 +212,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 +230,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 +250,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 +430,64 @@
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;
+ hdr.timeout = cgc.timeout;
+ err = sg_io(q, bdev, &hdr);
+
+ 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
*/
@@ -475,3 +523,4 @@
EXPORT_SYMBOL(scsi_cmd_ioctl);
EXPORT_SYMBOL(scsi_command_size);
+EXPORT_SYMBOL(sg_io);
===== 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
On Sat, Sep 27 2003, Jens Axboe wrote:
> On Sat, Sep 27 2003, Jens Axboe wrote:
> > On Fri, Sep 26 2003, Derek Foreman wrote:
> > > The example code from
> > > http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> > >
> > > Does not behave as expected on my 2.6.0-test5 system. While the command
> > > seems to be successfully sent - 2 of my drives report it as an invalid
> > > opcode - for the other 2 drives, the buffer comes back all zeros.
> > > (actually, the buffer's contents will remain in whatever state they're in
> > > before the ioctl is called)
> > >
> > > Sending the same command to those 2 drives with SG_IO results in the
> > > expected behaviour.
> >
> > Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
> >
> > However, cd_poll should be rewritten to use SG_IO. Pretty trivial
> > exercise.
>
> 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.
===== 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 Sat Sep 27 19:53:10 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,66 @@
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;
+ hdr.timeout = cgc.timeout;
+ memcpy(hdr.cmdp, cgc.cmd, sizeof(cgc.cmd));
+ hdr.cmd_len = sizeof(cgc.cmd);
+ err = sg_io(q, bdev, &hdr);
+
+ 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
On Sat, 27 Sep 2003, Jens Axboe wrote:
> On Sat, Sep 27 2003, Jens Axboe wrote:
> > On Sat, Sep 27 2003, Jens Axboe wrote:
> > > On Fri, Sep 26 2003, Derek Foreman wrote:
> > > > The example code from
> > > > http://www.ussg.iu.edu/hypermail/linux/kernel/0202.0/att-0603/01-cd_poll.c
> > > >
> > > > Does not behave as expected on my 2.6.0-test5 system. While the command
> > > > seems to be successfully sent - 2 of my drives report it as an invalid
> > > > opcode - for the other 2 drives, the buffer comes back all zeros.
> > > > (actually, the buffer's contents will remain in whatever state they're in
> > > > before the ioctl is called)
> > > >
> > > > Sending the same command to those 2 drives with SG_IO results in the
> > > > expected behaviour.
> > >
> > > Can you try current -bk? It has some fixes for CDROM_SEND_PACKET.
> > >
> > > However, cd_poll should be rewritten to use SG_IO. Pretty trivial
> > > exercise.
> >
> > 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.
The old one worked after I fixed it up to set cmdp and the sense buffer.
I'll test this one out a little later today.
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.
I think there has to be a hdr.mx_sb_len = sizeof(struct request_sense) in
there too?
Also, this changes the semantics of CDROM_SEND_PACKET, currently if
the command fails, it returns EIO, after the patch it succeeds.
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);
+ 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;
cgc.stat = err;
cgc.buflen = hdr.resid;
if (copy_to_user((struct cdrom_generic_command *) arg, &cgc, sizeof(cgc)))
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
On Sun, 28 Sep 2003, Jens Axboe wrote:
> On Sat, Sep 27 2003, Derek Foreman wrote:
> > On Sat, 27 Sep 2003, Jens Axboe wrote:
> >
> > - 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;
Actually, hdr.cmdp is expected to be a user pointer. in sg_io we do
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)
On Sun, Sep 28 2003, Derek Foreman wrote:
> On Sun, 28 Sep 2003, Jens Axboe wrote:
>
> > On Sat, Sep 27 2003, Derek Foreman wrote:
> > > On Sat, 27 Sep 2003, Jens Axboe wrote:
> > >
> > > - 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;
>
> Actually, hdr.cmdp is expected to be a user pointer. in sg_io we do
>
> 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)
Ah you are right, I'd rather kill that too then like I removed user
sg_io_hdr pointer as well. It makes for less error handling in sg_io().
===== 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 20:12:44 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,10 @@
/*
* fill in request structure
*/
- 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);
+ rq->cmd_len = hdr->cmd_len;
+ memcpy(rq->cmd, hdr->cmdp, 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 +230,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 +250,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);
@@ -289,8 +282,6 @@
/* may not have succeeded, but output values written to control
* structure (struct sg_io_hdr). */
return 0;
-out_request:
- blk_put_request(rq);
out_buffer:
kfree(buffer);
return -EFAULT;
@@ -437,9 +428,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
On Sun, 28 Sep 2003, Jens Axboe wrote:
> On Sun, Sep 28 2003, Derek Foreman wrote:
> > On Sun, 28 Sep 2003, Jens Axboe wrote:
> >
> > > On Sat, Sep 27 2003, Derek Foreman wrote:
> > > > On Sat, 27 Sep 2003, Jens Axboe wrote:
> > > >
> > > > - 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;
> >
> > Actually, hdr.cmdp is expected to be a user pointer. in sg_io we do
> >
> > 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)
>
> Ah you are right, I'd rather kill that too then like I removed user
> sg_io_hdr pointer as well. It makes for less error handling in sg_io().
>
That one works great, thanks