ext4: Buffer Head Leak In mmp

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>

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html