fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[David Dillow]

When using devices that support max_segments > BIO_MAX_PAGES (256),
direct IO tries to allocate a bio with more pages than allowed, which
leads to an oops in dio_bio_alloc(). Clamp that request to the supported
maximum.

Signed-off-by: David Dillow <dillowda@ornl.gov>
--
dio_bio_alloc() doesn't check the result of bio_alloc(), so it
dereferences a NULL pointer. bio_alloc(GFP_KERNEL, ...) doesn't fail
unless it gets called for an invalid number of pages, so it seems a bit
like overkill to check for failure in dio_bio_alloc(), though it would
have saved me some time tracking this down.


diff --git a/fs/direct-io.c b/fs/direct-io.c
index 85882f6..9eb0553 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -583,6 +583,7 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
 		goto out;
 	sector = start_sector << (dio->blkbits - 9);
 	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
+	nr_pages = min(nr_pages, BIO_MAX_PAGES);
 	BUG_ON(nr_pages <= 0);
 	ret = dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
 	dio->boundary = 0;

Re: fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[Jeff Moyer]

David Dillow <dillowda@ornl.gov> writes:

> When using devices that support max_segments > BIO_MAX_PAGES (256),
> direct IO tries to allocate a bio with more pages than allowed, which
> leads to an oops in dio_bio_alloc(). Clamp that request to the supported
> maximum.
>
> Signed-off-by: David Dillow <dillowda@ornl.gov>
> --
> dio_bio_alloc() doesn't check the result of bio_alloc(), so it
> dereferences a NULL pointer. bio_alloc(GFP_KERNEL, ...) doesn't fail
> unless it gets called for an invalid number of pages, so it seems a bit
> like overkill to check for failure in dio_bio_alloc(), though it would
> have saved me some time tracking this down.

We should always be checking return values.  Mind respinning the patch
to include that?

> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 85882f6..9eb0553 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -583,6 +583,7 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
>  		goto out;
>  	sector = start_sector << (dio->blkbits - 9);
>  	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
> +	nr_pages = min(nr_pages, BIO_MAX_PAGES);

This looks fine.  Thanks for tracking it down!

Cheers,
Jeff

[PATCH v2 alt 1] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[David Dillow]

When using devices that support max_segments > BIO_MAX_PAGES (256),
direct IO tries to allocate a bio with more pages than allowed, which
leads to an oops in dio_bio_alloc(). Clamp the request to the supported
maximum, and return an error to catch future cases.
    
Signed-off-by: David Dillow <dillowda@ornl.gov>
--
On Fri, 2011-01-14 at 09:44 -0500, Jeff Moyer wrote:
> David Dillow <dillowda@ornl.gov> writes:
> > dio_bio_alloc() doesn't check the result of bio_alloc(), so it
> > dereferences a NULL pointer. bio_alloc(GFP_KERNEL, ...) doesn't fail
> > unless it gets called for an invalid number of pages, so it seems a bit
> > like overkill to check for failure in dio_bio_alloc(), though it would
> > have saved me some time tracking this down.
> 
> We should always be checking return values.  Mind respinning the patch
> to include that?

Here's the first alternate, that just checks the return value. I think I
prefer the other one, though.


diff --git a/fs/direct-io.c b/fs/direct-io.c
index 85882f6..9fdbf99 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -332,6 +332,8 @@ dio_bio_alloc(struct dio *dio, struct block_device *bdev,
 	struct bio *bio;
 
 	bio = bio_alloc(GFP_KERNEL, nr_vecs);
+	if (!bio)
+		return -ENOMEM;
 
 	bio->bi_bdev = bdev;
 	bio->bi_sector = first_sector;
@@ -583,6 +585,7 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
 		goto out;
 	sector = start_sector << (dio->blkbits - 9);
 	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
+	nr_pages = min(nr_pages, BIO_MAX_PAGES);
 	BUG_ON(nr_pages <= 0);
 	ret = dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
 	dio->boundary = 0;

[PATCH v2 alt 2] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[David Dillow]

When using devices that support max_segments > BIO_MAX_PAGES (256),
direct IO tries to allocate a bio with more pages than allowed, which
leads to an oops in dio_bio_alloc(). Clamp the request to the supported
maximum, and change dio_bio_alloc() to reflect that bio_alloc() will
always return a bio when called with __GFP_WAIT and a valid number of
vectors.
    
Signed-off-by: David Dillow <dillowda@ornl.gov>
--
Second alternate, my preferred that reflects that bio_alloc() cannot
fail when properly called. I can take or leave the comment.


diff --git a/fs/direct-io.c b/fs/direct-io.c
index 85882f6..96a01d8 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -325,13 +325,17 @@ void dio_end_io(struct bio *bio, int error)
 }
 EXPORT_SYMBOL_GPL(dio_end_io);
 
