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

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

[FUJITA Tomonori]

Here is another ips patch, but not a bug fix.

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
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 <fujita.tomonori@lab.ntt.co.jp>
Cc: Salyzyn, Mark <Mark_Salyzyn@adaptec.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 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

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:tomof@acm.org]
> Sent: Tuesday, February 19, 2008 4:42 AM
> To: James.Bottomley@HansenPartnership.com
> Cc: linux-scsi@vger.kernel.org; Salyzyn, Mark;
> lnxninja@linux.vnet.ibm.com; fujita.tomonori@lab.ntt.co.jp
> 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 <fujita.tomonori@lab.ntt.co.jp>
> 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 <fujita.tomonori@lab.ntt.co.jp>
> Cc: Salyzyn, Mark <Mark_Salyzyn@adaptec.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  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;

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" <Mark_Salyzyn@adaptec.com> 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;
}