On Tuesday, April 30, 2019 5:30:28 AM IST Eric Biggers wrote:
> Hi Chandan,
>
> On Sun, Apr 28, 2019 at 10:01:10AM +0530, Chandan Rajendra wrote:
> > The "read callbacks" code is used by both Ext4 and F2FS. Hence to
> > remove duplicity, this commit moves the code into
> > include/linux/read_callbacks.h and fs/read_callbacks.c.
> >
> > The corresponding decrypt and verity "work" functions have been moved
> > inside fscrypt and fsverity sources. With these in place, the read
> > callbacks code now has to just invoke enqueue functions provided by
> > fscrypt and fsverity.
> >
> > Signed-off-by: Chandan Rajendra <chandan@xxxxxxxxxxxxx>
> > ---
> > fs/Kconfig | 4 +
> > fs/Makefile | 4 +
> > fs/crypto/Kconfig | 1 +
> > fs/crypto/bio.c | 23 ++---
> > fs/crypto/crypto.c | 17 +--
> > fs/crypto/fscrypt_private.h | 3 +
> > fs/ext4/ext4.h | 2 -
> > fs/ext4/readpage.c | 183 +++++----------------------------
> > fs/ext4/super.c | 9 +-
> > fs/f2fs/data.c | 148 ++++----------------------
> > fs/f2fs/super.c | 9 +-
> > fs/read_callbacks.c | 136 ++++++++++++++++++++++++
> > fs/verity/Kconfig | 1 +
> > fs/verity/verify.c | 12 +++
> > include/linux/fscrypt.h | 20 +---
> > include/linux/read_callbacks.h | 21 ++++
> > 16 files changed, 251 insertions(+), 342 deletions(-)
> > create mode 100644 fs/read_callbacks.c
> > create mode 100644 include/linux/read_callbacks.h
> >
>
> For easier review, can you split this into multiple patches? Ideally the ext4
> and f2fs patches would be separate, but if that's truly not possible due to
> interdependencies it seems you could at least do:
>
> 1. Introduce the read_callbacks.
> 2. Convert encryption to use the read_callbacks.
> 3. Remove union from struct fscrypt_context.
>
> Also: just FYI, fs-verity isn't upstream yet, and in the past few months I
> haven't had much time to work on it. So you might consider arranging your
> series so that initially just fscrypt is supported. That will be useful on its
> own, for block_size < PAGE_SIZE support. Then fsverity can be added later.
>
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index 97f9eb8df713..03084f2dbeaf 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -308,6 +308,10 @@ config NFS_COMMON
> > depends on NFSD || NFS_FS || LOCKD
> > default y
> >
> > +config FS_READ_CALLBACKS
> > + bool
> > + default n
>
> 'default n' is unnecesary, since 'n' is already the default.
>
> > +
> > source "net/sunrpc/Kconfig"
> > source "fs/ceph/Kconfig"
> > source "fs/cifs/Kconfig"
> > diff --git a/fs/Makefile b/fs/Makefile
> > index 9dd2186e74b5..e0c0fce8cf40 100644
> > --- a/fs/Makefile
> > +++ b/fs/Makefile
> > @@ -21,6 +21,10 @@ else
> > obj-y += no-block.o
> > endif
> >
> > +ifeq ($(CONFIG_FS_READ_CALLBACKS),y)
> > +obj-y += read_callbacks.o
> > +endif
> > +
>
> This can be simplified to:
>
> obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
>
> > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> > new file mode 100644
> > index 000000000000..b6d5b95e67d7
> > --- /dev/null
> > +++ b/fs/read_callbacks.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file tracks the state machine that needs to be executed after reading
> > + * data from files that are encrypted and/or have verity metadata associated
> > + * with them.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/mm.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/bio.h>
> > +#include <linux/fscrypt.h>
> > +#include <linux/fsverity.h>
> > +#include <linux/read_callbacks.h>
> > +
> > +#define NUM_PREALLOC_POST_READ_CTXS 128
> > +
> > +static struct kmem_cache *read_callbacks_ctx_cache;
> > +static mempool_t *read_callbacks_ctx_pool;
> > +
> > +/* Read callback state machine steps */
> > +enum read_callbacks_step {
> > + STEP_INITIAL = 0,
> > + STEP_DECRYPT,
> > + STEP_VERITY,
> > +};
> > +
> > +void end_read_callbacks(struct bio *bio)
> > +{
> > + struct page *page;
> > + struct bio_vec *bv;
> > + int i;
> > + struct bvec_iter_all iter_all;
> > +
> > + bio_for_each_segment_all(bv, bio, i, iter_all) {
> > + page = bv->bv_page;
> > +
> > + BUG_ON(bio->bi_status);
> > +
> > + if (!PageError(page))
> > + SetPageUptodate(page);
> > +
> > + unlock_page(page);
> > + }
> > + if (bio->bi_private)
> > + put_read_callbacks_ctx(bio->bi_private);
> > + bio_put(bio);
> > +}
> > +EXPORT_SYMBOL(end_read_callbacks);
>
> end_read_callbacks() is only called by read_callbacks() just below, so it should
> be 'static'.
>
> > +
> > +struct read_callbacks_ctx *get_read_callbacks_ctx(struct inode *inode,
> > + struct bio *bio,
> > + pgoff_t index)
> > +{
> > + unsigned int read_callbacks_steps = 0;
>
> Rename 'read_callbacks_steps' => 'enabled_steps', since it's clear from context.
>
> > + struct read_callbacks_ctx *ctx = NULL;
> > +
> > + if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode))
> > + read_callbacks_steps |= 1 << STEP_DECRYPT;
> > +#ifdef CONFIG_FS_VERITY
> > + if (inode->i_verity_info != NULL &&
> > + (index < ((i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT)))
> > + read_callbacks_steps |= 1 << STEP_VERITY;
> > +#endif
>
> To avoid the #ifdef, this should probably be made a function in fsverity.h.
>
> > + if (read_callbacks_steps) {
> > + ctx = mempool_alloc(read_callbacks_ctx_pool, GFP_NOFS);
> > + if (!ctx)
> > + return ERR_PTR(-ENOMEM);
> > + ctx->bio = bio;
> > + ctx->inode = inode;
> > + ctx->enabled_steps = read_callbacks_steps;
> > + ctx->cur_step = STEP_INITIAL;
> > + bio->bi_private = ctx;
> > + }
> > + return ctx;
> > +}
> > +EXPORT_SYMBOL(get_read_callbacks_ctx);
>
> The callers don't actually use the returned read_callbacks_ctx. Instead, they
> rely on this function storing it in ->bi_private. So, this function should just
> return an error code, and it should be renamed. Perhaps:
>
> int read_callbacks_setup_bio(struct inode *inode, struct bio *bio,
> pgoff_t first_pgoff);
>
> Please rename 'index' to 'first_pgoff' to make it clearer what it is, given that
> a bio can contain many pages.
>
> Please add kerneldoc for this function.
>
I will implement the changes suggested above.
--
chandan
