Re: kernel panic / NULL pointer dereference

Re: kernel panic / NULL pointer dereference

[Bernd Schubert]

On 05/10/2012 05:45 PM, Bernd Schubert wrote:
> Hi all,
> 
> I'm just playing with an SRP connected NetApp system and just got an XFS 
> related kernel panic. I guess it is due to large IO (32MiB). At least it 
> just came up after enabling 32MiB device max_sectors.
> As the tests are running in a RHEL6 image and as I needed at least 
> 2.6.39 to get a large srp_tablsize with SRP, I simply installed the 
> lasted oracle uek kernel. If needed I'm going to update to a vanilla 
> version.
> 
> 
>> May 10 17:31:49 sgi01 kernel: XFS (sdb): Mounting Filesystem
>> May 10 17:31:49 sgi01 kernel: XFS (sdb): Ending clean mount
>> May 10 17:33:00 sgi01 kernel: BUG: unable to handle kernel NULL 
>> pointer dereference at (null)
>> May 10 17:33:00 sgi01 kernel: IP: [<ffffffffa07f5483>] 
>> xfs_alloc_ioend_bio+0x33/0x50 [xfs]

Oh, there is a bio allocation path to return NULL:

bvec_alloc_bs(gfp_mask, nr_iovecs, ) => NULL when nr_iovecs  > BIO_MAX_PAGES
bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
bio_alloc(GFP_NOIO, nvecs)
xfs_alloc_ioend_bio()

And nvecs/nr_iovecs is obtained by bio_get_nr_vecs(), which does not check for
BIO_MAX_PAGES. Of course, all of that only happens with large IO sizes, 
which is exactly what I'm doing.
As xfs_alloc_ioend_bio() is using GFP_NOIO it does not expect bio_alloc 
to fail, but as I'm trying to send large IOs I guess that is exactly what happens here.


>May 10 17:33:00 sgi01 kernel: [<ffffffffa07f561e>] xfs_submit_ioend+0xfe/0x110 [xfs]
>May 10 17:33:00 sgi01 kernel: [<ffffffffa07f696b>] xfs_vm_writepage+0x26b/0x510 [xfs]
>May 10 17:33:00 sgi01 kernel: [<ffffffff81112377>] __writepage+0x17/0x40
>May 10 17:33:00 sgi01 kernel: [<ffffffff81113696>] write_cache_pages+0x246/0x520
>May 10 17:33:00 sgi01 kernel: [<ffffffff81112360>] ? set_page_dirty+0x70/0x70
>May 10 17:33:00 sgi01 kernel: [<ffffffff811139c1>] generic_writepages+0x51/0x80
>May 10 17:33:00 sgi01 kernel: [<ffffffffa07f537d>] xfs_vm_writepages+0x5d/0x80 [xfs]
>May 10 17:33:00 sgi01 kernel: [<ffffffff81113a11>] do_writepages+0x21/0x40
>May 10 17:33:00 sgi01 kernel: [<ffffffff8118df2e>] writeback_single_inode+0x10e/0x270

[PATCH] bio allocation failure due to bio_get_nr_vecs()

[Bernd Schubert]

>>> May 10 17:31:49 sgi01 kernel: XFS (sdb): Mounting Filesystem
>>> May 10 17:31:49 sgi01 kernel: XFS (sdb): Ending clean mount
>>> May 10 17:33:00 sgi01 kernel: BUG: unable to handle kernel NULL
>>> pointer dereference at (null)
>>> May 10 17:33:00 sgi01 kernel: IP: [<ffffffffa07f5483>]
>>> xfs_alloc_ioend_bio+0x33/0x50 [xfs]
> 
> Oh, there is a bio allocation path to return NULL:
> 
> bvec_alloc_bs(gfp_mask, nr_iovecs, ) =>  NULL when nr_iovecs>  BIO_MAX_PAGES
> bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
> bio_alloc(GFP_NOIO, nvecs)
> xfs_alloc_ioend_bio()
> 
> And nvecs/nr_iovecs is obtained by bio_get_nr_vecs(), which does not check for
> BIO_MAX_PAGES. Of course, all of that only happens with large IO sizes,
> which is exactly what I'm doing.
> As xfs_alloc_ioend_bio() is using GFP_NOIO it does not expect bio_alloc
> to fail, but as I'm trying to send large IOs I guess that is exactly what happens here.

I see that Kent already fixed an overflow issue 
in commit 5abebfdd02450fa1349daacf242e70b3736581e3. But even with this commit, 
bio_get_nr_vecs() still only checks for queue_max_segments(). As we have a maximum 
of 2048 segments, that does not help much here.
After cherry-picking 5abebfdd02450fa1349daacf242e70b3736581e3 and applying the patch
below, I didn't run into panics / NULL pointer dereferences anymore.


