Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761742AbcLPPsA convert rfc822-to-8bit (ORCPT ); Fri, 16 Dec 2016 10:48:00 -0500 Received: from mail.sigma-star.at ([95.130.255.111]:45996 "EHLO mail.sigma-star.at" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757547AbcLPPr4 (ORCPT ); Fri, 16 Dec 2016 10:47:56 -0500 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH] fscrypt: Factor out bio specific functions From: David Gstir In-Reply-To: <20161216105006.10207-1-richard@nod.at> Date: Fri, 16 Dec 2016 16:37:40 +0100 Cc: linux-fsdevel , linux-kernel@vger.kernel.org, jaegeuk@kernel.org, tytso@mit.edu, hch@infradead.org, arnd@arndb.de, dedekind1@gmail.com, linux-mtd@lists.infradead.org, adrian.hunter@intel.com, linux-ext4@vger.kernel.org, ebiggers@google.com, rdunlap@infradead.org Content-Transfer-Encoding: 8BIT Message-Id: <72D4D0FB-F56D-4457-9660-4CFF7B4CFB6E@sigma-star.at> References: <20161216105006.10207-1-richard@nod.at> To: Richard Weinberger X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10633 Lines: 359 Hi, > On 16.12.2016, at 11:50, Richard Weinberger wrote: > > That way we can get rid of the direct dependency on CONFIG_BLOCK. > > Reported-by: Arnd Bergmann > Reported-by: Randy Dunlap > Suggested-by: Christoph Hellwig > Fixes: d475a507457b ("ubifs: Add skeleton for fscrypto") > Signed-off-by: Richard Weinberger > --- > fs/crypto/Kconfig | 1 - > fs/crypto/Makefile | 1 + > fs/crypto/bio.c | 115 ++++++++++++++++++++++++++++++++++++++++++++ > fs/crypto/crypto.c | 89 ++-------------------------------- > fs/crypto/fscrypt_private.h | 14 ++++++ > include/linux/fscrypto.h | 6 ++- > 6 files changed, 137 insertions(+), 89 deletions(-) > create mode 100644 fs/crypto/bio.c > > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig > index f514978f6688..08b46e6e3995 100644 > --- a/fs/crypto/Kconfig > +++ b/fs/crypto/Kconfig > @@ -1,6 +1,5 @@ > config FS_ENCRYPTION > tristate "FS Encryption (Per-file encryption)" > - depends on BLOCK > select CRYPTO > select CRYPTO_AES > select CRYPTO_CBC > diff --git a/fs/crypto/Makefile b/fs/crypto/Makefile > index f17684c48739..9f6607f17b53 100644 > --- a/fs/crypto/Makefile > +++ b/fs/crypto/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_FS_ENCRYPTION) += fscrypto.o > > fscrypto-y := crypto.o fname.o policy.o keyinfo.o > +fscrypto-$(CONFIG_BLOCK) += bio.o > diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c > new file mode 100644 > index 000000000000..6a6e694c6879 > --- /dev/null > +++ b/fs/crypto/bio.c > @@ -0,0 +1,115 @@ > +/* > + * This contains encryption functions for per-file encryption. > + * > + * Copyright (C) 2015, Google, Inc. > + * Copyright (C) 2015, Motorola Mobility > + * > + * Written by Michael Halcrow, 2014. > + * > + * Filename encryption additions > + * Uday Savagaonkar, 2014 > + * Encryption policy handling additions > + * Ildar Muslukhov, 2014 > + * Add fscrypt_pullback_bio_page() > + * Jaegeuk Kim, 2015. > + * > + * This has not yet undergone a rigorous security audit. > + * > + * The usage of AES-XTS should conform to recommendations in NIST > + * Special Publication 800-38E and IEEE P1619/D16. > + */ > + > +#include > +#include > +#include > +#include > +#include "fscrypt_private.h" > + > +/* > + * Call fscrypt_decrypt_page on every single page, reusing the encryption > + * context. > + */ > +static void completion_pages(struct work_struct *work) > +{ > + struct fscrypt_ctx *ctx = > + container_of(work, struct fscrypt_ctx, r.work); > + struct bio *bio = ctx->r.bio; > + struct bio_vec *bv; > + int i; > + > + bio_for_each_segment_all(bv, bio, i) { > + struct page *page = bv->bv_page; > + int ret = fscrypt_decrypt_page(page->mapping->host, page, > + PAGE_SIZE, 0, page->index); > + > + if (ret) { > + WARN_ON_ONCE(1); > + SetPageError(page); > + } else { > + SetPageUptodate(page); > + } > + unlock_page(page); > + } > + fscrypt_release_ctx(ctx); > + bio_put(bio); > +} > + > +void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, struct bio *bio) > +{ > + INIT_WORK(&ctx->r.work, completion_pages); > + ctx->r.bio = bio; > + queue_work(fscrypt_read_workqueue, &ctx->r.work); > +} > +EXPORT_SYMBOL(fscrypt_decrypt_bio_pages); > + > +void fscrypt_pullback_bio_page(struct page **page, bool restore) > +{ > + struct fscrypt_ctx *ctx; > + struct page *bounce_page; > + > + /* The bounce data pages are unmapped. */ > + if ((*page)->mapping) > + return; > + > + /* The bounce data page is unmapped. */ > + bounce_page = *page; > + ctx = (struct fscrypt_ctx *)page_private(bounce_page); > + > + /* restore control page */ > + *page = ctx->w.control_page; > + > + if (restore) > + fscrypt_restore_control_page(bounce_page); > +} > +EXPORT_SYMBOL(fscrypt_pullback_bio_page); > + > +int fscrypt_bio_submit_page(const struct inode *inode, sector_t blk, > + struct page *page) > +{ > + struct bio *bio; > + int ret; > + > + bio = bio_alloc(GFP_NOWAIT, 1); > + if (!bio) { > + ret = -ENOMEM; > + goto errout; > + } > + bio->bi_bdev = inode->i_sb->s_bdev; > + bio->bi_iter.bi_sector = blk << (inode->i_sb->s_blocksize_bits - 9); > + bio_set_op_attrs(bio, REQ_OP_WRITE, 0); > + ret = bio_add_page(bio, page, inode->i_sb->s_blocksize, 0); > + if (ret != inode->i_sb->s_blocksize) { > + /* should never happen! */ > + WARN_ON(1); > + bio_put(bio); > + ret = -EIO; > + goto errout; > + } > + ret = submit_bio_wait(bio); > + if ((ret == 0) && bio->bi_error) > + ret = -EIO; > + bio_put(bio); > + > +errout: > + return ret; > +} > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c > index ac8e4f6a3773..3e981f0fd6e3 100644 > --- a/fs/crypto/crypto.c > +++ b/fs/crypto/crypto.c > @@ -24,7 +24,6 @@ > #include > #include > #include > -#include > #include > #include > #include "fscrypt_private.h" > @@ -44,7 +43,7 @@ static mempool_t *fscrypt_bounce_page_pool = NULL; > static LIST_HEAD(fscrypt_free_ctxs); > static DEFINE_SPINLOCK(fscrypt_ctx_lock); > > -static struct workqueue_struct *fscrypt_read_workqueue; > +struct workqueue_struct *fscrypt_read_workqueue; > static DEFINE_MUTEX(fscrypt_init_mutex); > > static struct kmem_cache *fscrypt_ctx_cachep; > @@ -330,8 +329,7 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk, > { > struct fscrypt_ctx *ctx; > struct page *ciphertext_page = NULL; > - struct bio *bio; > - int ret, err = 0; > + int err = 0; > > BUG_ON(inode->i_sb->s_blocksize != PAGE_SIZE); > > @@ -349,33 +347,10 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk, > err = do_page_crypto(inode, FS_ENCRYPT, lblk, > ZERO_PAGE(0), ciphertext_page, > PAGE_SIZE, 0, GFP_NOFS); > + err = fscrypt_bio_submit_page(inode, pblk, ciphertext_page); Any specific reason why you didn't just move the whole fscrypt_zeroout_range() to bio.c? It's quite useless without having CONFIG_BLOCK=y. This also would then save you the #ifdef CONFIG_BLOCK in fs/crypto/fscrypt_private.h below. Apart from that, the rest looks good to me. :) - David > if (err) > goto errout; > > - bio = bio_alloc(GFP_NOWAIT, 1); > - if (!bio) { > - err = -ENOMEM; > - goto errout; > - } > - bio->bi_bdev = inode->i_sb->s_bdev; > - bio->bi_iter.bi_sector = > - pblk << (inode->i_sb->s_blocksize_bits - 9); > - bio_set_op_attrs(bio, REQ_OP_WRITE, 0); > - ret = bio_add_page(bio, ciphertext_page, > - inode->i_sb->s_blocksize, 0); > - if (ret != inode->i_sb->s_blocksize) { > - /* should never happen! */ > - WARN_ON(1); > - bio_put(bio); > - err = -EIO; > - goto errout; > - } > - err = submit_bio_wait(bio); > - if ((err == 0) && bio->bi_error) > - err = -EIO; > - bio_put(bio); > - if (err) > - goto errout; > lblk++; > pblk++; > } > @@ -442,64 +417,6 @@ const struct dentry_operations fscrypt_d_ops = { > }; > EXPORT_SYMBOL(fscrypt_d_ops); > > -/* > - * Call fscrypt_decrypt_page on every single page, reusing the encryption > - * context. > - */ > -static void completion_pages(struct work_struct *work) > -{ > - struct fscrypt_ctx *ctx = > - container_of(work, struct fscrypt_ctx, r.work); > - struct bio *bio = ctx->r.bio; > - struct bio_vec *bv; > - int i; > - > - bio_for_each_segment_all(bv, bio, i) { > - struct page *page = bv->bv_page; > - int ret = fscrypt_decrypt_page(page->mapping->host, page, > - PAGE_SIZE, 0, page->index); > - > - if (ret) { > - WARN_ON_ONCE(1); > - SetPageError(page); > - } else { > - SetPageUptodate(page); > - } > - unlock_page(page); > - } > - fscrypt_release_ctx(ctx); > - bio_put(bio); > -} > - > -void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, struct bio *bio) > -{ > - INIT_WORK(&ctx->r.work, completion_pages); > - ctx->r.bio = bio; > - queue_work(fscrypt_read_workqueue, &ctx->r.work); > -} > -EXPORT_SYMBOL(fscrypt_decrypt_bio_pages); > - > -void fscrypt_pullback_bio_page(struct page **page, bool restore) > -{ > - struct fscrypt_ctx *ctx; > - struct page *bounce_page; > - > - /* The bounce data pages are unmapped. */ > - if ((*page)->mapping) > - return; > - > - /* The bounce data page is unmapped. */ > - bounce_page = *page; > - ctx = (struct fscrypt_ctx *)page_private(bounce_page); > - > - /* restore control page */ > - *page = ctx->w.control_page; > - > - if (restore) > - fscrypt_restore_control_page(bounce_page); > -} > -EXPORT_SYMBOL(fscrypt_pullback_bio_page); > - > void fscrypt_restore_control_page(struct page *page) > { > struct fscrypt_ctx *ctx; > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h > index aeab032d7d35..32a5b3598446 100644 > --- a/fs/crypto/fscrypt_private.h > +++ b/fs/crypto/fscrypt_private.h > @@ -86,8 +86,22 @@ struct fscrypt_completion_result { > > /* crypto.c */ > int fscrypt_initialize(unsigned int cop_flags); > +extern struct workqueue_struct *fscrypt_read_workqueue; > > /* keyinfo.c */ > extern int fscrypt_get_crypt_info(struct inode *); > > +/* bio.c */ > +#ifdef CONFIG_BLOCK > +extern int fscrypt_bio_submit_page(const struct inode *inode, sector_t blk, > + struct page *page); > +#else > +static inline int fscrypt_bio_submit_page(const struct inode *inode, > + sector_t blk, struct page *page) > +{ > + WARN_ON_ONCE(1); > + return -EINVAL; > +} > +#endif > + > #endif /* _FSCRYPT_PRIVATE_H */ > diff --git a/include/linux/fscrypto.h b/include/linux/fscrypto.h > index c074b670aa99..167b0dfd4e98 100644 > --- a/include/linux/fscrypto.h > +++ b/include/linux/fscrypto.h > @@ -174,8 +174,6 @@ extern struct page *fscrypt_encrypt_page(const struct inode *, struct page *, > u64, gfp_t); > extern int fscrypt_decrypt_page(const struct inode *, struct page *, unsigned int, > unsigned int, u64); > -extern void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *, struct bio *); > -extern void fscrypt_pullback_bio_page(struct page **, bool); > extern void fscrypt_restore_control_page(struct page *); > extern int fscrypt_zeroout_range(const struct inode *, pgoff_t, sector_t, > unsigned int); > @@ -201,6 +199,10 @@ extern int fscrypt_fname_disk_to_usr(struct inode *, u32, u32, > const struct fscrypt_str *, struct fscrypt_str *); > extern int fscrypt_fname_usr_to_disk(struct inode *, const struct qstr *, > struct fscrypt_str *); > + > +/* bio.c */ > +extern void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *, struct bio *); > +extern void fscrypt_pullback_bio_page(struct page **, bool); > #endif > > /* crypto.c */ > -- > 2.10.2 >