RE: [PATCH] ips: sg chaining support to the path to non I/O commands

[FUJITA Tomonori]

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

RE: [PATCH] ips: sg chaining support to the path to non I/O commands

[Salyzyn, Mark]

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