bio: bio_get_nr_vecs() must not return more than BIO_MAX_PAGES

From: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>

The number of bio_get_nr_vecs() is passed down via bio_alloc() to
bvec_alloc_bs(), which fails the bio allocation if
nr_iovecs > BIO_MAX_PAGES. For the underlying caller this causes an
unexpected bio allocation failure.
Limiting to queue_max_segments() is not sufficient, as max_segments
also might be very large.

bvec_alloc_bs(gfp_mask, nr_iovecs, ) => NULL when nr_iovecs  > BIO_MAX_PAGES
bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
bio_alloc(GFP_NOIO, nvecs)
xfs_alloc_ioend_bio()


Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
---
 fs/bio.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/bio.c b/fs/bio.c
index e453924..84da885 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -505,9 +505,14 @@ EXPORT_SYMBOL(bio_clone);
 int bio_get_nr_vecs(struct block_device *bdev)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
-	return min_t(unsigned,
+	int nr_pages;
+
+	nr_pages = min_t(unsigned,
 		     queue_max_segments(q),
 		     queue_max_sectors(q) / (PAGE_SIZE >> 9) + 1);
+
+	return min_t(unsigned, nr_pages, BIO_MAX_PAGES);
+
 }
 EXPORT_SYMBOL(bio_get_nr_vecs);
 

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Jeff Moyer]

Bernd Schubert <bernd.schubert@itwm.fraunhofer.de> writes:

