scsi: properly count the number of pages in scsi_req_map_sg()

PATCH In review Dan Aloni 5 participants 11 messages Mar 21, 2006 – May 26, 2006 SCSI

Share & tools

Dan Aloni

author

2006-03-21 08:37 UTC #0 lore raw

PATCH 1/1 scsi: properly count the number of pages in scsi_req_map_sg()

Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.

For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.

Signed-off-by Dan Aloni <[email protected]>

---
commit 8faa94b01e6fd4518b760ce39a2db0ede9444ded
tree c2e3c6ee5f59a4c1e166e4798ddc6e938f448de2
parent c4a1745aa09fc110afdefea0e5d025043e348bae
author Dan Aloni <[email protected]> Tue, 21 Mar 2006 10:19:11 +0200 committer Dan Aloni <[email protected]> Tue, 21 Mar 2006 10:19:11 +0200

 drivers/scsi/scsi_lib.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

drivers/scsi/scsi_lib.c +8 -1

index 701a328..a42f3aa 100644
@@ -368,13 +368,20 @@ static int scsi_req_map_sg(struct reques
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
 
 	for (i = 0; i < nsegs; i++) {
+		off = sgl[i].offset;
+		len = sgl[i].length;
+
+		nr_pages += ((off + len + PAGE_SIZE - 1) >> PAGE_SHIFT) - (off >> PAGE_SHIFT);
+	}
+
+	for (i = 0; i < nsegs; i++) {
 		page = sgl[i].page;
 		off = sgl[i].offset;
 		len = sgl[i].length;

Signature · 2 lines

Dan Aloni
[email protected], [email protected], [email protected], [email protected]

James Bottomley

2006-03-21 15:54 UTC #1 lore raw

This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.

Quoted 6 lines · Dan Aloni

Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.

Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?

James

Dan Aloni

author

2006-03-21 16:18 UTC #2 lore raw

Quoted 11 lines · James Bottomley

This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 6 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.

Quoted 5 lines

Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?

These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).

Signature · 2 lines

Dan Aloni
[email protected], [email protected], [email protected], [email protected]

Bryan Holty

2006-03-21 18:05 UTC #3 lore raw

Quoted 18 lines · Dan Aloni

Quoted 11 lines · James Bottomley
This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 6 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.
Quoted 5 lines
Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?

Quoted 5 lines

These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).

Hello,
I am seeing the same issue when using direct io with sg. sg will perform
direct io on any date that is aligned with the devices dma_align. The default for drivers that do not specify is 512. sg builds the scatter gather list from the user specified location, offsetting the first entry in the list if not page aligned. This is the case that causes the improper allocation of "nr_iovec" in scsi_req_map_sgand the later slab corruption.

I don't think it is necessary to calculate nr_pages from the entire list.
Only sgl[0] is allowed to have an offset, so we can calculate from that as follows.

drivers/scsi/scsi_lib.c +5 -2

@@ -368,12 +368,15 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
-
+	
+	if (nsegs)
+ 		nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	
 	for (i = 0; i < nsegs; i++) {
 		page = sgl[i].page;
 		off = sgl[i].offset;

--
Bryan Holty

Mike Christie

2006-03-21 19:18 UTC #4 lore raw

Quoted 40 lines · Bryan Holty

Quoted 20 lines · Dan Aloni
Quoted 12 lines · James Bottomley
This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 6 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.
Quoted 5 lines
Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?
Quoted 5 lines
These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).
Hello,
I am seeing the same issue when using direct io with sg. sg will perform
direct io on any date that is aligned with the devices dma_align. The default for drivers that do not specify is 512. sg builds the scatter gather list from the user specified location, offsetting the first entry in the list if not page aligned. This is the case that causes the improper allocation of "nr_iovec" in scsi_req_map_sgand the later slab corruption.
I don't think it is necessary to calculate nr_pages from the entire list.
Only sgl[0] is allowed to have an offset, so we can calculate from that as follows.
drivers/scsi/scsi_lib.c +0 -0
@@ -368,12 +368,15 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {

drivers/scsi/scsi_lib.c line 377

Quoted 10 lines

drivers/scsi/scsi_lib.c +3 -2

 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
-
+	
+	if (nsegs)

you can drop that test

drivers/scsi/scsi_lib.c line 379

Quoted 2 lines

drivers/scsi/scsi_lib.c +2 -0

+ 		nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
+

I think we can do this without looping but I think this is broken. If we had a slight variant of Dan's example but we have a page and some change in the first entry {3644, 4548} and 0,60} in the last one, that would would only calculate two pages but we want three.

I think we can have to calculate the first and last entries but the middle ones we can assume have no offset and lengths that are multiples of a page.

Bryan Holty

2006-03-21 20:48 UTC #5 lore raw

Quoted 59 lines · Mike Christie

Quoted 42 lines · Bryan Holty
Quoted 18 lines · Dan Aloni
Quoted 11 lines · James Bottomley
This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 6 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.
Quoted 5 lines
Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?
Quoted 5 lines
These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).
Hello,
I am seeing the same issue when using direct io with sg. sg will
perform direct io on any date that is aligned with the devices dma_align.
The default for drivers that do not specify is 512. sg builds the
scatter gather list from the user specified location, offsetting the first entry in the list if not page aligned. This is the case that causes the improper allocation of "nr_iovec" in scsi_req_map_sgand the later slab corruption.
I don't think it is necessary to calculate nr_pages from the entire
list. Only sgl[0] is allowed to have an offset, so we can calculate from that as follows.
drivers/scsi/scsi_lib.c +0 -0
@@ -368,12 +368,15 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
drivers/scsi/scsi_lib.c line 377
Quoted 10 lines
drivers/scsi/scsi_lib.c +3 -2
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
-
+
+	if (nsegs)
you can drop that test
drivers/scsi/scsi_lib.c line 379
Quoted 2 lines
drivers/scsi/scsi_lib.c +2 -0
+ 		nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
+

