[PATCH] Test for sb_getblk return value

[PATCH] Test for sb_getblk return value

[Glauber de Oliveira Costa]

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

Hi all,

As we discussed earlier, I'm sending a patch that adds test for the
return value of sb_getblk. This time I focused on the code of the ext2/3
filesystems. I'm assuming that getblk fails happens due to I/O errors
and thus returning returning an EIO back wherever it's needed.

Al, hope it's better this time.

-- 
=====================================
Glauber de Oliveira Costa
IBM Linux Technology Center - Brazil
glommer@br.ibm.com
=====================================

[-- Attachment #2: patch_getblk_ext3 --]
[-- Type: text/plain, Size: 3104 bytes --]

diff -Naurp linux-2.6.14-rc2-orig/fs/ext2/inode.c linux-2.6.14-rc2-cleanp/fs/ext2/inode.c
--- linux-2.6.14-rc2-orig/fs/ext2/inode.c	2005-09-26 13:58:15.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext2/inode.c	2005-10-17 12:32:46.000000000 +0000
@@ -439,7 +439,10 @@ static int ext2_alloc_branch(struct inod
 		 * Get buffer_head for parent block, zero it out and set 
 		 * the pointer to new one, then send parent to disk.
 		 */
-		bh = sb_getblk(inode->i_sb, parent);
+		if (!(bh = sb_getblk(inode->i_sb, parent))){
+			err = -EIO;
+			break;
+		}
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/inode.c linux-2.6.14-rc2-cleanp/fs/ext3/inode.c
--- linux-2.6.14-rc2-orig/fs/ext3/inode.c	2005-10-09 20:26:54.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/inode.c	2005-10-17 12:28:38.000000000 +0000
@@ -523,14 +523,15 @@ static int ext3_alloc_branch(handle_t *h
 			if (!nr)
 				break;
 			branch[n].key = cpu_to_le32(nr);
-			keys = n+1;
 
 			/*
 			 * Get buffer_head for parent block, zero it out
 			 * and set the pointer to new one, then send
 			 * parent to disk.  
 			 */
-			bh = sb_getblk(inode->i_sb, parent);
+			if (!(bh = sb_getblk(inode->i_sb, parent)))
+				break;	
+			keys = n+1;
 			branch[n].bh = bh;
 			lock_buffer(bh);
 			BUFFER_TRACE(bh, "call get_create_access");
@@ -863,7 +864,10 @@ struct buffer_head *ext3_getblk(handle_t
 	*errp = ext3_get_block_handle(handle, inode, block, &dummy, create, 1);
 	if (!*errp && buffer_mapped(&dummy)) {
 		struct buffer_head *bh;
-		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+		if (!(bh = sb_getblk(inode->i_sb, dummy.b_blocknr))){
+			*errp = -EIO;
+			return NULL;
+		}
 		if (buffer_new(&dummy)) {
 			J_ASSERT(create != 0);
 			J_ASSERT(handle != 0);
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/resize.c linux-2.6.14-rc2-cleanp/fs/ext3/resize.c
--- linux-2.6.14-rc2-orig/fs/ext3/resize.c	2005-10-09 19:58:40.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/resize.c	2005-10-17 12:41:49.000000000 +0000
@@ -117,7 +117,8 @@ static struct buffer_head *bclean(handle
 	struct buffer_head *bh;
 	int err;
 
-	bh = sb_getblk(sb, blk);
+	if (!(bh = sb_getblk(sb, blk)))
+		return ERR_PTR(-EIO);
 	if ((err = ext3_journal_get_write_access(handle, bh))) {
 		brelse(bh);
 		bh = ERR_PTR(err);
@@ -201,7 +202,10 @@ static int setup_new_group_blocks(struct
 
 		ext3_debug("update backup group %#04lx (+%d)\n", block, bit);
 
-		gdb = sb_getblk(sb, block);
+		if (!(gdb = sb_getblk(sb, block))){
+			err = -EIO;
+			goto exit_bh;
+		}
 		if ((err = ext3_journal_get_write_access(handle, gdb))) {
 			brelse(gdb);
 			goto exit_bh;
@@ -642,7 +646,10 @@ static void update_backups(struct super_
 		    (err = ext3_journal_restart(handle, EXT3_MAX_TRANS_DATA)))
 			break;
 
-		bh = sb_getblk(sb, group * bpg + blk_off);
+		if (!(bh = sb_getblk(sb, group * bpg + blk_off))){
+			err = -EIO;
+			goto exit_err;
+		}
 		ext3_debug("update metadata backup %#04lx\n",
 			  (unsigned long)bh->b_blocknr);
 		if ((err = ext3_journal_get_write_access(handle, bh)))

Re: [PATCH] Test for sb_getblk return value

[Jesper Juhl]

On 10/17/05, Glauber de Oliveira Costa <glauber@br.ibm.com> wrote:
> Hi all,
>
> As we discussed earlier, I'm sending a patch that adds test for the
> return value of sb_getblk. This time I focused on the code of the ext2/3
> filesystems. I'm assuming that getblk fails happens due to I/O errors
> and thus returning returning an EIO back wherever it's needed.
>

> -		bh = sb_getblk(inode->i_sb, parent);
> +		if (!(bh = sb_getblk(inode->i_sb, parent))){
> +			err = -EIO;
> +			break;
> +		}

Would be more readable as

		bh = sb_getblk(inode->i_sb, parent);
		if (!bh) {
			err = -EIO;
			break;
		}


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Test for sb_getblk return value

[Glauber de Oliveira Costa]

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

I'm resending it now with the changes you suggested. 
Actually, 2 copies of it follows. 

In the first one(v2), I kept the style in the changes in resize.c, as this 
seems to be the default way things like this are done there. In the other, 
(v3), I did statement checks in the way you suggested in both files.

Also, sorry for the last mail. I got a problem with my relay, and my mail 
address was sent wrong before I noticed that. Mails sent to it will probably 
return.

On Monday 17 October 2005 12:10, Jesper Juhl wrote:
> On 10/17/05, Glauber de Oliveira Costa <glauber@br.ibm.com> wrote:
> > Hi all,
> >
> > As we discussed earlier, I'm sending a patch that adds test for the
> > return value of sb_getblk. This time I focused on the code of the ext2/3
> > filesystems. I'm assuming that getblk fails happens due to I/O errors
> > and thus returning returning an EIO back wherever it's needed.
> >
> >
> > -  bh = sb_getblk(inode->i_sb, parent);
> > +  if (!(bh = sb_getblk(inode->i_sb, parent))){
> > +   err = -EIO;
> > +   break;
> > +  }
>
> Would be more readable as
>
>   bh = sb_getblk(inode->i_sb, parent);
>   if (!bh) {
>    err = -EIO;
>    break;
>   }
>
>
> --
> Jesper Juhl <jesper.juhl@gmail.com>
> Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please      http://www.expita.com/nomime.html
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
=====================================
Glauber de Oliveira Costa
IBM Linux Technology Center - Brazil
glommer@br.ibm.com
=====================================

[-- Attachment #2: patch_getblk_ext3.v2 --]
[-- Type: text/x-diff, Size: 2865 bytes --]

diff -Naurp linux-2.6.14-rc2-orig/fs/ext2/inode.c linux-2.6.14-rc2-cleanp/fs/ext2/inode.c
--- linux-2.6.14-rc2-orig/fs/ext2/inode.c	2005-09-26 13:58:15.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext2/inode.c	2005-10-17 14:17:54.000000000 +0000
@@ -440,6 +440,10 @@ static int ext2_alloc_branch(struct inod
 		 * the pointer to new one, then send parent to disk.
 		 */
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh){
+			err = -EIO;
+			break;
+		}
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/inode.c linux-2.6.14-rc2-cleanp/fs/ext3/inode.c
--- linux-2.6.14-rc2-orig/fs/ext3/inode.c	2005-10-09 20:26:54.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/inode.c	2005-10-17 14:23:50.000000000 +0000
@@ -523,7 +523,6 @@ static int ext3_alloc_branch(handle_t *h
 			if (!nr)
 				break;
 			branch[n].key = cpu_to_le32(nr);
-			keys = n+1;
 
 			/*
 			 * Get buffer_head for parent block, zero it out
@@ -531,6 +530,9 @@ static int ext3_alloc_branch(handle_t *h
 			 * parent to disk.  
 			 */
 			bh = sb_getblk(inode->i_sb, parent);
+			if (!bh)
+				break;	
+			keys = n+1;
 			branch[n].bh = bh;
 			lock_buffer(bh);
 			BUFFER_TRACE(bh, "call get_create_access");
@@ -864,6 +866,10 @@ struct buffer_head *ext3_getblk(handle_t
 	if (!*errp && buffer_mapped(&dummy)) {
 		struct buffer_head *bh;
 		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+		if (!bh){
+			*errp = -EIO;
+			return NULL;
+		}
 		if (buffer_new(&dummy)) {
 			J_ASSERT(create != 0);
 			J_ASSERT(handle != 0);
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/resize.c linux-2.6.14-rc2-cleanp/fs/ext3/resize.c
--- linux-2.6.14-rc2-orig/fs/ext3/resize.c	2005-10-09 19:58:40.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/resize.c	2005-10-17 13:02:07.000000000 +0000
@@ -117,7 +117,8 @@ static struct buffer_head *bclean(handle
 	struct buffer_head *bh;
 	int err;
 
-	bh = sb_getblk(sb, blk);
+	if (!(bh = sb_getblk(sb, blk)))
+		return ERR_PTR(-EIO);
 	if ((err = ext3_journal_get_write_access(handle, bh))) {
 		brelse(bh);
 		bh = ERR_PTR(err);
@@ -201,7 +202,10 @@ static int setup_new_group_blocks(struct
 
 		ext3_debug("update backup group %#04lx (+%d)\n", block, bit);
 
-		gdb = sb_getblk(sb, block);
+		if (!(gdb = sb_getblk(sb, block))){
+			err = -EIO;
+			goto exit_bh;
+		}
 		if ((err = ext3_journal_get_write_access(handle, gdb))) {
 			brelse(gdb);
 			goto exit_bh;
@@ -642,7 +646,10 @@ static void update_backups(struct super_
 		    (err = ext3_journal_restart(handle, EXT3_MAX_TRANS_DATA)))
 			break;
 
-		bh = sb_getblk(sb, group * bpg + blk_off);
+		if (!(bh = sb_getblk(sb, group * bpg + blk_off))){
+			err = -EIO;
+			goto exit_err;
+		}
 		ext3_debug("update metadata backup %#04lx\n",
 			  (unsigned long)bh->b_blocknr);
 		if ((err = ext3_journal_get_write_access(handle, bh)))

[-- Attachment #3: patch_getblk_ext3.v3 --]
[-- Type: text/x-diff, Size: 2681 bytes --]

diff -Naurp linux-2.6.14-rc2-orig/fs/ext2/inode.c linux-2.6.14-rc2-cleanp/fs/ext2/inode.c
--- linux-2.6.14-rc2-orig/fs/ext2/inode.c	2005-09-26 13:58:15.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext2/inode.c	2005-10-17 14:17:54.000000000 +0000
@@ -440,6 +440,10 @@ static int ext2_alloc_branch(struct inod
 		 * the pointer to new one, then send parent to disk.
 		 */
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh){
+			err = -EIO;
+			break;
+		}
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/inode.c linux-2.6.14-rc2-cleanp/fs/ext3/inode.c
--- linux-2.6.14-rc2-orig/fs/ext3/inode.c	2005-10-09 20:26:54.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/inode.c	2005-10-17 14:23:50.000000000 +0000
@@ -523,7 +523,6 @@ static int ext3_alloc_branch(handle_t *h
 			if (!nr)
 				break;
 			branch[n].key = cpu_to_le32(nr);
-			keys = n+1;
 
 			/*
 			 * Get buffer_head for parent block, zero it out
@@ -531,6 +530,9 @@ static int ext3_alloc_branch(handle_t *h
 			 * parent to disk.  
 			 */
 			bh = sb_getblk(inode->i_sb, parent);
+			if (!bh)
+				break;	
+			keys = n+1;
 			branch[n].bh = bh;
 			lock_buffer(bh);
 			BUFFER_TRACE(bh, "call get_create_access");
@@ -864,6 +866,10 @@ struct buffer_head *ext3_getblk(handle_t
 	if (!*errp && buffer_mapped(&dummy)) {
 		struct buffer_head *bh;
 		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+		if (!bh){
+			*errp = -EIO;
+			return NULL;
+		}
 		if (buffer_new(&dummy)) {
 			J_ASSERT(create != 0);
 			J_ASSERT(handle != 0);
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/resize.c linux-2.6.14-rc2-cleanp/fs/ext3/resize.c
--- linux-2.6.14-rc2-orig/fs/ext3/resize.c	2005-10-09 19:58:40.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/resize.c	2005-10-17 14:23:01.000000000 +0000
@@ -118,6 +118,8 @@ static struct buffer_head *bclean(handle
 	int err;
 
 	bh = sb_getblk(sb, blk);
+	if (!bh)
+		return ERR_PTR(-EIO);
 	if ((err = ext3_journal_get_write_access(handle, bh))) {
 		brelse(bh);
 		bh = ERR_PTR(err);
@@ -202,6 +204,10 @@ static int setup_new_group_blocks(struct
 		ext3_debug("update backup group %#04lx (+%d)\n", block, bit);
 
 		gdb = sb_getblk(sb, block);
+		if (!bh){
+			err = -EIO;
+			goto exit_bh;
+		}
 		if ((err = ext3_journal_get_write_access(handle, gdb))) {
 			brelse(gdb);
 			goto exit_bh;
@@ -643,6 +649,10 @@ static void update_backups(struct super_
 			break;
 
 		bh = sb_getblk(sb, group * bpg + blk_off);
+		if (!bh){
+			err = -EIO;
+			goto exit_err;
+		}
 		ext3_debug("update metadata backup %#04lx\n",
 			  (unsigned long)bh->b_blocknr);
 		if ((err = ext3_journal_get_write_access(handle, bh)))

Re: [PATCH] Test for sb_getblk return value

[Andrew Morton]

Glauber de Oliveira Costa <glommer@br.ibm.com> wrote:
>
> I'm resending it now with the changes you suggested. 
> Actually, 2 copies of it follows. 

argh.  Please never attach multiple patches to a single email.

And please always include a complete, uptodate changelog with each iteration
of a patch.  I don't want to have to troll back through the mailing list,
identify the initial changelog and then replay the email thread making any
needed updates to that changelog.

Also please review section 11 of Documentation/SubmittingPatches then
include a Signed-off-by: with your patches.

> In the first one(v2), I kept the style in the changes in resize.c, as this 
> seems to be the default way things like this are done there. In the other, 
> (v3), I did statement checks in the way you suggested in both files.

Don't worry about the surrounding style - if it's wrong, it's wrong.  Just
stick with Documentation/CodingStyle.

Do this:

		if (!bh) {

and not this:

		if (!bh){

> Also, sorry for the last mail. I got a problem with my relay, and my mail 
> address was sent wrong before I noticed that. Mails sent to it will probably 
> return.

The change to update_backups() is wrong - it will leave a JBD transaction
open on return.

Please fix all that up and resend. 
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt may prove useful,
thanks.

Re: [PATCH] Test for sb_getblk return value

[Glauber de Oliveira Costa]

Andrew,

Thanks a lot for your patience and review. And also for the document. It
was really helpfull. Patch follows in next mail.



On Tue, Oct 18, 2005 at 04:32:01PM -0700, Andrew Morton wrote:
> Glauber de Oliveira Costa <glommer@br.ibm.com> wrote:
> >
> > I'm resending it now with the changes you suggested. 
> > Actually, 2 copies of it follows. 
> 
> argh.  Please never attach multiple patches to a single email.
> 
> And please always include a complete, uptodate changelog with each iteration
> of a patch.  I don't want to have to troll back through the mailing list,
> identify the initial changelog and then replay the email thread making any
> needed updates to that changelog.
> 
> Also please review section 11 of Documentation/SubmittingPatches then
> include a Signed-off-by: with your patches.
> 
> > In the first one(v2), I kept the style in the changes in resize.c, as this 
> > seems to be the default way things like this are done there. In the other, 
> > (v3), I did statement checks in the way you suggested in both files.
> 
> Don't worry about the surrounding style - if it's wrong, it's wrong.  Just
> stick with Documentation/CodingStyle.
> 
> Do this:
> 
> 		if (!bh) {
> 
> and not this:
> 
> 		if (!bh){
> 
> > Also, sorry for the last mail. I got a problem with my relay, and my mail 
> > address was sent wrong before I noticed that. Mails sent to it will probably 
> > return.
> 
> The change to update_backups() is wrong - it will leave a JBD transaction
> open on return.
> 
> Please fix all that up and resend. 
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt may prove useful,
> thanks.
> 
> 

-- 
=====================================
Glauber de Oliveira Costa
IBM Linux Technology Center - Brazil
glommer@br.ibm.com
=====================================

Re: [PATCH] Test for sb_getblk return value

[Glauber de Oliveira Costa]

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

This patch adds tests for the return value of sb_getblk() in the ext2/3
filesystems. In fs/buffer.c it is stated that the getblk() function
never fails. However, it does can return NULL in some situations due to
I/O errors, which may lead us to NULL pointer dereferences 

Signed-off-by: Glauber de Oliveira Costa <glommer@br.ibm.com>

-- 
=====================================
Glauber de Oliveira Costa
IBM Linux Technology Center - Brazil
glommer@br.ibm.com
=====================================

[-- Attachment #2: patch_getblk_ext3.v4 --]
[-- Type: text/plain, Size: 2677 bytes --]

diff -Naurp linux-2.6.14-rc2-orig/fs/ext2/inode.c linux-2.6.14-rc2-cleanp/fs/ext2/inode.c
--- linux-2.6.14-rc2-orig/fs/ext2/inode.c	2005-10-19 02:04:12.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext2/inode.c	2005-10-19 12:07:12.000000000 +0000
@@ -440,6 +440,10 @@ static int ext2_alloc_branch(struct inod
 		 * the pointer to new one, then send parent to disk.
 		 */
 		bh = sb_getblk(inode->i_sb, parent);
+		if (!bh) {
+			err = -EIO;
+			break;
+		}
 		lock_buffer(bh);
 		memset(bh->b_data, 0, blocksize);
 		branch[n].bh = bh;
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/inode.c linux-2.6.14-rc2-cleanp/fs/ext3/inode.c
--- linux-2.6.14-rc2-orig/fs/ext3/inode.c	2005-10-19 02:04:12.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/inode.c	2005-10-19 02:03:22.000000000 +0000
@@ -523,7 +523,6 @@ static int ext3_alloc_branch(handle_t *h
 			if (!nr)
 				break;
 			branch[n].key = cpu_to_le32(nr);
-			keys = n+1;
 
 			/*
 			 * Get buffer_head for parent block, zero it out
@@ -531,6 +530,9 @@ static int ext3_alloc_branch(handle_t *h
 			 * parent to disk.  
 			 */
 			bh = sb_getblk(inode->i_sb, parent);
+			if (!bh)
+				break;	
+			keys = n+1;
 			branch[n].bh = bh;
 			lock_buffer(bh);
 			BUFFER_TRACE(bh, "call get_create_access");
@@ -864,6 +866,10 @@ struct buffer_head *ext3_getblk(handle_t
 	if (!*errp && buffer_mapped(&dummy)) {
 		struct buffer_head *bh;
 		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+		if (!bh) {
+			*errp = -EIO;
+			return NULL;
+		}
 		if (buffer_new(&dummy)) {
 			J_ASSERT(create != 0);
 			J_ASSERT(handle != 0);
diff -Naurp linux-2.6.14-rc2-orig/fs/ext3/resize.c linux-2.6.14-rc2-cleanp/fs/ext3/resize.c
--- linux-2.6.14-rc2-orig/fs/ext3/resize.c	2005-10-19 02:04:12.000000000 +0000
+++ linux-2.6.14-rc2-cleanp/fs/ext3/resize.c	2005-10-19 01:54:47.000000000 +0000
@@ -118,6 +118,8 @@ static struct buffer_head *bclean(handle
 	int err;
 
 	bh = sb_getblk(sb, blk);
+	if (!bh)
+		return ERR_PTR(-EIO);
 	if ((err = ext3_journal_get_write_access(handle, bh))) {
 		brelse(bh);
 		bh = ERR_PTR(err);
@@ -202,6 +204,10 @@ static int setup_new_group_blocks(struct
 		ext3_debug("update backup group %#04lx (+%d)\n", block, bit);
 
 		gdb = sb_getblk(sb, block);
+		if (!bh) {
+			err = -EIO;
+			goto exit_bh;
+		}
 		if ((err = ext3_journal_get_write_access(handle, gdb))) {
 			brelse(gdb);
 			goto exit_bh;
@@ -643,6 +649,10 @@ static void update_backups(struct super_
 			break;
 
 		bh = sb_getblk(sb, group * bpg + blk_off);
+		if (!bh) {
+			err = -EIO;
+			break;
+		}
 		ext3_debug("update metadata backup %#04lx\n",
 			  (unsigned long)bh->b_blocknr);
 		if ((err = ext3_journal_get_write_access(handle, bh)))