[PATCH] fsverity: don't use bio_first_page_all() in fsverity_verify_bio()

[PATCH] fsverity: don't use bio_first_page_all() in fsverity_verify_bio()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

bio_first_page_all(bio)->mapping->host is not compatible with large
folios, since the first page of the bio is not necessarily the head page
of the folio, and therefore it might not have the mapping pointer set.

Therefore, move the dereference of ->mapping->host into
verify_data_blocks(), which works with a folio.

(Like the commit that this Fixes, this hasn't actually been tested with
large folios yet, since the filesystems that use fs/verity/ don't
support that yet.  But based on code review, I think this is needed.)

Fixes: 5d0f0e57ed90 ("fsverity: support verifying data from large folios")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/verity/verify.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/verity/verify.c b/fs/verity/verify.c
index 702500ef1f348..0b54f94d763e6 100644
--- a/fs/verity/verify.c
+++ b/fs/verity/verify.c
@@ -256,9 +256,10 @@ verify_data_block(struct inode *inode, struct fsverity_info *vi,
 }
 
 static bool
-verify_data_blocks(struct inode *inode, struct folio *data_folio,
-		   size_t len, size_t offset, unsigned long max_ra_pages)
+verify_data_blocks(struct folio *data_folio, size_t len, size_t offset,
+		   unsigned long max_ra_pages)
 {
+	struct inode *inode = data_folio->mapping->host;
 	struct fsverity_info *vi = inode->i_verity_info;
 	const unsigned int block_size = vi->tree_params.block_size;
 	u64 pos = (u64)data_folio->index << PAGE_SHIFT;
@@ -298,7 +299,7 @@ verify_data_blocks(struct inode *inode, struct folio *data_folio,
  */
 bool fsverity_verify_blocks(struct folio *folio, size_t len, size_t offset)
 {
-	return verify_data_blocks(folio->mapping->host, folio, len, offset, 0);
+	return verify_data_blocks(folio, len, offset, 0);
 
 }
 EXPORT_SYMBOL_GPL(fsverity_verify_blocks);
@@ -320,7 +321,6 @@ EXPORT_SYMBOL_GPL(fsverity_verify_blocks);
  */
 void fsverity_verify_bio(struct bio *bio)
 {
-	struct inode *inode = bio_first_page_all(bio)->mapping->host;
 	struct folio_iter fi;
 	unsigned long max_ra_pages = 0;
 
@@ -338,7 +338,7 @@ void fsverity_verify_bio(struct bio *bio)
 	}
 
 	bio_for_each_folio_all(fi, bio) {
-		if (!verify_data_blocks(inode, fi.folio, fi.length, fi.offset,
+		if (!verify_data_blocks(fi.folio, fi.length, fi.offset,
 					max_ra_pages)) {
 			bio->bi_status = BLK_STS_IOERR;
 			break;

base-commit: c61c38330e582e664fdb97bcb9faf9fa0e4ee175
-- 
2.41.0

Re: [PATCH] fsverity: don't use bio_first_page_all() in fsverity_verify_bio()

[Matthew Wilcox]

On Sat, Jun 03, 2023 at 07:21:01PM -0700, Eric Biggers wrote:
> bio_first_page_all(bio)->mapping->host is not compatible with large
> folios, since the first page of the bio is not necessarily the head page
> of the folio, and therefore it might not have the mapping pointer set.

Yes, that's true.  It is going to depend on the filesystem, since these
two bios are equivalent:

(folio->page[0], offset=0x4000, len=0x300)
(folio->page[4], offset=0, len=0x300)

and we don't yet have a rule that filesystems must construct one or the
other.  We probably _should_, but that was pretty low down my list of
things to care about right now.

> Therefore, move the dereference of ->mapping->host into
> verify_data_blocks(), which works with a folio.

Seems reasonable.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

> @@ -320,7 +321,6 @@ EXPORT_SYMBOL_GPL(fsverity_verify_blocks);
>   */
>  void fsverity_verify_bio(struct bio *bio)
>  {
> -	struct inode *inode = bio_first_page_all(bio)->mapping->host;

An alternative fix could be

	struct folio *first = page_folio(bio_first_page_all(bio));
	struct inode *inode = first->mapping->host;

Or we could add a bio_first_folio_all() that wraps that for you.