[patch] block: fix infinite loop in __getblk_slow

[patch] block: fix infinite loop in __getblk_slow

[Jeff Moyer]

Hi,

This commit:

commit 080399aaaf3531f5b8761ec0ac30ff98891e8686
Author: Jeff Moyer <jmoyer@redhat.com>
Date:   Fri May 11 16:34:10 2012 +0200

    block: don't mark buffers beyond end of disk as mapped

exposed a bug in __getblk_slow that causes mount to hang as it loops
infinitely waiting for a buffer that lies beyond the end of the disk to
become uptodate.  The problem was initially reported by Torsten Hilbrich
here: https://lkml.org/lkml/2012/6/18/54, and also reported
independently here:
http://www.sysresccd.org/forums/viewtopic.php?f=13&t=4511, and then
Richard W.M. Jones and Marcos Mello noted a few separate bugzillas also
associated with the same issue.

The main problem is here, in __getblk_slow:

        for (;;) {
                struct buffer_head * bh;
                int ret;

                bh = __find_get_block(bdev, block, size);
                if (bh)
                        return bh;

                ret = grow_buffers(bdev, block, size);
                if (ret < 0)
                        return NULL;
                if (ret == 0)
                        free_more_memory();
        }

__find_get_block does not find the block, since it will not be marked as
mapped, and so grow_buffers is called to fill in the buffers for the
associated page.  I believe the for (;;) loop is there primarily to
retry in the case of memory pressure keeping grow_buffers from
succeeding.  However, we also continue to loop for other cases, like the
block lying beond the end of the disk.  So, the fix I came up with is to
only loop when grow_buffers fails due to memory allocation issues
(return value of 0).

The attached patch was tested by myself, Torsten, and Rich, and was
found to resolve the problem in call cases.

Comments, as always, are appreciated.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Reported-and-Tested-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
Cc: Stable <stable@vger.kernel.org>

--
Stable Notes: this patch requires backport to 3.0, 3.2 and 3.3.

