[PATCH] [SCSI] sg.c: fixes to sg_build_indirect()

[PATCH] [SCSI] sg.c: fixes to sg_build_indirect()

[Luben Tuikov]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

The sg driver corrupts memory when the data buffer
size is 5493376 (0x53d280) bytes.  The reason for this
is the computation of the left over size given the
size of scatter-gather array and the size of the
scatter-gather element.  This patch fixes this.

Other fixes:
1. Eliminate scatter_elem_sz_prev, and use memory
   barriers to get its current value.  Reset it to PAGE_SIZE
   if the user has set it to something smaller.
2. Use roundup() defined in linux/kernel.h
3. Figure out if the buff_size will fit in the maximum
   table size. If not, give up early and print an
   informative message.
4. Greatly simplified allocation loop.
5. Use min() defined in linux/kernel.h.
6. The remaining size (rem_sz) is decremented not by ret_sz, but
   by sg->length = min(num, ret_sz), i.e. by the effective memory
   we've allocated.
7. schp->bufflen is set to buff_size, not blk_size, since
   this is the effective xfer size we want to xfer.
8. blk_size must equal exactly 0 iff all allocation succeeded.
  (this is the proof of the loop)

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
---
 drivers/scsi/sg.c |  108 +++++++++++++++++++++++++++-------------------------
 1 files changed, 56 insertions(+), 52 deletions(-)

Getting out of my hair some patches in my dev (gateway) tree.

    Luben

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 795371012-sg.txt --]
[-- Type: text/inline; name="sg.txt", Size: 4400 bytes --]

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 18fa739..f377295 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -95,11 +95,8 @@ static int def_reserved_size = -1;	/* pi
 static int sg_allow_dio = SG_ALLOW_DIO_DEF;
 
 static int scatter_elem_sz = SG_SCATTER_SZ;
-static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 
 #define SG_SECTOR_SZ 512
-#define SG_SECTOR_MSK (SG_SECTOR_SZ - 1)
-
 #define SG_DEV_ARR_LUMP 32	/* amount to over allocate sg_dev_arr by */
 
 static int sg_add(struct class_device *, struct class_interface *);
