Re: [PATCH 1/8] ext4 crypto: fix memory leaks in ext4_encrypted_zeroout

[Jaegeuk Kim <jaegeuk@kernel.org>]

On Thu, May 28, 2015 at 07:47:40PM -0400, Theodore Ts'o wrote:
> ext4_encrypted_zeroout() could end up leaking a bio and bounce page.
> Fortunately it's not used much.  While we're fixing things up,
> refactor out common code into the static function alloc_bounce_page().
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>  fs/ext4/crypto.c | 56 +++++++++++++++++++++++++-------------------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
> index 68c7ab8..e43ed93 100644
> --- a/fs/ext4/crypto.c
> +++ b/fs/ext4/crypto.c
> @@ -324,6 +324,28 @@ static int ext4_page_crypto(struct ext4_crypto_ctx *ctx,
>  	return 0;
>  }
>  
> +static struct page *alloc_bounce_page(struct ext4_crypto_ctx *ctx)
> +{
> +	struct page *ciphertext_page = alloc_page(GFP_NOFS);
> +
> +	if (!ciphertext_page) {
> +		/* This is a potential bottleneck, but at least we'll have
> +		 * forward progress. */
> +		ciphertext_page = mempool_alloc(ext4_bounce_page_pool,
> +						 GFP_NOFS);
> +		if (WARN_ON_ONCE(!ciphertext_page)) {
> +			ciphertext_page = mempool_alloc(ext4_bounce_page_pool,
> +							 GFP_NOFS | __GFP_WAIT);

Not sure why __GFP_WAIT is defined here.
Even GFP_NOFS has __GFP_WAIT.

#define GFP_NOFS	(__GFP_WAIT | __GFP_IO)

IMO, __GFP_NOFAIL should be used here?
Otherwise, we seem to handle ENOMEM instead.

Thanks,

> +		}
> +		ctx->flags &= ~EXT4_BOUNCE_PAGE_REQUIRES_FREE_ENCRYPT_FL;
> +	} else {
> +		ctx->flags |= EXT4_BOUNCE_PAGE_REQUIRES_FREE_ENCRYPT_FL;
> +	}
> +	ctx->flags |= EXT4_WRITE_PATH_FL;
> +	ctx->w.bounce_page = ciphertext_page;
> +	return ciphertext_page;
> +}
> +
>  /**
>   * ext4_encrypt() - Encrypts a page
>   * @inode:          The inode for which the encryption should take place
> @@ -353,22 +375,7 @@ struct page *ext4_encrypt(struct inode *inode,
>  		return (struct page *) ctx;
>  
>  	/* The encryption operation will require a bounce page. */
> -	ciphertext_page = alloc_page(GFP_NOFS);
> -	if (!ciphertext_page) {
> -		/* This is a potential bottleneck, but at least we'll have
> -		 * forward progress. */
> -		ciphertext_page = mempool_alloc(ext4_bounce_page_pool,
> -						 GFP_NOFS);
> -		if (WARN_ON_ONCE(!ciphertext_page)) {
> -			ciphertext_page = mempool_alloc(ext4_bounce_page_pool,
> -							 GFP_NOFS | __GFP_WAIT);
> -		}
> -		ctx->flags &= ~EXT4_BOUNCE_PAGE_REQUIRES_FREE_ENCRYPT_FL;
> -	} else {
> -		ctx->flags |= EXT4_BOUNCE_PAGE_REQUIRES_FREE_ENCRYPT_FL;
> -	}
> -	ctx->flags |= EXT4_WRITE_PATH_FL;
> -	ctx->w.bounce_page = ciphertext_page;
> +	ciphertext_page = alloc_bounce_page(ctx);
>  	ctx->w.control_page = plaintext_page;
>  	err = ext4_page_crypto(ctx, inode, EXT4_ENCRYPT, plaintext_page->index,
>  			       plaintext_page, ciphertext_page);
> @@ -434,21 +441,7 @@ int ext4_encrypted_zeroout(struct inode *inode, struct ext4_extent *ex)
>  	if (IS_ERR(ctx))
>  		return PTR_ERR(ctx);
>  
> -	ciphertext_page = alloc_page(GFP_NOFS);
> -	if (!ciphertext_page) {
> -		/* This is a potential bottleneck, but at least we'll have
> -		 * forward progress. */
> -		ciphertext_page = mempool_alloc(ext4_bounce_page_pool,
> -						 GFP_NOFS);
> -		if (WARN_ON_ONCE(!ciphertext_page)) {
> -			ciphertext_page = mempool_alloc(ext4_bounce_page_pool,
> -							 GFP_NOFS | __GFP_WAIT);
> -		}
> -		ctx->flags &= ~EXT4_BOUNCE_PAGE_REQUIRES_FREE_ENCRYPT_FL;
> -	} else {
> -		ctx->flags |= EXT4_BOUNCE_PAGE_REQUIRES_FREE_ENCRYPT_FL;
> -	}
> -	ctx->w.bounce_page = ciphertext_page;
> +	ciphertext_page = alloc_bounce_page(ctx);
>  
>  	while (len--) {
>  		err = ext4_page_crypto(ctx, inode, EXT4_ENCRYPT, lblk,
> @@ -470,6 +463,7 @@ int ext4_encrypted_zeroout(struct inode *inode, struct ext4_extent *ex)
>  			goto errout;
>  		}
>  		err = submit_bio_wait(WRITE, bio);
> +		bio_put(bio);
>  		if (err)
>  			goto errout;
>  	}
> -- 
> 2.3.0