diff --git a/fs/buffer.c b/fs/buffer.c
index 838a9cf..c7062c8 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1036,6 +1036,9 @@ grow_buffers(struct block_device *bdev, sector_t block, int size)
 static struct buffer_head *
 __getblk_slow(struct block_device *bdev, sector_t block, int size)
 {
+	int ret;
+	struct buffer_head *bh;
+
 	/* Size must be multiple of hard sectorsize */
 	if (unlikely(size & (bdev_logical_block_size(bdev)-1) ||
 			(size < 512 || size > PAGE_SIZE))) {
@@ -1048,20 +1051,21 @@ __getblk_slow(struct block_device *bdev, sector_t block, int size)
 		return NULL;
 	}
 
-	for (;;) {
-		struct buffer_head * bh;
-		int ret;
+retry:
+	bh = __find_get_block(bdev, block, size);
+	if (bh)
+		return bh;
 
+	ret = grow_buffers(bdev, block, size);
+	if (ret == 0) {
+		free_more_memory();
+		goto retry;
+	} else if (ret > 0) {
 		bh = __find_get_block(bdev, block, size);
 		if (bh)
 			return bh;
-
-		ret = grow_buffers(bdev, block, size);
-		if (ret < 0)
-			return NULL;
-		if (ret == 0)
-			free_more_memory();
 	}
+	return NULL;
 }
 
 /*

Re: [patch] block: fix infinite loop in __getblk_slow

[Josh Boyer]

On Tue, Jun 26, 2012 at 10:55 AM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Hi,
>
> This commit:
>
> commit 080399aaaf3531f5b8761ec0ac30ff98891e8686
> Author: Jeff Moyer <jmoyer@redhat.com>
> Date:   Fri May 11 16:34:10 2012 +0200
>
>     block: don't mark buffers beyond end of disk as mapped
>
> exposed a bug in __getblk_slow that causes mount to hang as it loops
> infinitely waiting for a buffer that lies beyond the end of the disk to
> become uptodate.  The problem was initially reported by Torsten Hilbrich
> here: https://lkml.org/lkml/2012/6/18/54, and also reported
> independently here:
> http://www.sysresccd.org/forums/viewtopic.php?f=13&t=4511, and then
> Richard W.M. Jones and Marcos Mello noted a few separate bugzillas also
> associated with the same issue.
>
> The main problem is here, in __getblk_slow:
>
>         for (;;) {
>                 struct buffer_head * bh;
>                 int ret;
>
>                 bh = __find_get_block(bdev, block, size);
>                 if (bh)
>                         return bh;
>
>                 ret = grow_buffers(bdev, block, size);
>                 if (ret < 0)
>                         return NULL;
>                 if (ret == 0)
>                         free_more_memory();
>         }
>
> __find_get_block does not find the block, since it will not be marked as
> mapped, and so grow_buffers is called to fill in the buffers for the
> associated page.  I believe the for (;;) loop is there primarily to
> retry in the case of memory pressure keeping grow_buffers from
> succeeding.  However, we also continue to loop for other cases, like the
> block lying beond the end of the disk.  So, the fix I came up with is to
> only loop when grow_buffers fails due to memory allocation issues
> (return value of 0).
>
> The attached patch was tested by myself, Torsten, and Rich, and was
> found to resolve the problem in call cases.
>
> Comments, as always, are appreciated.

Is this needed in addition to your earlier patch here:

http://article.gmane.org/gmane.linux.kernel/1316228

or is it a replacement for that?

josh

Re: [patch] block: fix infinite loop in __getblk_slow

[Jeff Moyer]

Josh Boyer <jwboyer@gmail.com> writes:

> Is this needed in addition to your earlier patch here:
>
> http://article.gmane.org/gmane.linux.kernel/1316228
>
> or is it a replacement for that?

Sorry for the confusion, it's a replacement for that patch.

Cheers,
Jeff

Re: [patch] block: fix infinite loop in __getblk_slow

[Jeff Moyer]

Jens, ping?  It would be good to get this reviewed/merged quickly.

Thanks!
Jeff

Jeff Moyer <jmoyer@redhat.com> writes:

> Hi,
>
> This commit:
>
> commit 080399aaaf3531f5b8761ec0ac30ff98891e8686
> Author: Jeff Moyer <jmoyer@redhat.com>
> Date:   Fri May 11 16:34:10 2012 +0200
>
>     block: don't mark buffers beyond end of disk as mapped
>
> exposed a bug in __getblk_slow that causes mount to hang as it loops
> infinitely waiting for a buffer that lies beyond the end of the disk to
> become uptodate.  The problem was initially reported by Torsten Hilbrich
> here: https://lkml.org/lkml/2012/6/18/54, and also reported
> independently here:
> http://www.sysresccd.org/forums/viewtopic.php?f=13&t=4511, and then
> Richard W.M. Jones and Marcos Mello noted a few separate bugzillas also
> associated with the same issue.
>
> The main problem is here, in __getblk_slow:
>
>         for (;;) {
>                 struct buffer_head * bh;
>                 int ret;
>
>                 bh = __find_get_block(bdev, block, size);
>                 if (bh)
>                         return bh;
>
>                 ret = grow_buffers(bdev, block, size);
>                 if (ret < 0)
>                         return NULL;
>                 if (ret == 0)
>                         free_more_memory();
>         }
>
> __find_get_block does not find the block, since it will not be marked as
> mapped, and so grow_buffers is called to fill in the buffers for the
> associated page.  I believe the for (;;) loop is there primarily to
> retry in the case of memory pressure keeping grow_buffers from
> succeeding.  However, we also continue to loop for other cases, like the
> block lying beond the end of the disk.  So, the fix I came up with is to
> only loop when grow_buffers fails due to memory allocation issues
> (return value of 0).
>
> The attached patch was tested by myself, Torsten, and Rich, and was
> found to resolve the problem in call cases.
>
> Comments, as always, are appreciated.
>
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> Reported-and-Tested-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> Cc: Stable <stable@vger.kernel.org>
>
> --
> Stable Notes: this patch requires backport to 3.0, 3.2 and 3.3.
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 838a9cf..c7062c8 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1036,6 +1036,9 @@ grow_buffers(struct block_device *bdev, sector_t block, int size)
>  static struct buffer_head *
>  __getblk_slow(struct block_device *bdev, sector_t block, int size)
>  {
> +	int ret;
> +	struct buffer_head *bh;
> +
>  	/* Size must be multiple of hard sectorsize */
>  	if (unlikely(size & (bdev_logical_block_size(bdev)-1) ||
>  			(size < 512 || size > PAGE_SIZE))) {
> @@ -1048,20 +1051,21 @@ __getblk_slow(struct block_device *bdev, sector_t block, int size)
>  		return NULL;
>  	}
>  
> -	for (;;) {
> -		struct buffer_head * bh;
> -		int ret;
> +retry:
> +	bh = __find_get_block(bdev, block, size);
> +	if (bh)
> +		return bh;
>  
> +	ret = grow_buffers(bdev, block, size);
> +	if (ret == 0) {
> +		free_more_memory();
> +		goto retry;
> +	} else if (ret > 0) {
>  		bh = __find_get_block(bdev, block, size);
>  		if (bh)
>  			return bh;
> -
> -		ret = grow_buffers(bdev, block, size);
> -		if (ret < 0)
> -			return NULL;
> -		if (ret == 0)
> -			free_more_memory();
>  	}
> +	return NULL;
>  }
>  
>  /*