Quoted 8 lines

I think we can do this without looping but I think this is broken. If we had a slight variant of Dan's example but we have a page and some change in the first entry {3644, 4548} and 0,60} in the last one, that would would only calculate two pages but we want three.
I think we can have to calculate the first and last entries but the middle ones we can assume have no offset and lengths that are multiples of a page.

You are absolutely correct.
bufflen is not page masked and when added to the offset of the first entry, will generate the correct nr_pages.

    nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
==
    nr_pages = ((bufflen & PAGE_MASK) + (PAGE_SIZE-1) + sgl[0].offset +

sgl[nsegs-1].length) >> PAGE_SHIFT;

Either will calculate correctly.

--
Bryan Holty

Bryan Holty

2006-03-22 12:35 UTC #6 lore raw

Quoted 80 lines · Bryan Holty

Quoted 59 lines · Mike Christie
Quoted 42 lines · Bryan Holty
Quoted 18 lines · Dan Aloni
Quoted 11 lines · James Bottomley
This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 6 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.
Quoted 5 lines
Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?
Quoted 5 lines
These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).
Hello,
I am seeing the same issue when using direct io with sg. sg will
perform direct io on any date that is aligned with the devices dma_align. The default for drivers that do not specify is 512. sg builds the scatter gather list from the user specified location, offsetting the first entry in the list if not page aligned. This is the case that causes the improper allocation of "nr_iovec" in scsi_req_map_sgand the later slab corruption.
I don't think it is necessary to calculate nr_pages from the entire
list. Only sgl[0] is allowed to have an offset, so we can calculate from that as follows.
drivers/scsi/scsi_lib.c +0 -0
@@ -368,12 +368,15 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
drivers/scsi/scsi_lib.c line 377
Quoted 10 lines
drivers/scsi/scsi_lib.c +3 -2
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
-
+
+	if (nsegs)
you can drop that test
drivers/scsi/scsi_lib.c line 379
Quoted 2 lines
drivers/scsi/scsi_lib.c +2 -0
+ 		nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
+
Quoted 8 lines
I think we can do this without looping but I think this is broken. If we had a slight variant of Dan's example but we have a page and some change in the first entry {3644, 4548} and 0,60} in the last one, that would would only calculate two pages but we want three.
I think we can have to calculate the first and last entries but the middle ones we can assume have no offset and lengths that are multiples of a page.
You are absolutely correct.
bufflen is not page masked and when added to the offset of the first entry, will generate the correct nr_pages.
    nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
==
    nr_pages = ((bufflen & PAGE_MASK) + (PAGE_SIZE-1) + sgl[0].offset +
sgl[nsegs-1].length) >> PAGE_SHIFT;
Either will calculate correctly.

Quoted 7 lines

--
Bryan Holty
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Based on above, I think the most intuitive fix would be the offset addition of the first entry to the initialization of nr_pages.

Without this change, for instance, with 4K io's every sg io that is dma_aligned for direct io, but not page aligned will cause slab corruption and an oops

I am able to run a number of tests with sg that cause the boundary to be crossed, and with this fix there is no slab corruption or data corruption.

Thanks Dan, I had been hunting for this for a couple of days!!

Thoughts??

Signed-off-by Bryan Holty <[email protected]>

