[PATCH] ext4: Buffer Head Leak In mmp

[PATCH] ext4: Buffer Head Leak In mmp

[vikram.jadhav07]

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

Description:  ext4: Buffer Head Leak In mmp

There is memory leak as both caller function kmmpd() and callee
read_mmp_block() not releasing bh_check  (i.e buffer_head).
Given patch fixes this problem.

Signed-off-by: Jadhav Vikram <vikramjadhavpucsd2007@gmail.com>

Patch:
=====
--- /root/linux.orig/fs/ext4/mmp.c      2016-01-18 07:23:06.227519005 +0530
+++ /root/linux/fs/ext4/mmp.c   2016-01-18 07:31:18.545518552 +0530
@@ -98,12 +98,17 @@
        }

        mmp = (struct mmp_struct *)((*bh)->b_data);
-       if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC)
+       if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
                ret = -EFSCORRUPTED;
-       else if (!ext4_mmp_csum_verify(sb, mmp))
+               brelse(*bh);
+               *bh = NULL;
+       } else if (!ext4_mmp_csum_verify(sb, mmp)) {
                ret = -EFSBADCRC;
-       else
+               brelse(*bh);
+               *bh = NULL;
+       } else {
                return 0;
+       }

 warn_exit:
        ext4_warning(sb, "Error %d while reading MMP block %llu",
@@ -225,6 +230,7 @@
                                             "The filesystem seems to have been"
                                             " multiply mounted.");
                                ext4_error(sb, "abort");
+                               put_bh(bh_check);
                                goto failed;
                        }
                        put_bh(bh_check);

[-- Attachment #2: ext4-mmp-brelse.patch --]
[-- Type: application/octet-stream, Size: 823 bytes --]

--- /root/linux.orig/fs/ext4/mmp.c	2016-01-18 07:23:06.227519005 +0530
+++ /root/linux/fs/ext4/mmp.c	2016-01-18 07:31:18.545518552 +0530
@@ -98,12 +98,17 @@
 	}
 
 	mmp = (struct mmp_struct *)((*bh)->b_data);
-	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC)
+	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
 		ret = -EFSCORRUPTED;
-	else if (!ext4_mmp_csum_verify(sb, mmp))
+		brelse(*bh);
+		*bh = NULL;
+	} else if (!ext4_mmp_csum_verify(sb, mmp)) {
 		ret = -EFSBADCRC;
-	else
+		brelse(*bh);
+		*bh = NULL;
+	} else {
 		return 0;
+	}
 
 warn_exit:
 	ext4_warning(sb, "Error %d while reading MMP block %llu",
@@ -225,6 +230,7 @@
 					     "The filesystem seems to have been"
 					     " multiply mounted.");
 				ext4_error(sb, "abort");
+				put_bh(bh_check);
 				goto failed;
 			}
 			put_bh(bh_check);

Re: [PATCH] ext4: Buffer Head Leak In mmp

[Andreas Dilger]

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

On Jan 20, 2016, at 8:48 AM, vikram.jadhav07 <vikramjadhavpucsd2007@gmail.com> wrote:
> 
> Description:  ext4: Buffer Head Leak In mmp
> 
> There is memory leak as both caller function kmmpd() and callee
> read_mmp_block() not releasing bh_check  (i.e buffer_head).
> Given patch fixes this problem.

I was going to give this a Signed-off-by, since it matches patches that we
have included into the Lustre ext4 patch series, but it seems the code is
different in newer kernels because of the metadata checksum feature and
could be improved a bit further.

> Signed-off-by: Jadhav Vikram <vikramjadhavpucsd2007@gmail.com>
> 
> Patch:
> =====
> --- /root/linux.orig/fs/ext4/mmp.c      2016-01-18 07:23:06.227519005 +0530
> +++ /root/linux/fs/ext4/mmp.c   2016-01-18 07:31:18.545518552 +0530
> @@ -98,12 +98,17 @@
>         }
> 
>         mmp = (struct mmp_struct *)((*bh)->b_data);
> +       if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
>                ret = -EFSCORRUPTED;
> +               brelse(*bh);
> +               *bh = NULL;
> +       } else if (!ext4_mmp_csum_verify(sb, mmp)) {
>                ret = -EFSBADCRC;
> +               brelse(*bh);
> +               *bh = NULL;
> +       } else {
>                return 0;
> +       }

Having the normal return be at the end of the error handling is non-standard
and would be better structured to have "goto out_bh;" in each of the
error cases, and then the "naked" return at the end.  That avoids duplicating
the error handling, and will avoid bugs/leaks in the future if this code is
changed to add more failure conditions or returns.


	mmp = (struct mmp_struct *)((*bh)->b_data);
	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
		ret = -EFSCORRUPTED;
		goto out_bh;
	}
	if (!ext4_mmp_csum_verify(sb, mmp)) {
		ret = -EFSBADCRC;
		goto out_bh;
	}

	return 0;

out_bh:
	brelse(*bh);
	*bh = NULL;