@@ -1560,10 +1557,8 @@ init_sg(void)
 {
 	int rc;
 
-	if (scatter_elem_sz < PAGE_SIZE) {
+	if (scatter_elem_sz < PAGE_SIZE)
 		scatter_elem_sz = PAGE_SIZE;
-		scatter_elem_sz_prev = scatter_elem_sz;
-	}
 	if (def_reserved_size >= 0)
 		sg_big_buff = def_reserved_size;
 	else
@@ -1831,62 +1826,71 @@ static int
 sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 {
 	struct scatterlist *sg;
-	int ret_sz = 0, k, rem_sz, num, mx_sc_elems;
-	int sg_tablesize = sfp->parentdp->sg_tablesize;
-	int blk_size = buff_size;
-	struct page *p = NULL;
-
-	if ((blk_size < 0) || (!sfp))
+	int max_tablesize = sfp->parentdp->sg_tablesize;
+	int sg_tablesize, blk_size, k;
+	int sc_elem_sz;
+	
+	if ((buff_size < 0) || (!sfp))
 		return -EFAULT;
-	if (0 == blk_size)
-		++blk_size;	/* don't know why */
-/* round request up to next highest SG_SECTOR_SZ byte boundary */
-	blk_size = (blk_size + SG_SECTOR_MSK) & (~SG_SECTOR_MSK);
-	SCSI_LOG_TIMEOUT(4, printk("sg_build_indirect: buff_size=%d, blk_size=%d\n",
-				   buff_size, blk_size));
-
-	/* N.B. ret_sz carried into this block ... */
-	mx_sc_elems = sg_build_sgat(schp, sfp, sg_tablesize);
-	if (mx_sc_elems < 0)
-		return mx_sc_elems;	/* most likely -ENOMEM */
-
-	num = scatter_elem_sz;
-	if (unlikely(num != scatter_elem_sz_prev)) {
-		if (num < PAGE_SIZE) {
-			scatter_elem_sz = PAGE_SIZE;
-			scatter_elem_sz_prev = PAGE_SIZE;
-		} else
-			scatter_elem_sz_prev = num;
+	if (0 == buff_size)
+		buff_size = 1;
+
+	if (scatter_elem_sz < PAGE_SIZE)
+		scatter_elem_sz = PAGE_SIZE;
+	sc_elem_sz = scatter_elem_sz;
+	mb();
+
+	blk_size = roundup(buff_size, SG_SECTOR_SZ);
+	SCSI_LOG_TIMEOUT(4, printk("sg_build_indirect: buff_size=%d, "
+				   "blk_size=%d\n", buff_size, blk_size));
+
+	sg_tablesize = (blk_size + sc_elem_sz - 1)/sc_elem_sz;
+	if (sg_tablesize > max_tablesize) {
+		printk(KERN_NOTICE "%s: sg_table too big (%d > %d) for "
+		       "round-up transfer size of %d bytes with %d bytes an "
+		       "sg element\n",
+		       __FUNCTION__, sg_tablesize, max_tablesize,
+		       blk_size, sc_elem_sz);
+		return -ENOMEM;
 	}
-	for (k = 0, sg = schp->buffer, rem_sz = blk_size;
-	     (rem_sz > 0) && (k < mx_sc_elems);
-	     ++k, rem_sz -= ret_sz, ++sg) {
+
+	/* Allocate the maximum table size, in case external memory
+	 * fragmentation stretches us over the nominal sg size for the
+	 * given blk_size and sc_elem_sz.
+	 */
+	sg_tablesize = sg_build_sgat(schp, sfp, max_tablesize);
+	if (sg_tablesize <= 0)
+		return -ENOMEM;
+
+	for (k = 0, sg = schp->buffer;
+	     0 < blk_size && k < sg_tablesize;
+	     ++k, ++sg) {
 		
-		num = (rem_sz > scatter_elem_sz_prev) ?
-		      scatter_elem_sz_prev : rem_sz;
-		p = sg_page_malloc(num, sfp->low_dma, &ret_sz);
-		if (!p)
+		int ret_sz;
+		int num = min(blk_size, sc_elem_sz);
+
+		sg->page = sg_page_malloc(num, sfp->low_dma, &ret_sz);
+		if (!sg->page)
 			return -ENOMEM;
 
-		if (num == scatter_elem_sz_prev) {
-			if (unlikely(ret_sz > scatter_elem_sz_prev)) {
-				scatter_elem_sz = ret_sz;
-				scatter_elem_sz_prev = ret_sz;
-			}
-		}
-		sg->page = p;
-		sg->length = ret_sz;
+		sg->length = min(num, ret_sz);
 
-		SCSI_LOG_TIMEOUT(5, printk("sg_build_build: k=%d, a=0x%p, len=%d\n",
-				  k, p, ret_sz));
-	}		/* end of for loop */
+		blk_size -= sg->length;
+
+		SCSI_LOG_TIMEOUT(5, printk("sg_build_build: k=%d, a=0x%p, "
+					   "len=%d\n",
+					   k, sg->page, sg->length));
+	}
 
 	schp->k_use_sg = k;
-	SCSI_LOG_TIMEOUT(5, printk("sg_build_indirect: k_use_sg=%d, rem_sz=%d\n", k, rem_sz));
+	SCSI_LOG_TIMEOUT(5, printk("sg_build_indirect: k_use_sg=%d, "
+				   "remained=%d\n", k, blk_size));
 
-	schp->bufflen = blk_size;
-	if (rem_sz > 0)	/* must have failed */
+	schp->bufflen = buff_size;
+	if (blk_size != 0) {
+		/* must have failed */
 		return -ENOMEM;
+	}
 
 	return 0;
 }
-- 
1.4.3.3.g8478

Re: [PATCH] [SCSI] sg.c: fixes to sg_build_indirect()

[Christoph Hellwig]

On Sat, Nov 04, 2006 at 11:48:42AM -0800, Luben Tuikov wrote:
> The sg driver corrupts memory when the data buffer
> size is 5493376 (0x53d280) bytes.  The reason for this
> is the computation of the left over size given the
> size of scatter-gather array and the size of the
> scatter-gather element.  This patch fixes this.
> 
> Other fixes:
> 1. Eliminate scatter_elem_sz_prev, and use memory
>    barriers to get its current value.  Reset it to PAGE_SIZE
>    if the user has set it to something smaller.

I just stumbled over the scatter_elem_sz / scatter_elem_sz_prev
usage aswell when reasearching something unrelated in sg.
It seems very wrong to me to have unprotectred globals for buffer
size estimations.  If these estimates make sense we should do 
them per open file descriptor to avoid the races and to make sure
they still work properly in cases where multiple I/Os are going on
through sg.

Anyway, your patch looks very good and is an improvement in that area.