From: Jaegeuk Kim Subject: Re: [PATCH 1/8] ext4 crypto: fix memory leaks in ext4_encrypted_zeroout Date: Fri, 29 May 2015 11:08:34 -0700 Message-ID: <20150529180834.GA22657@jaegeuk-mac02> References: <1432856867-5710-1-git-send-email-tytso@mit.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Ext4 Developers List , mhalcrow@google.com To: Theodore Ts'o Return-path: Received: from mail.kernel.org ([198.145.29.136]:40313 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752882AbbE2SIj (ORCPT ); Fri, 29 May 2015 14:08:39 -0400 Content-Disposition: inline In-Reply-To: <1432856867-5710-1-git-send-email-tytso@mit.edu> Sender: linux-ext4-owner@vger.kernel.org List-ID: 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 > --- > 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