warn_exit:
	ext4_warning(sb, "Error %d while reading MMP block %llu",
		     ret, mmp_block);
	return ret;

> @@ -225,6 +230,7 @@
>                                             "The filesystem seems to have been"
>                                             " multiply mounted.");
>                                ext4_error(sb, "abort");
> +                               put_bh(bh_check);
>                                goto failed;
>                        }
>                        put_bh(bh_check);

I was going to suggest something similar here to consolidate the "put_bh()"
calls, but it doesn't look like that can be done easily.

Instead I found a bug because this isn't setting s_mmp_tsk = NULL as the
other error branches are doing, which will lead to the filesystem cleanup
code trying to stop this thread again.  Could you please remove the duplicate
"s_mmp_tsk = NULL" assignments.  I also see that the error handling is still
not quite correct because "retval" is not set in this branch, which further
emphasizes the benefits of using the "goto NNN" style of error handling.
It should be something like:

			if (retval) {
				ext4_error(sb, "error reading MMP data: %d",
					   retval);
				goto out_tsk;
			}
			if (mmp->mmp_seq != ...) {
				dump_mmp_msg(sb, mmp_check, ...);
				ext4_error(sb, "abort due to multiple mount");
				put_bh(bh_check);
				retval = -EBUSY;
				goto out_tsk;
			}
			put_bh(bh_check);
		}

		:
		:
	}

	:
	:

out_tsk:
	EXT4_SB(sb)->s_mmp_tsk = NULL;
out_free:
	kfree(data);
	brelse(bh);
	return retval;
}

Note "failed:" is renamed to "out_free:" since that code path is also used
in non-failure cases, for normal thread exit, and also to ensure that the
other two cases that explicitly set s_mmp_tsk = NULL and jumped to "failed:"
will also be cleaned up.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ext4: Buffer Head Leak In mmp

[Theodore Ts'o]

I've made the changes Andreas suggested, and ended up with this.  Andreas, PTAL?

     	      	      	      		     	   - Ted

commit 0304688676bdfc8159e165313d71da19c118ba27
Author: vikram.jadhav07 <vikramjadhavpucsd2007@gmail.com>
Date:   Sun Mar 13 17:56:52 2016 -0400

    ext4: clean up error handling in the MMP support
    
    There is memory leak as both caller function kmmpd() and callee
    read_mmp_block() not releasing bh_check  (i.e buffer_head).
    Given patch fixes this problem.
    
    [ Additional changes suggested by Andreas Dilger -- TYT ]
    
    Signed-off-by: Jadhav Vikram <vikramjadhavpucsd2007@gmail.com>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 0a512aa..2444527 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -91,21 +91,22 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
 	submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
 	wait_on_buffer(*bh);
 	if (!buffer_uptodate(*bh)) {
-		brelse(*bh);
-		*bh = NULL;
 		ret = -EIO;
 		goto warn_exit;
 	}
-
 	mmp = (struct mmp_struct *)((*bh)->b_data);
-	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC)
+	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
 		ret = -EFSCORRUPTED;
-	else if (!ext4_mmp_csum_verify(sb, mmp))
+		goto warn_exit;
+	}
+	if (!ext4_mmp_csum_verify(sb, mmp)) {
 		ret = -EFSBADCRC;
-	else
-		return 0;
-
+		goto warn_exit;
+	}
+	return 0;
 warn_exit:
+	brelse(*bh);
+	*bh = NULL;
 	ext4_warning(sb, "Error %d while reading MMP block %llu",
 		     ret, mmp_block);
 	return ret;
@@ -181,15 +182,13 @@ static int kmmpd(void *data)
 		    EXT4_FEATURE_INCOMPAT_MMP)) {
 			ext4_warning(sb, "kmmpd being stopped since MMP feature"
 				     " has been disabled.");
-			EXT4_SB(sb)->s_mmp_tsk = NULL;
-			goto failed;
+			goto exit_thread;
 		}
 
 		if (sb->s_flags & MS_RDONLY) {
 			ext4_warning(sb, "kmmpd being stopped since filesystem "
 				     "has been remounted as readonly.");
-			EXT4_SB(sb)->s_mmp_tsk = NULL;
-			goto failed;
+			goto exit_thread;
 		}
 
 		diff = jiffies - last_update_time;
@@ -211,9 +210,7 @@ static int kmmpd(void *data)
 			if (retval) {
 				ext4_error(sb, "error reading MMP data: %d",
 					   retval);
-
-				EXT4_SB(sb)->s_mmp_tsk = NULL;
-				goto failed;
+				goto exit_thread;
 			}
 
 			mmp_check = (struct mmp_struct *)(bh_check->b_data);
@@ -225,7 +222,9 @@ static int kmmpd(void *data)
 					     "The filesystem seems to have been"
 					     " multiply mounted.");
 				ext4_error(sb, "abort");
-				goto failed;
+				put_bh(bh_check);
+				retval = -EBUSY;
+				goto exit_thread;
 			}
 			put_bh(bh_check);
 		}
