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