* CDROM_SEND_PACKET oddity @ 2003-09-27 3:33 Derek Foreman 2003-09-27 11:47 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Derek Foreman @ 2003-09-27 3:33 UTC (permalink / raw) To: linux-kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-27 3:33 CDROM_SEND_PACKET oddity Derek Foreman @ 2003-09-27 11:47 ` Jens Axboe 2003-09-27 12:27 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2003-09-27 11:47 UTC (permalink / raw) To: Derek Foreman; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-27 11:47 ` Jens Axboe @ 2003-09-27 12:27 ` Jens Axboe 2003-09-27 17:54 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2003-09-27 12:27 UTC (permalink / raw) To: Derek Foreman; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-27 12:27 ` Jens Axboe @ 2003-09-27 17:54 ` Jens Axboe 2003-09-27 18:11 ` Derek Foreman 2003-09-28 3:50 ` Derek Foreman 0 siblings, 2 replies; 10+ messages in thread From: Jens Axboe @ 2003-09-27 17:54 UTC (permalink / raw) To: Derek Foreman; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-27 17:54 ` Jens Axboe @ 2003-09-27 18:11 ` Derek Foreman 2003-09-28 3:50 ` Derek Foreman 1 sibling, 0 replies; 10+ messages in thread From: Derek Foreman @ 2003-09-27 18:11 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-kernel 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-27 17:54 ` Jens Axboe 2003-09-27 18:11 ` Derek Foreman @ 2003-09-28 3:50 ` Derek Foreman 2003-09-28 8:51 ` Jens Axboe 1 sibling, 1 reply; 10+ messages in thread From: Derek Foreman @ 2003-09-28 3:50 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-kernel 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))) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-28 3:50 ` Derek Foreman @ 2003-09-28 8:51 ` Jens Axboe 2003-09-28 16:56 ` Derek Foreman 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2003-09-28 8:51 UTC (permalink / raw) To: Derek Foreman; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-28 8:51 ` Jens Axboe @ 2003-09-28 16:56 ` Derek Foreman 2003-09-28 18:14 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Derek Foreman @ 2003-09-28 16:56 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-kernel 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) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-28 16:56 ` Derek Foreman @ 2003-09-28 18:14 ` Jens Axboe 2003-09-28 21:02 ` Derek Foreman 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2003-09-28 18:14 UTC (permalink / raw) To: Derek Foreman; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CDROM_SEND_PACKET oddity 2003-09-28 18:14 ` Jens Axboe @ 2003-09-28 21:02 ` Derek Foreman 0 siblings, 0 replies; 10+ messages in thread From: Derek Foreman @ 2003-09-28 21:02 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2003-09-28 21:02 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-09-27 3:33 CDROM_SEND_PACKET oddity Derek Foreman 2003-09-27 11:47 ` Jens Axboe 2003-09-27 12:27 ` Jens Axboe 2003-09-27 17:54 ` Jens Axboe 2003-09-27 18:11 ` Derek Foreman 2003-09-28 3:50 ` Derek Foreman 2003-09-28 8:51 ` Jens Axboe 2003-09-28 16:56 ` Derek Foreman 2003-09-28 18:14 ` Jens Axboe 2003-09-28 21:02 ` Derek Foreman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).