-static int
+static void
 dio_bio_alloc(struct dio *dio, struct block_device *bdev,
 		sector_t first_sector, int nr_vecs)
 {
 	struct bio *bio;
 
+	/* bio_alloc() is guaranteed to return a bio when called with
+	 * __GFP_WAIT and we request a valid number of vectors.
+	 */
 	bio = bio_alloc(GFP_KERNEL, nr_vecs);
+	BUG_ON(!bio);
 
 	bio->bi_bdev = bdev;
 	bio->bi_sector = first_sector;
@@ -342,7 +346,6 @@ dio_bio_alloc(struct dio *dio, struct block_device *bdev,
 
 	dio->bio = bio;
 	dio->logical_offset_in_bio = dio->cur_page_fs_offset;
-	return 0;
 }
 
 /*
@@ -583,8 +586,9 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
 		goto out;
 	sector = start_sector << (dio->blkbits - 9);
 	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
+	nr_pages = min(nr_pages, BIO_MAX_PAGES);
 	BUG_ON(nr_pages <= 0);
-	ret = dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
+	dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
 	dio->boundary = 0;
 out:
 	return ret;

Re: [PATCH v2 alt 2] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[Jeff Moyer]

David Dillow <dillowda@ornl.gov> writes:

> When using devices that support max_segments > BIO_MAX_PAGES (256),
> direct IO tries to allocate a bio with more pages than allowed, which
> leads to an oops in dio_bio_alloc(). Clamp the request to the supported
> maximum, and change dio_bio_alloc() to reflect that bio_alloc() will
> always return a bio when called with __GFP_WAIT and a valid number of
> vectors.

I like this version better than the first, just clean up the comment to
conform to CodingStyle and you can add my

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Be sure to CC akpm on the next posting, as DIO changes usually go in
through his tree.

Cheers,
Jeff

> Signed-off-by: David Dillow <dillowda@ornl.gov>
> --
> Second alternate, my preferred that reflects that bio_alloc() cannot
> fail when properly called. I can take or leave the comment.
>
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 85882f6..96a01d8 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -325,13 +325,17 @@ void dio_end_io(struct bio *bio, int error)
>  }
>  EXPORT_SYMBOL_GPL(dio_end_io);
>  
> -static int
> +static void
>  dio_bio_alloc(struct dio *dio, struct block_device *bdev,
>  		sector_t first_sector, int nr_vecs)
>  {
>  	struct bio *bio;
>  
> +	/* bio_alloc() is guaranteed to return a bio when called with
> +	 * __GFP_WAIT and we request a valid number of vectors.
> +	 */
>  	bio = bio_alloc(GFP_KERNEL, nr_vecs);
> +	BUG_ON(!bio);
>  
>  	bio->bi_bdev = bdev;
>  	bio->bi_sector = first_sector;
> @@ -342,7 +346,6 @@ dio_bio_alloc(struct dio *dio, struct block_device *bdev,
>  
>  	dio->bio = bio;
>  	dio->logical_offset_in_bio = dio->cur_page_fs_offset;
> -	return 0;
>  }
>  
>  /*
> @@ -583,8 +586,9 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
>  		goto out;
>  	sector = start_sector << (dio->blkbits - 9);
>  	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
> +	nr_pages = min(nr_pages, BIO_MAX_PAGES);
>  	BUG_ON(nr_pages <= 0);
> -	ret = dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
> +	dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
>  	dio->boundary = 0;
>  out:
>  	return ret;

[PATCH v3] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[David Dillow]

When using devices that support max_segments > BIO_MAX_PAGES (256),
direct IO tries to allocate a bio with more pages than allowed, which
leads to an oops in dio_bio_alloc(). Clamp the request to the supported
maximum, and change dio_bio_alloc() to reflect that bio_alloc() will
always return a bio when called with __GFP_WAIT and a valid number of
vectors.

Signed-off-by: David Dillow <dillowda@ornl.gov>
Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
Cc: stable@kernel.org
--
Also added cc to stable, as this has been a longstanding item.

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 85882f6..0df6597 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -325,13 +325,18 @@ void dio_end_io(struct bio *bio, int error)
 }
 EXPORT_SYMBOL_GPL(dio_end_io);
 
-static int
+static void
 dio_bio_alloc(struct dio *dio, struct block_device *bdev,
 		sector_t first_sector, int nr_vecs)
 {
 	struct bio *bio;
 
+	/*
+	 * bio_alloc() is guaranteed to return a bio when called with
+	 * __GFP_WAIT and we request a valid number of vectors.
+	 */
 	bio = bio_alloc(GFP_KERNEL, nr_vecs);
+	BUG_ON(!bio);
 
 	bio->bi_bdev = bdev;
 	bio->bi_sector = first_sector;
@@ -342,7 +347,6 @@ dio_bio_alloc(struct dio *dio, struct block_device *bdev,
 
 	dio->bio = bio;
 	dio->logical_offset_in_bio = dio->cur_page_fs_offset;
-	return 0;
 }
 
 /*
@@ -583,8 +587,9 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
 		goto out;
 	sector = start_sector << (dio->blkbits - 9);
 	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
+	nr_pages = min(nr_pages, BIO_MAX_PAGES);
 	BUG_ON(nr_pages <= 0);
-	ret = dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
+	dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
 	dio->boundary = 0;
 out:
 	return ret;

Re: [PATCH v3] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[Andrew Morton]

On Mon, 17 Jan 2011 22:00:27 -0500
David Dillow <dillowda@ornl.gov> wrote:

> When using devices that support max_segments > BIO_MAX_PAGES (256),
> direct IO tries to allocate a bio with more pages than allowed, which
> leads to an oops in dio_bio_alloc(). Clamp the request to the supported
> maximum, and change dio_bio_alloc() to reflect that bio_alloc() will
> always return a bio when called with __GFP_WAIT and a valid number of
> vectors.

Which device driver triggers this condition?

> Also added cc to stable, as this has been a longstanding item.

Well.  -stable only needs the patch if the driver which triggers the
problem is also there.  But we don't know what driver that is, yet???

> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -325,13 +325,18 @@ void dio_end_io(struct bio *bio, int error)
>  }
>  EXPORT_SYMBOL_GPL(dio_end_io);
>  
> -static int
> +static void
>  dio_bio_alloc(struct dio *dio, struct block_device *bdev,
>  		sector_t first_sector, int nr_vecs)
>  {
>  	struct bio *bio;
>  
> +	/*
> +	 * bio_alloc() is guaranteed to return a bio when called with
> +	 * __GFP_WAIT and we request a valid number of vectors.
> +	 */
>  	bio = bio_alloc(GFP_KERNEL, nr_vecs);
> +	BUG_ON(!bio);

This BUG_ON() is pretty pointless,

>  	bio->bi_bdev = bdev;

because the next statement will reliably oops, providing us with the
same information.


>  	bio->bi_sector = first_sector;
> @@ -342,7 +347,6 @@ dio_bio_alloc(struct dio *dio, struct block_device *bdev,
>  
>  	dio->bio = bio;
>  	dio->logical_offset_in_bio = dio->cur_page_fs_offset;
> -	return 0;
>  }
>  
>  /*
> @@ -583,8 +587,9 @@ static int dio_new_bio(struct dio *dio, sector_t start_sector)
>  		goto out;
>  	sector = start_sector << (dio->blkbits - 9);
>  	nr_pages = min(dio->pages_in_io, bio_get_nr_vecs(dio->map_bh.b_bdev));
> +	nr_pages = min(nr_pages, BIO_MAX_PAGES);
>  	BUG_ON(nr_pages <= 0);
> -	ret = dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
> +	dio_bio_alloc(dio, dio->map_bh.b_bdev, sector, nr_pages);
>  	dio->boundary = 0;
>  out:
>  	return ret;
> 

Re: [PATCH v3] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[David Dillow]

On Tue, 2011-01-18 at 16:53 -0800, Andrew Morton wrote:
> On Mon, 17 Jan 2011 22:00:27 -0500
> David Dillow <dillowda@ornl.gov> wrote:
> 
> > When using devices that support max_segments > BIO_MAX_PAGES (256),
> > direct IO tries to allocate a bio with more pages than allowed, which
> > leads to an oops in dio_bio_alloc(). Clamp the request to the supported
> > maximum, and change dio_bio_alloc() to reflect that bio_alloc() will
> > always return a bio when called with __GFP_WAIT and a valid number of
> > vectors.
> 
> Which device driver triggers this condition?

I found it with the changes I've been making to the SRP driver to
support much larger IOs; this is not upstream yet.

> > Also added cc to stable, as this has been a longstanding item.
> 
> Well.  -stable only needs the patch if the driver which triggers the
> problem is also there.  But we don't know what driver that is, yet???

Good point.

Any driver that supports a sg_tablesize of more than 256 has the
potential for problems. A quick search of the tree indicates that the
following set their initial sg_tablesize to SCSI_MAX_SG_CHAIN_SEGMENTS
(2048), though they may be saved by a separate bug I found -- and sent a
patch to the list -- that incorrectly causes that define to take the
value 128. 

./drivers/usb/storage/scsiglue.c
./drivers/scsi/arm/powertec.c
./drivers/scsi/arm/eesox.c
./drivers/scsi/arm/cumana_2.c
./drivers/ata/pata_icside.c

CCISS looks to pull the config from the hardware and has a case for more
than 512 entries, so maybe on that one. AACRAID has cases above 256, and
also seems to calculate the setting in some instances as well. FNIC uses
1024.

> > +	/*
> > +	 * bio_alloc() is guaranteed to return a bio when called with
> > +	 * __GFP_WAIT and we request a valid number of vectors.
> > +	 */
> >  	bio = bio_alloc(GFP_KERNEL, nr_vecs);
> > +	BUG_ON(!bio);
> 
> This BUG_ON() is pretty pointless,
> 
> >  	bio->bi_bdev = bdev;
> 
> because the next statement will reliably oops, providing us with the
> same information.

Fair enough, though I'd argue -- weakly -- that BUG_ON gives a pointer
to the source. I had to dig through the assembly and figure out that
this function was inlined, but I'd agree that it wasn't too hard and
it'd be nice to avoid the conditional on a hot path.

I'll reroll it, or you can drop the BUG_ON, either way is fine by me.
-- 
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Re: [PATCH v3] fs/direct-io.c: don't try to allocate more than BIO_MAX_PAGES in a bio

[Boaz Harrosh]

On 01/19/2011 04:34 AM, David Dillow wrote:
> On Tue, 2011-01-18 at 16:53 -0800, Andrew Morton wrote:
>>
>> Well.  -stable only needs the patch if the driver which triggers the
>> problem is also there.  But we don't know what driver that is, yet???
> 
> Good point.
> 
> Any driver that supports a sg_tablesize of more than 256 has the
> potential for problems. A quick search of the tree indicates that the
> following set their initial sg_tablesize to SCSI_MAX_SG_CHAIN_SEGMENTS
> (2048), though they may be saved by a separate bug I found -- and sent a
> patch to the list -- that incorrectly causes that define to take the
> value 128. 
> 
> ./drivers/usb/storage/scsiglue.c
> ./drivers/scsi/arm/powertec.c
> ./drivers/scsi/arm/eesox.c
> ./drivers/scsi/arm/cumana_2.c
> ./drivers/ata/pata_icside.c
> 
<snip>

iscsi has it on 2048 since ever

Boaz