Message-Id: <5473968.ZdFUMrgSOT@localhost.localdomain>
Date:   Wed, 01 May 2019 19:10:32 +0530
From:   Chandan Rajendra <chandan@...ux.ibm.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-fscrypt@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jaegeuk@...nel.org, yuchao0@...wei.com,
        hch@...radead.org
Subject: Re: [PATCH V2 07/13] Add decryption support for sub-pagesized blocks

Hi Eric,

On Tuesday, April 30, 2019 6:08:18 AM IST Eric Biggers wrote:
> On Sun, Apr 28, 2019 at 10:01:15AM +0530, Chandan Rajendra wrote:
> > To support decryption of sub-pagesized blocks this commit adds code to,
> > 1. Track buffer head in "struct read_callbacks_ctx".
> > 2. Pass buffer head argument to all read callbacks.
> > 3. In the corresponding endio, loop across all the blocks mapped by the
> >    page, decrypting each block in turn.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@...ux.ibm.com>
> > ---
> >  fs/buffer.c                    | 83 +++++++++++++++++++++++++---------
> >  fs/crypto/bio.c                | 50 +++++++++++++-------
> >  fs/crypto/crypto.c             | 19 +++++++-
> >  fs/f2fs/data.c                 |  2 +-
> >  fs/mpage.c                     |  2 +-
> >  fs/read_callbacks.c            | 53 ++++++++++++++--------
> >  include/linux/buffer_head.h    |  1 +
> >  include/linux/read_callbacks.h |  5 +-
> >  8 files changed, 154 insertions(+), 61 deletions(-)
> > 
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index ce357602f471..f324727e24bb 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -45,6 +45,7 @@
> >  #include <linux/bit_spinlock.h>
> >  #include <linux/pagevec.h>
> >  #include <linux/sched/mm.h>
> > +#include <linux/read_callbacks.h>
> >  #include <trace/events/block.h>
> >  
> >  static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);
> > @@ -245,11 +246,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
> >  	return ret;
> >  }
> >  
> > -/*
> > - * I/O completion handler for block_read_full_page() - pages
> > - * which come unlocked at the end of I/O.
> > - */
> > -static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> > +void end_buffer_page_read(struct buffer_head *bh)
> 
> I think __end_buffer_async_read() would be a better name, since the *page* isn't
> necessarily done yet.
> 
> >  {
> >  	unsigned long flags;
> >  	struct buffer_head *first;
> > @@ -257,17 +254,7 @@ static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> >  	struct page *page;
> >  	int page_uptodate = 1;
> >  
> > -	BUG_ON(!buffer_async_read(bh));
> > -
> >  	page = bh->b_page;
> > -	if (uptodate) {
> > -		set_buffer_uptodate(bh);
> > -	} else {
> > -		clear_buffer_uptodate(bh);
> > -		buffer_io_error(bh, ", async page read");
> > -		SetPageError(page);
> > -	}
> > -
> >  	/*
> >  	 * Be _very_ careful from here on. Bad things can happen if
> >  	 * two buffer heads end IO at almost the same time and both
> > @@ -305,6 +292,44 @@ static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> >  	local_irq_restore(flags);
> >  	return;
> >  }
> > +EXPORT_SYMBOL(end_buffer_page_read);
> 
> No need for EXPORT_SYMBOL() here, as this is only called by built-in code.
> 
> > +
> > +/*
> > + * I/O completion handler for block_read_full_page() - pages
> > + * which come unlocked at the end of I/O.
> > + */
> 
> This comment is no longer correct.  Change to something like:
> 
> /*
>  * I/O completion handler for block_read_full_page().  Pages are unlocked after
>  * the I/O completes and the read callbacks (if any) have executed.
>  */
> 
> > +static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> > +{
> > +	struct page *page;
> > +
> > +	BUG_ON(!buffer_async_read(bh));
> > +
> > +#if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> > +	if (uptodate && bh->b_private) {
> > +		struct read_callbacks_ctx *ctx = bh->b_private;
> > +
> > +		read_callbacks(ctx);
> > +		return;
> > +	}
> > +
> > +	if (bh->b_private) {
> > +		struct read_callbacks_ctx *ctx = bh->b_private;
> > +
> > +		WARN_ON(uptodate);
> > +		put_read_callbacks_ctx(ctx);
> > +	}
> > +#endif
> 
> These details should be handled in read_callbacks code, not here.  AFAICS, all
> you need is a function read_callbacks_end_bh() that returns a bool indicating
> whether it handled the buffer_head or not:
> 
> static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
> {
> 	BUG_ON(!buffer_async_read(bh));
> 
> 	if (read_callbacks_end_bh(bh, uptodate))
> 		return;
> 
> 	page = bh->b_page;
> 	...
> }
> 
> Then read_callbacks_end_bh() would check ->b_private and uptodate, and call
> read_callbacks() or put_read_callbacks_ctx() as appropriate.  When
> CONFIG_FS_READ_CALLBACKS=n it would be a stub that always returns false.
> 
> > +	page = bh->b_page;
> [...]
> 
> >  	}
> > @@ -2292,11 +2323,21 @@ int block_read_full_page(struct page *page, get_block_t *get_block)
> >  	 * the underlying blockdev brought it uptodate (the sct fix).
> >  	 */
> >  	for (i = 0; i < nr; i++) {
> > -		bh = arr[i];
> > -		if (buffer_uptodate(bh))
> > +		bh = arr[i].bh;
> > +		if (buffer_uptodate(bh)) {
> >  			end_buffer_async_read(bh, 1);
> > -		else
> > +		} else {
> > +#if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> > +			struct read_callbacks_ctx *ctx;
> > +
> > +			ctx = get_read_callbacks_ctx(inode, NULL, bh, arr[i].blk_nr);
> > +			if (WARN_ON(IS_ERR(ctx))) {
> > +				end_buffer_async_read(bh, 0);
> > +				continue;
> > +			}
> > +#endif
> >  			submit_bh(REQ_OP_READ, 0, bh);
> > +		}
> >  	}
> >  	return 0;
> 
> Similarly here.  This level of detail doesn't need to be exposed outside of the
> read_callbacks code.  Just call read_callbacks_setup_bh() or something, make it
> return an 'err' rather than the read_callbacks_ctx, and make read_callbacks.h
> stub it out when !CONFIG_FS_READ_CALLBACKS.  There should be no #ifdef here.
> 
> > diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> > index 27f5618174f2..856f4694902d 100644
> > --- a/fs/crypto/bio.c
> > +++ b/fs/crypto/bio.c
> > @@ -24,44 +24,62 @@
> >  #include <linux/module.h>
> >  #include <linux/bio.h>
> >  #include <linux/namei.h>
> > +#include <linux/buffer_head.h>
> >  #include <linux/read_callbacks.h>
> >  
> >  #include "fscrypt_private.h"
> >  
> > -static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
> > +static void fscrypt_decrypt(struct bio *bio, struct buffer_head *bh)
> >  {
> > +	struct inode *inode;
> > +	struct page *page;
> >  	struct bio_vec *bv;
> > +	sector_t blk_nr;
> > +	int ret;
> >  	int i;
> >  	struct bvec_iter_all iter_all;
> >  
> > -	bio_for_each_segment_all(bv, bio, i, iter_all) {
> > -		struct page *page = bv->bv_page;
> > -		int ret = fscrypt_decrypt_page(page->mapping->host, page,
> > -				PAGE_SIZE, 0, page->index);
> > +	WARN_ON(!bh && !bio);
> >  
> > +	if (bh) {
> > +		page = bh->b_page;
> > +		inode = page->mapping->host;
> > +
> > +		blk_nr = page->index << (PAGE_SHIFT - inode->i_blkbits);
> > +		blk_nr += (bh_offset(bh) >> inode->i_blkbits);
> > +
> > +		ret = fscrypt_decrypt_page(inode, page, i_blocksize(inode),
> > +					bh_offset(bh), blk_nr);
> >  		if (ret) {
> >  			WARN_ON_ONCE(1);
> >  			SetPageError(page);
> > -		} else if (done) {
> > -			SetPageUptodate(page);
> >  		}
> > -		if (done)
> > -			unlock_page(page);
> > +	} else if (bio) {
> > +		bio_for_each_segment_all(bv, bio, i, iter_all) {
> > +			unsigned int blkbits;
> > +
> > +			page = bv->bv_page;
> > +			inode = page->mapping->host;
> > +			blkbits = inode->i_blkbits;
> > +			blk_nr = page->index << (PAGE_SHIFT - blkbits);
> > +			blk_nr += (bv->bv_offset >> blkbits);
> > +			ret = fscrypt_decrypt_page(page->mapping->host,
> > +						page, bv->bv_len,
> > +						bv->bv_offset, blk_nr);
> > +			if (ret) {
> > +				WARN_ON_ONCE(1);
> > +				SetPageError(page);
> > +			}
> > +		}
> >  	}
> >  }
> 
> For clarity, can you make these two different functions?
> fscrypt_decrypt_bio() and fscrypt_decrypt_bh().
> 
> FYI, the WARN_ON_ONCE() here was removed in the latest fscrypt tree.
> 
> >  
> > -void fscrypt_decrypt_bio(struct bio *bio)
> > -{
> > -	__fscrypt_decrypt_bio(bio, false);
> > -}
> > -EXPORT_SYMBOL(fscrypt_decrypt_bio);
> > -
> >  void fscrypt_decrypt_work(struct work_struct *work)
> >  {
> >  	struct read_callbacks_ctx *ctx =
> >  		container_of(work, struct read_callbacks_ctx, work);
> >  
> > -	fscrypt_decrypt_bio(ctx->bio);
> > +	fscrypt_decrypt(ctx->bio, ctx->bh);
> >  
> >  	read_callbacks(ctx);
> >  }
> > diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> > index ffa9302a7351..4f0d832cae71 100644
> > --- a/fs/crypto/crypto.c
> > +++ b/fs/crypto/crypto.c
> > @@ -305,11 +305,26 @@ EXPORT_SYMBOL(fscrypt_encrypt_page);
> >  int fscrypt_decrypt_page(const struct inode *inode, struct page *page,
> >  			unsigned int len, unsigned int offs, u64 lblk_num)
> >  {
> > +	int i, page_nr_blks;
> > +	int err = 0;
> > +
> >  	if (!(inode->i_sb->s_cop->flags & FS_CFLG_OWN_PAGES))
> >  		BUG_ON(!PageLocked(page));
> >  
> > -	return fscrypt_do_page_crypto(inode, FS_DECRYPT, lblk_num, page, page,
> > -				      len, offs, GFP_NOFS);
> > +	page_nr_blks = len >> inode->i_blkbits;
> > +
> > +	for (i = 0; i < page_nr_blks; i++) {
> > +		err = fscrypt_do_page_crypto(inode, FS_DECRYPT, lblk_num,
> > +					page, page, i_blocksize(inode), offs,
> > +					GFP_NOFS);
> > +		if (err)
> > +			break;
> > +
> > +		++lblk_num;
> > +		offs += i_blocksize(inode);
> > +	}
> > +
> > +	return err;
> >  }
> >  EXPORT_SYMBOL(fscrypt_decrypt_page);
> 
> I was confused by the code calling this until I saw you updated it to handle
> multiple blocks.  Can you please rename it to fscrypt_decrypt_blocks()?  The
> function comment also needs to be updated to clarify what it does now (decrypt a
> contiguous sequence of one or more filesystem blocks in the page).  Also,
> 'lblk_num' should be renamed to 'starting_lblk_num' or similar.
>

fscrypt_decrypt_page() has the same semantics as fscrypt_encrypt_page()
i.e. they decrypt/encrypt contiguous blocks mapped by a page. This was the
reason behind leaving the names unchanged. Please let me know if you still think
that the names of both the functions need to be renamed to
fscrypt_[decrypt|encrypt]_blocks().

> Please also rename fscrypt_do_page_crypto() to fscrypt_crypt_block().

Sure, I will make the change.

> 
> Also, there should be a check that the len and offset are block-aligned:
> 
> 	const unsigned int blocksize = i_blocksize(inode);
> 
>         if (!IS_ALIGNED(len | offs, blocksize))
>                 return -EINVAL;
> 
> >  
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 05430d3650ab..ba437a2085e7 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -527,7 +527,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
> >  	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
> >  
> >  #if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> > -	ctx = get_read_callbacks_ctx(inode, bio, first_idx);
> > +	ctx = get_read_callbacks_ctx(inode, bio, NULL, first_idx);
> >  	if (IS_ERR(ctx)) {
> >  		bio_put(bio);
> >  		return (struct bio *)ctx;
> > diff --git a/fs/mpage.c b/fs/mpage.c
> > index e342b859ee44..0557479fdca4 100644
> > --- a/fs/mpage.c
> > +++ b/fs/mpage.c
> > @@ -348,7 +348,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
> >  			goto confused;
> >  
> >  #if defined(CONFIG_FS_ENCRYPTION) || defined(CONFIG_FS_VERITY)
> > -		ctx = get_read_callbacks_ctx(inode, args->bio, page->index);
> > +		ctx = get_read_callbacks_ctx(inode, args->bio, NULL, page->index);
> >  		if (IS_ERR(ctx)) {
> >  			bio_put(args->bio);
> >  			args->bio = NULL;
> > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> > index 6dea54b0baa9..b3881c525720 100644
> > --- a/fs/read_callbacks.c
> > +++ b/fs/read_callbacks.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/pagemap.h>
> >  #include <linux/bio.h>
> > +#include <linux/buffer_head.h>
> >  #include <linux/fscrypt.h>
> >  #include <linux/fsverity.h>
> >  #include <linux/read_callbacks.h>
> > @@ -24,26 +25,41 @@ enum read_callbacks_step {
> >  	STEP_VERITY,
> >  };
> >  
> > -void end_read_callbacks(struct bio *bio)
> > +void end_read_callbacks(struct bio *bio, struct buffer_head *bh)
> >  {
> > +	struct read_callbacks_ctx *ctx;
> >  	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;
> > +	if (bh) {
> > +		if (!PageError(bh->b_page))
> > +			set_buffer_uptodate(bh);
> >  
> > -		BUG_ON(bio->bi_status);
> > +		ctx = bh->b_private;
> >  
> > -		if (!PageError(page))
> > -			SetPageUptodate(page);
> > +		end_buffer_page_read(bh);
> >  
> > -		unlock_page(page);
> > +		put_read_callbacks_ctx(ctx);
> > +	} else if (bio) {
> > +		bio_for_each_segment_all(bv, bio, i, iter_all) {
> > +			page = bv->bv_page;
> > +
> > +			WARN_ON(bio->bi_status);
> > +
> > +			if (!PageError(page))
> > +				SetPageUptodate(page);
> > +
> > +			unlock_page(page);
> > +		}
> > +		WARN_ON(!bio->bi_private);
> > +
> > +		ctx = bio->bi_private;
> > +		put_read_callbacks_ctx(ctx);
> > +
> > +		bio_put(bio);
> >  	}
> > -	if (bio->bi_private)
> > -		put_read_callbacks_ctx(bio->bi_private);
> > -	bio_put(bio);
> >  }
> >  EXPORT_SYMBOL(end_read_callbacks);
> 
> To make this easier to read, can you split this into end_read_callbacks_bio()
> and end_read_callbacks_bh()?

Sure, I will make the change.
> 
> >  
> > @@ -70,18 +86,21 @@ void read_callbacks(struct read_callbacks_ctx *ctx)
> >  		ctx->cur_step++;
> >  		/* fall-through */
> >  	default:
> > -		end_read_callbacks(ctx->bio);
> > +		end_read_callbacks(ctx->bio, ctx->bh);
> >  	}
> >  }
> >  EXPORT_SYMBOL(read_callbacks);
> >  
> >  struct read_callbacks_ctx *get_read_callbacks_ctx(struct inode *inode,
> >  						struct bio *bio,
> > +						struct buffer_head *bh,
> >  						pgoff_t index)
> >  {
> >  	unsigned int read_callbacks_steps = 0;
> >  	struct read_callbacks_ctx *ctx = NULL;
> >  
> > +	WARN_ON(!bh && !bio);
> > +
> 
> If this condition is true, return an error code; don't continue on.
> 
> >  	if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode))
> >  		read_callbacks_steps |= 1 << STEP_DECRYPT;
> >  #ifdef CONFIG_FS_VERITY
> > @@ -95,11 +114,15 @@ struct read_callbacks_ctx *get_read_callbacks_ctx(struct inode *inode,
> >  		ctx = mempool_alloc(read_callbacks_ctx_pool, GFP_NOFS);
> >  		if (!ctx)
> >  			return ERR_PTR(-ENOMEM);
> > +		ctx->bh = bh;
> >  		ctx->bio = bio;
> >  		ctx->inode = inode;
> >  		ctx->enabled_steps = read_callbacks_steps;
> >  		ctx->cur_step = STEP_INITIAL;
> > -		bio->bi_private = ctx;
> > +		if (bio)
> > +			bio->bi_private = ctx;
> > +		else if (bh)
> > +			bh->b_private = ctx;
> 
> ... and if doing that, then you don't need to check 'else if (bh)' here.

I agree.

> 
> >  	}
> >  	return ctx;
> >  }
> > @@ -111,12 +134,6 @@ void put_read_callbacks_ctx(struct read_callbacks_ctx *ctx)
> >  }
> >  EXPORT_SYMBOL(put_read_callbacks_ctx);
> >  
> > -bool read_callbacks_required(struct bio *bio)
> > -{
> > -	return bio->bi_private && !bio->bi_status;
> > -}
> > -EXPORT_SYMBOL(read_callbacks_required);
> > -
> 
> It's unexpected that the patch series introduces this function,
> only to delete it later.

I had replaced bio_post_read_required() with read_callbacks_required(). I will
remove this since the requirement for post read checking will need to work for
buffer heads as well.

-- 
chandan

Powered by blists - more mailing lists