@@ -248,7 +247,8 @@ static int kmmpd(void *data)
 
 	retval = write_mmp_block(sb, bh);
 
-failed:
+exit_thread:
+	EXT4_SB(sb)->s_mmp_tsk = NULL;
 	kfree(data);
 	brelse(bh);
 	return retval;

Re: [PATCH] ext4: Buffer Head Leak In mmp

[Andreas Dilger]

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

On Mar 13, 2016, at 3:57 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> I've made the changes Andreas suggested, and ended up with this.  Andreas, PTAL?

Sorry, not sure what "PTAL" means... :-)

In any case, the patch looks good to me.

Reviewed-by: Andreas Dilger <adilger@dilger.ca.

> commit 0304688676bdfc8159e165313d71da19c118ba27
> Author: vikram.jadhav07 <vikramjadhavpucsd2007@gmail.com>
> Date:   Sun Mar 13 17:56:52 2016 -0400
> 
>    ext4: clean up error handling in the MMP support
> 
>    There is memory leak as both caller function kmmpd() and callee
>    read_mmp_block() not releasing bh_check  (i.e buffer_head).
>    Given patch fixes this problem.
> 
>    [ Additional changes suggested by Andreas Dilger -- TYT ]
> 
>    Signed-off-by: Jadhav Vikram <vikramjadhavpucsd2007@gmail.com>
>    Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 0a512aa..2444527 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -91,21 +91,22 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,
> 	submit_bh(READ_SYNC | REQ_META | REQ_PRIO, *bh);
> 	wait_on_buffer(*bh);
> 	if (!buffer_uptodate(*bh)) {
> -		brelse(*bh);
> -		*bh = NULL;
> 		ret = -EIO;
> 		goto warn_exit;
> 	}
> -
> 	mmp = (struct mmp_struct *)((*bh)->b_data);
> -	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC)
> +	if (le32_to_cpu(mmp->mmp_magic) != EXT4_MMP_MAGIC) {
> 		ret = -EFSCORRUPTED;
> -	else if (!ext4_mmp_csum_verify(sb, mmp))
> +		goto warn_exit;
> +	}
> +	if (!ext4_mmp_csum_verify(sb, mmp)) {
> 		ret = -EFSBADCRC;
> -	else
> -		return 0;
> -
> +		goto warn_exit;
> +	}
> +	return 0;
> warn_exit:
> +	brelse(*bh);
> +	*bh = NULL;
> 	ext4_warning(sb, "Error %d while reading MMP block %llu",
> 		     ret, mmp_block);
> 	return ret;
> @@ -181,15 +182,13 @@ static int kmmpd(void *data)
> 		    EXT4_FEATURE_INCOMPAT_MMP)) {
> 			ext4_warning(sb, "kmmpd being stopped since MMP feature"
> 				     " has been disabled.");
> -			EXT4_SB(sb)->s_mmp_tsk = NULL;
> -			goto failed;
> +			goto exit_thread;
> 		}
> 
> 		if (sb->s_flags & MS_RDONLY) {
> 			ext4_warning(sb, "kmmpd being stopped since filesystem "
> 				     "has been remounted as readonly.");
> -			EXT4_SB(sb)->s_mmp_tsk = NULL;
> -			goto failed;
> +			goto exit_thread;
> 		}
> 
> 		diff = jiffies - last_update_time;
> @@ -211,9 +210,7 @@ static int kmmpd(void *data)
> 			if (retval) {
> 				ext4_error(sb, "error reading MMP data: %d",
> 					   retval);
> -
> -				EXT4_SB(sb)->s_mmp_tsk = NULL;
> -				goto failed;
> +				goto exit_thread;
> 			}
> 
> 			mmp_check = (struct mmp_struct *)(bh_check->b_data);
> @@ -225,7 +222,9 @@ static int kmmpd(void *data)
> 					     "The filesystem seems to have been"
> 					     " multiply mounted.");
> 				ext4_error(sb, "abort");
> -				goto failed;
> +				put_bh(bh_check);
> +				retval = -EBUSY;
> +				goto exit_thread;
> 			}
> 			put_bh(bh_check);
> 		}
> @@ -248,7 +247,8 @@ static int kmmpd(void *data)
> 
> 	retval = write_mmp_block(sb, bh);
> 
> -failed:
> +exit_thread:
> +	EXT4_SB(sb)->s_mmp_tsk = NULL;
> 	kfree(data);
> 	brelse(bh);
> 	return retval;


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ext4: Buffer Head Leak In mmp

[Theodore Ts'o]

On Mon, Mar 14, 2016 at 02:41:34AM -0600, Andreas Dilger wrote:
> On Mar 13, 2016, at 3:57 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> > 
> > I've made the changes Andreas suggested, and ended up with this.  Andreas, PTAL?
> 
> Sorry, not sure what "PTAL" means... :-)

"Please take a look".

> In any case, the patch looks good to me.

Which is sometimes abbreviated "LGTM".   :-)

Thanks for the code review!

					- Ted