> diff --git a/fs/bio.c b/fs/bio.c
> index e453924..84da885 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -505,9 +505,14 @@ EXPORT_SYMBOL(bio_clone);
>  int bio_get_nr_vecs(struct block_device *bdev)
>  {
>  	struct request_queue *q = bdev_get_queue(bdev);
> -	return min_t(unsigned,
> +	int nr_pages;

Looks like a corrupt patch.

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Bernd Schubert]

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

On 05/11/2012 04:06 PM, Jeff Moyer wrote:
> Bernd Schubert<bernd.schubert@itwm.fraunhofer.de>  writes:
>
>> diff --git a/fs/bio.c b/fs/bio.c
>> index e453924..84da885 100644
>> --- a/fs/bio.c
>> +++ b/fs/bio.c
>> @@ -505,9 +505,14 @@ EXPORT_SYMBOL(bio_clone);
>>   int bio_get_nr_vecs(struct block_device *bdev)
>>   {
>>   	struct request_queue *q = bdev_get_queue(bdev);
>> -	return min_t(unsigned,
>> +	int nr_pages;
>
> Looks like a corrupt patch.

What do you actually mean? Issue by thunderbird? I just saved the mail 
in my sent folder and it looks? Just to be sure, patch attached.


Thanks,
Bernd

[-- Attachment #2: fix-bio-nrvec.patch --]
[-- Type: text/x-patch, Size: 1242 bytes --]

bio: bio_get_nr_vecs() must not return more than BIO_MAX_PAGES

From: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>

The number of bio_get_nr_vecs() is passed down via bio_alloc() to
bvec_alloc_bs(), which fails the bio allocation if
nr_iovecs > BIO_MAX_PAGES. For the underlying caller this causes an
unexpected bio allocation failure.
Limiting to queue_max_segments() is not sufficiet, as max_segments
also might be very large.

bvec_alloc_bs(gfp_mask, nr_iovecs, ) => NULL when nr_iovecs  > BIO_MAX_PAGES
bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
bio_alloc(GFP_NOIO, nvecs)
xfs_alloc_ioend_bio()


Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
---
 fs/bio.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/bio.c b/fs/bio.c
index e453924..84da885 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -505,9 +505,14 @@ EXPORT_SYMBOL(bio_clone);
 int bio_get_nr_vecs(struct block_device *bdev)
 {
 	struct request_queue *q = bdev_get_queue(bdev);
-	return min_t(unsigned,
+	int nr_pages;
+
+	nr_pages = min_t(unsigned,
 		     queue_max_segments(q),
 		     queue_max_sectors(q) / (PAGE_SIZE >> 9) + 1);
+
+	return min_t(unsigned, nr_pages, BIO_MAX_PAGES);
+
 }
 EXPORT_SYMBOL(bio_get_nr_vecs);
 

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Jens Axboe]

On 05/11/2012 04:06 PM, Jeff Moyer wrote:
> Bernd Schubert <bernd.schubert@itwm.fraunhofer.de> writes:
> 
>> diff --git a/fs/bio.c b/fs/bio.c
>> index e453924..84da885 100644
>> --- a/fs/bio.c
>> +++ b/fs/bio.c
>> @@ -505,9 +505,14 @@ EXPORT_SYMBOL(bio_clone);
>>  int bio_get_nr_vecs(struct block_device *bdev)
>>  {
>>  	struct request_queue *q = bdev_get_queue(bdev);
>> -	return min_t(unsigned,
>> +	int nr_pages;
> 
> Looks like a corrupt patch.

It's fine, I think you are misreading the added and removed lines :-)

-- 
Jens Axboe

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Jens Axboe]

On 05/11/2012 03:49 PM, Bernd Schubert wrote:
> The number of bio_get_nr_vecs() is passed down via bio_alloc() to
> bvec_alloc_bs(), which fails the bio allocation if
> nr_iovecs > BIO_MAX_PAGES. For the underlying caller this causes an
> unexpected bio allocation failure.
> Limiting to queue_max_segments() is not sufficient, as max_segments
> also might be very large.
> 
> bvec_alloc_bs(gfp_mask, nr_iovecs, ) => NULL when nr_iovecs  > BIO_MAX_PAGES
> bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
> bio_alloc(GFP_NOIO, nvecs)
> xfs_alloc_ioend_bio()

Thanks, looks sane. Applied.

-- 
Jens Axboe

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Bernd Schubert]

On 05/11/2012 04:36 PM, Jens Axboe wrote:
> On 05/11/2012 03:49 PM, Bernd Schubert wrote:
>> The number of bio_get_nr_vecs() is passed down via bio_alloc() to
>> bvec_alloc_bs(), which fails the bio allocation if
>> nr_iovecs>  BIO_MAX_PAGES. For the underlying caller this causes an
>> unexpected bio allocation failure.
>> Limiting to queue_max_segments() is not sufficient, as max_segments
>> also might be very large.
>>
>> bvec_alloc_bs(gfp_mask, nr_iovecs, ) =>  NULL when nr_iovecs>  BIO_MAX_PAGES
>> bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
>> bio_alloc(GFP_NOIO, nvecs)
>> xfs_alloc_ioend_bio()
>
> Thanks, looks sane. Applied.
>

Great, thanks! Should we CC linux-stable for commit 
5abebfdd02450fa1349daacf242e70b3736581e3 and this one, as I got a hard 
kernel panic?


Thanks,
Bernd

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Jens Axboe]

On 05/11/2012 04:44 PM, Bernd Schubert wrote:
> On 05/11/2012 04:36 PM, Jens Axboe wrote:
>> On 05/11/2012 03:49 PM, Bernd Schubert wrote:
>>> The number of bio_get_nr_vecs() is passed down via bio_alloc() to
>>> bvec_alloc_bs(), which fails the bio allocation if
>>> nr_iovecs>  BIO_MAX_PAGES. For the underlying caller this causes an
>>> unexpected bio allocation failure.
>>> Limiting to queue_max_segments() is not sufficient, as max_segments
>>> also might be very large.
>>>
>>> bvec_alloc_bs(gfp_mask, nr_iovecs, ) =>  NULL when nr_iovecs>  BIO_MAX_PAGES
>>> bio_alloc_bioset(gfp_mask, nr_iovecs, ...)
>>> bio_alloc(GFP_NOIO, nvecs)
>>> xfs_alloc_ioend_bio()
>>
>> Thanks, looks sane. Applied.
>>
> 
> Great, thanks! Should we CC linux-stable for commit
> 5abebfdd02450fa1349daacf242e70b3736581e3 and this one, as I got a hard
> kernel panic?

Yes, that's a good idea. I've ammended the commit now to include stable.

-- 
Jens Axboe

Re: [PATCH] bio allocation failure due to bio_get_nr_vecs()

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 05/11/2012 04:06 PM, Jeff Moyer wrote:
>> Bernd Schubert <bernd.schubert@itwm.fraunhofer.de> writes:
>> 
>>> diff --git a/fs/bio.c b/fs/bio.c
>>> index e453924..84da885 100644
>>> --- a/fs/bio.c
>>> +++ b/fs/bio.c
>>> @@ -505,9 +505,14 @@ EXPORT_SYMBOL(bio_clone);
>>>  int bio_get_nr_vecs(struct block_device *bdev)
>>>  {
>>>  	struct request_queue *q = bdev_get_queue(bdev);
>>> -	return min_t(unsigned,
>>> +	int nr_pages;
>> 
>> Looks like a corrupt patch.
>
> It's fine, I think you are misreading the added and removed lines :-)

Whoops, sorry!