--- a/drivers/scsi/scsi_lib.c 2006-03-03 13:17:22.000000000 -0600
+++ b/drivers/scsi/scsi_lib.c 2006-03-22 06:09:09.669599539 -0600
@@ -368,7 +368,7 @@
int nsegs, unsigned bufflen, gfp_t gfp)
{
struct request_queue *q = rq->q;
- int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+ int nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
unsigned int data_len = 0, len, bytes, off; struct page *page;
struct bio *bio = NULL;
--
Bryan Holty

Mike Christie

2006-05-26 06:13 UTC #7 lore raw

Quoted 102 lines · Bryan Holty

Quoted 74 lines · Bryan Holty
Quoted 54 lines · Mike Christie
Quoted 39 lines · Bryan Holty
Quoted 16 lines · Dan Aloni
Quoted 7 lines · James Bottomley
This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 3 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
Quoted 8 lines
Quoted 3 lines
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.
Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?
Quoted 5 lines
These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).
Hello,
I am seeing the same issue when using direct io with sg. sg will
perform direct io on any date that is aligned with the devices dma_align. The default for drivers that do not specify is 512. sg builds the scatter gather list from the user specified location, offsetting the first entry in the list if not page aligned. This is the case that causes the improper allocation of "nr_iovec" in scsi_req_map_sgand the later slab corruption.
I don't think it is necessary to calculate nr_pages from the entire
list. Only sgl[0] is allowed to have an offset, so we can calculate from that as follows.
drivers/scsi/scsi_lib.c +0 -0
@@ -368,12 +368,15 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
drivers/scsi/scsi_lib.c line 377
Quoted 10 lines
drivers/scsi/scsi_lib.c +3 -2
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
-
+
+	if (nsegs)
you can drop that test
drivers/scsi/scsi_lib.c line 379
Quoted 2 lines
drivers/scsi/scsi_lib.c +2 -0
+ 		nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
+
Quoted 8 lines
I think we can do this without looping but I think this is broken. If we had a slight variant of Dan's example but we have a page and some change in the first entry {3644, 4548} and 0,60} in the last one, that would would only calculate two pages but we want three.
I think we can have to calculate the first and last entries but the middle ones we can assume have no offset and lengths that are multiples of a page.
You are absolutely correct.
bufflen is not page masked and when added to the offset of the first entry, will generate the correct nr_pages.
    nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
==
    nr_pages = ((bufflen & PAGE_MASK) + (PAGE_SIZE-1) + sgl[0].offset +
sgl[nsegs-1].length) >> PAGE_SHIFT;
Either will calculate correctly.
Quoted 7 lines
--
Bryan Holty
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Based on above, I think the most intuitive fix would be the offset addition of the first entry to the initialization of nr_pages.
Without this change, for instance, with 4K io's every sg io that is dma_aligned for direct io, but not page aligned will cause slab corruption and an oops
I am able to run a number of tests with sg that cause the boundary to be crossed, and with this fix there is no slab corruption or data corruption.
Thanks Dan, I had been hunting for this for a couple of days!!
Thoughts??
Signed-off-by Bryan Holty <[email protected]>
--- a/drivers/scsi/scsi_lib.c 2006-03-03 13:17:22.000000000 -0600
+++ b/drivers/scsi/scsi_lib.c 2006-03-22 06:09:09.669599539 -0600

drivers/scsi/scsi_lib.c

Quoted 9 lines

drivers/scsi/scsi_lib.c +1 -2

@@ -368,7 +368,7 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
--

Was this patch ok? I was looking at some other problem and noticed this was not merged. The original calculation that I did in the code is wrong. I thought the calculation in this patch was correct, but I have showed that my math skills are lacking so I may be wrong.

Bryan Holty

2006-05-26 13:24 UTC #8 lore raw

Quoted 116 lines · Mike Christie

Quoted 115 lines · Bryan Holty
Quoted 80 lines · Bryan Holty
Quoted 59 lines · Mike Christie
Quoted 42 lines · Bryan Holty
Quoted 18 lines · Dan Aloni
Quoted 11 lines · James Bottomley
This is a good email to discuss on the scsi list:
[email protected]; whom I've added to the cc list.
Quoted 6 lines · Dan Aloni
Improper calculation of the number of pages causes bio_alloc() to be called with nr_iovecs=0, and slab corruption later.
For example, a simple scatterlist that fails: {(3644,452), (0, 60)}, (offset, size). bufflen=512 => nr_pages=1 => breakage. The proper page count for this example is 2.
Quoted 5 lines
Such a scatterlist would likely violate the device's underlying boundaries and is not legal ... there's supposed to be special code checking the queue alignment and copying the bio to an aligned buffer if the limits are violated. Where are you generating these scatterlists from?
Quoted 5 lines
These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).
Hello,
I am seeing the same issue when using direct io with sg. sg will
perform direct io on any date that is aligned with the devices dma_align. The default for drivers that do not specify is 512. sg builds the scatter gather list from the user specified location, offsetting the first entry in the list if not page aligned. This is the case that causes the improper allocation of "nr_iovec" in scsi_req_map_sgand the later slab corruption.
I don't think it is necessary to calculate nr_pages from the
entire list. Only sgl[0] is allowed to have an offset, so we can calculate from that as follows.
drivers/scsi/scsi_lib.c +0 -0
@@ -368,12 +368,15 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
drivers/scsi/scsi_lib.c line 377
Quoted 10 lines
drivers/scsi/scsi_lib.c +3 -2
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = 0;
 	unsigned int data_len = 0, len, bytes, off;
 	struct page *page;
 	struct bio *bio = NULL;
 	int i, err, nr_vecs = 0;
