RE: [PATCH] ips: sg chaining support to the path to non I/O commands
On Tue, 19 Feb 2008 04:39:00 -0800 Salyzyn, Mark [EMAIL PROTECTED] wrote: ACK Thanks! Other RAID drivers (eg: aacraid) makes the assumption that commands in these paths (INQUIRY, READ CAPACITY, MODE SENSE etc spoofing) are single scatter gather elements and have yet to be bitten. I agree with Fujita-san about the practical unlikelihood. The fix does not incur any change in code overhead, so it is worth hardening! Can one create a request via /dev/sg for a buffer smaller than 256 and manage to create a multi-element scatter gather? I think that we can do post 2.6.24 since we relaxed the default alignment requirements (from 511 to 3). So a buffer more than 8 bytes can leads to multi-element scatter gathers though it rarely happens. Shortly I will submit the helper functions to copy data between sg list and a buffer. It can replace aac_internal_transfer but it's not for scsi-rc-fixes. If you worry that aac_internal_transfer can't handle multiple sg entries, you can do something like this, I think: diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c index ae5f74f..73fa7c2 100644 --- a/drivers/scsi/aacraid/linit.c +++ b/drivers/scsi/aacraid/linit.c @@ -455,6 +455,12 @@ static int aac_slave_configure(struct scsi_device *sdev) return 0; } +static int aac_slave_alloc(struct scsi_device *sdev) +{ + blk_queue_update_dma_alignment(sdev-request_queue, (512 - 1)); + return 0; +} + /** * aac_change_queue_depth - alter queue depths * @sdev: SCSI device we are considering @@ -1015,6 +1021,7 @@ static struct scsi_host_template aac_driver_template = { .queuecommand = aac_queuecommand, .bios_param = aac_biosparm, .shost_attrs= aac_attrs, + .slave_alloc= aac_slave_alloc, .slave_configure= aac_slave_configure, .change_queue_depth = aac_change_queue_depth, .sdev_attrs = aac_dev_attrs, = Here's a malicious version of sg_inq, which tries to create multiple sg entries: = #include stdio.h #include stdlib.h #include string.h #include malloc.h #include fcntl.h #include sys/ioctl.h #include sys/types.h #include sys/stat.h #include scsi/sg.h #define INQ_REPLY_LEN 96 #define INQ_CMD_LEN 6 int main(int argc, char **argv) { struct sg_io_hdr hdr; unsigned char scb[INQ_CMD_LEN] = {0x12, 0, 0, 0, INQ_REPLY_LEN, 0}; unsigned char sense[32]; void *buf; int offset = 4096 - 4; int fd, ret; buf = valloc(8192); memset(hdr, 0, sizeof(hdr)); hdr.interface_id = 'S'; hdr.cmd_len = sizeof(scb); hdr.mx_sb_len = sizeof(sense); hdr.dxfer_direction = SG_DXFER_FROM_DEV; hdr.dxfer_len = INQ_REPLY_LEN; hdr.dxferp = buf + offset; hdr.cmdp = scb; hdr.sbp = sense; hdr.flags |= SG_FLAG_DIRECT_IO; fd = open(argv[1], O_RDONLY); if (fd 0) { fprintf(stderr, fail to open %s\n, argv[1]); return fd; } ret = ioctl(fd, SG_IO, hdr); if (ret 0) { fprintf(stderr, fail to ioctl %d\n, ret); return ret; } return ret; } - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ips: sg chaining support to the path to non I/O commands
Here is another ips patch, but not a bug fix. = From: FUJITA Tomonori [EMAIL PROTECTED] Subject: [PATCH] ips: sg chaining support to the path to non I/O commands I overlooked ips_scmd_buf_write and ips_scmd_buf_read when I converted ips to use the data buffer accessors. ips is unlikely to use sg chaining (especially in this path) since a) this path is used only for non I/O commands (with little data transfer), b) ips's sg_tablesize is set to just 17. Thanks to Tim Pepper for testing this patch. Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED] Cc: Salyzyn, Mark [EMAIL PROTECTED] Cc: James Bottomley [EMAIL PROTECTED] --- drivers/scsi/ips.c | 18 ++ 1 files changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c index 7ed568f..e5467a4 100644 --- a/drivers/scsi/ips.c +++ b/drivers/scsi/ips.c @@ -3510,15 +3510,16 @@ ips_scmd_buf_write(struct scsi_cmnd *scmd, void *data, unsigned int count) struct scatterlist *sg = scsi_sglist(scmd); for (i = 0, xfer_cnt = 0; - (i scsi_sg_count(scmd)) (xfer_cnt count); i++) { -min_cnt = min(count - xfer_cnt, sg[i].length); + (i scsi_sg_count(scmd)) (xfer_cnt count); + i++, sg = sg_next(sg)) { +min_cnt = min(count - xfer_cnt, sg-length); /* kmap_atomic() ensures addressability of the data buffer.*/ /* local_irq_save() protects the KM_IRQ0 address slot. */ local_irq_save(flags); -buffer = kmap_atomic(sg_page(sg[i]), KM_IRQ0) + sg[i].offset; + buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg-offset; memcpy(buffer, cdata[xfer_cnt], min_cnt); -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); + kunmap_atomic(buffer - sg-offset, KM_IRQ0); local_irq_restore(flags); xfer_cnt += min_cnt; @@ -3543,15 +3544,16 @@ ips_scmd_buf_read(struct scsi_cmnd *scmd, void *data, unsigned int count) struct scatterlist *sg = scsi_sglist(scmd); for (i = 0, xfer_cnt = 0; - (i scsi_sg_count(scmd)) (xfer_cnt count); i++) { -min_cnt = min(count - xfer_cnt, sg[i].length); + (i scsi_sg_count(scmd)) (xfer_cnt count); + i++, sg = sg_next(sg)) { + min_cnt = min(count - xfer_cnt, sg-length); /* kmap_atomic() ensures addressability of the data buffer.*/ /* local_irq_save() protects the KM_IRQ0 address slot. */ local_irq_save(flags); -buffer = kmap_atomic(sg_page(sg[i]), KM_IRQ0) + sg[i].offset; + buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg-offset; memcpy(cdata[xfer_cnt], buffer, min_cnt); -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); + kunmap_atomic(buffer - sg-offset, KM_IRQ0); local_irq_restore(flags); xfer_cnt += min_cnt; -- 1.5.3.7 - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ips: sg chaining support to the path to non I/O commands
ACK Other RAID drivers (eg: aacraid) makes the assumption that commands in these paths (INQUIRY, READ CAPACITY, MODE SENSE etc spoofing) are single scatter gather elements and have yet to be bitten. I agree with Fujita-san about the practical unlikelihood. The fix does not incur any change in code overhead, so it is worth hardening! Can one create a request via /dev/sg for a buffer smaller than 256 and manage to create a multi-element scatter gather? Sincerely -- Mark Salyzyn -Original Message- From: FUJITA Tomonori [mailto:[EMAIL PROTECTED] Sent: Tuesday, February 19, 2008 4:42 AM To: [EMAIL PROTECTED] Cc: linux-scsi@vger.kernel.org; Salyzyn, Mark; [EMAIL PROTECTED]; [EMAIL PROTECTED] Subject: [PATCH] ips: sg chaining support to the path to non I/O commands Here is another ips patch, but not a bug fix. = From: FUJITA Tomonori [EMAIL PROTECTED] Subject: [PATCH] ips: sg chaining support to the path to non I/O commands I overlooked ips_scmd_buf_write and ips_scmd_buf_read when I converted ips to use the data buffer accessors. ips is unlikely to use sg chaining (especially in this path) since a) this path is used only for non I/O commands (with little data transfer), b) ips's sg_tablesize is set to just 17. Thanks to Tim Pepper for testing this patch. Signed-off-by: FUJITA Tomonori [EMAIL PROTECTED] Cc: Salyzyn, Mark [EMAIL PROTECTED] Cc: James Bottomley [EMAIL PROTECTED] --- drivers/scsi/ips.c | 18 ++ 1 files changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c index 7ed568f..e5467a4 100644 --- a/drivers/scsi/ips.c +++ b/drivers/scsi/ips.c @@ -3510,15 +3510,16 @@ ips_scmd_buf_write(struct scsi_cmnd *scmd, void *data, unsigned int count) struct scatterlist *sg = scsi_sglist(scmd); for (i = 0, xfer_cnt = 0; - (i scsi_sg_count(scmd)) (xfer_cnt count); i++) { -min_cnt = min(count - xfer_cnt, sg[i].length); + (i scsi_sg_count(scmd)) (xfer_cnt count); + i++, sg = sg_next(sg)) { +min_cnt = min(count - xfer_cnt, sg-length); /* kmap_atomic() ensures addressability of the data buffer.*/ /* local_irq_save() protects the KM_IRQ0 address slot. */ local_irq_save(flags); -buffer = kmap_atomic(sg_page(sg[i]), KM_IRQ0) + sg[i].offset; + buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg-offset; memcpy(buffer, cdata[xfer_cnt], min_cnt); -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); + kunmap_atomic(buffer - sg-offset, KM_IRQ0); local_irq_restore(flags); xfer_cnt += min_cnt; @@ -3543,15 +3544,16 @@ ips_scmd_buf_read(struct scsi_cmnd *scmd, void *data, unsigned int count) struct scatterlist *sg = scsi_sglist(scmd); for (i = 0, xfer_cnt = 0; - (i scsi_sg_count(scmd)) (xfer_cnt count); i++) { -min_cnt = min(count - xfer_cnt, sg[i].length); + (i scsi_sg_count(scmd)) (xfer_cnt count); + i++, sg = sg_next(sg)) { + min_cnt = min(count - xfer_cnt, sg-length); /* kmap_atomic() ensures addressability of the data buffer.*/ /* local_irq_save() protects the KM_IRQ0 address slot. */ local_irq_save(flags); -buffer = kmap_atomic(sg_page(sg[i]), KM_IRQ0) + sg[i].offset; + buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg-offset; memcpy(cdata[xfer_cnt], buffer, min_cnt); -kunmap_atomic(buffer - sg[i].offset, KM_IRQ0); + kunmap_atomic(buffer - sg-offset, KM_IRQ0); local_irq_restore(flags); xfer_cnt += min_cnt; - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html