ips: sg chaining support to the path to non I/O commands
PATCH 1/1 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.
--- drivers/scsi/ips.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-)
drivers/scsi/ips.c +10 -8
@@ -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;
Signature · 1 line
1.5.3.7
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
Quoted 88 lines
-----Original Message-----
From: FUJITA Tomonori [mailto:[email protected]]
Sent: Tuesday, February 19, 2008 4:42 AM
To: [email protected]
Cc: [email protected]; Salyzyn, Mark; [email protected]; [email protected]
Subject: [PATCH] ips: sg chaining support to the path to non I/O commandsHere 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 commandsI overlooked
ips_scmd_buf_writeandips_scmd_buf_readwhen 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_tablesizeis 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(-)drivers/scsi/ips.c +3
-2@@ -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);drivers/scsi/ips.c +4
-3-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);drivers/scsi/ips.c +1
-1-kunmap_atomic (buffer - sg[i].offset,KM_IRQ0 ); +kunmap_atomic (buffer - sg->offset,KM_IRQ0 );local_irq_restore (flags); xfer_cnt += min_cnt;
Quoted 1 line · Mark
ACK
Thanks!
Quoted 8 lines
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/sgfor 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:
drivers/scsi/aacraid/linit.c +7 -0
@@ -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;
}