-
+
+	if (nsegs)
you can drop that test
Quoted 2 lines
drivers/scsi/scsi_lib.c +1 -0
+ 		nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >>
PAGE_SHIFT; +
Quoted 8 lines
I think we can do this without looping but I think this is broken. If we had a slight variant of Dan's example but we have a page and some change in the first entry {3644, 4548} and 0,60} in the last one, that would would only calculate two pages but we want three.
I think we can have to calculate the first and last entries but the middle ones we can assume have no offset and lengths that are multiples of a page.
You are absolutely correct.
bufflen is not page masked and when added to the offset of the first entry, will generate the correct nr_pages.
    nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
==
    nr_pages = ((bufflen & PAGE_MASK) + (PAGE_SIZE-1) + sgl[0].offset +
sgl[nsegs-1].length) >> PAGE_SHIFT;
Either will calculate correctly.
Quoted 7 lines
--
Bryan Holty
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Based on above, I think the most intuitive fix would be the offset addition of the first entry to the initialization of nr_pages.
Without this change, for instance, with 4K io's every sg io that is dma_aligned for direct io, but not page aligned will cause slab corruption and an oops
I am able to run a number of tests with sg that cause the boundary to be crossed, and with this fix there is no slab corruption or data corruption.
Thanks Dan, I had been hunting for this for a couple of days!!
Thoughts??
Signed-off-by Bryan Holty <[email protected]>
--- a/drivers/scsi/scsi_lib.c 2006-03-03 13:17:22.000000000 -0600
+++ b/drivers/scsi/scsi_lib.c 2006-03-22 06:09:09.669599539 -0600
drivers/scsi/scsi_lib.c +1 -1
@@ -368,7 +368,7 @@
 			   int nsegs, unsigned bufflen, gfp_t gfp)
 {
 	struct request_queue *q = rq->q;
-	int nr_pages = (bufflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	int nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	unsigned int data_len = 0, len, bytes, off;

Quoted 8 lines

drivers/scsi/scsi_lib.c
Quoted 3 lines
drivers/scsi/scsi_lib.c +0 -1
 	struct page *page;
 	struct bio *bio = NULL;
--
Was this patch ok? I was looking at some other problem and noticed this was not merged. The original calculation that I did in the code is wrong. I thought the calculation in this patch was correct, but I have showed that my math skills are lacking so I may be wrong.

Yes, this patch fixes the problem. We have been running with it since it was originally posted without problem. We crash near immediately without the patch in place. We are using sg with direct_io heavily.

--
Thanks,
Bryan Holty

Christoph Hellwig

2006-03-23 14:52 UTC #9 lore raw

In reply to Dan Aloni

Quoted 5 lines · Dan Aloni

These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).

What kernel version did you reproduce this with? Since 2.6.16 sg should obey all request size/alingment limitations. If not that's a bug in scsi_execute_async and it's helpers and should be fixed there.

Bryan Holty

2006-03-23 16:51 UTC #10 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Dan Aloni
These scatterlists can be generated using the sg driver. Though I am actually running a customized version of the sg driver, it seems the conversion from a userspace array of sg_iovec_t to scatterlist stays the same and also applies to the original driver (see st_map_user_pages()).

Quoted 8 lines

What kernel version did you reproduce this with? Since 2.6.16 sg should obey all request size/alingment limitations. If not that's a bug in scsi_execute_async and it's helpers and should be fixed there. -
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

I am able to reproduce this with 2.6.16-rc5 - 2.6.16. There is a problem in scsi_req_map_sg which is called by scsi_execute_async.

Currently, scsi_req_map_sg assumes every sgl entry is page aligned. It will cause later slab corruption by under-allocating the number of bio entries if sgl[0].offset + sgl[last].length > PAGE_SIZE.

Dan pointed this out, and I have submitted a patch that I believe correctly fixes the issue. Just waiting for some feedback.

--
Bryan Holty