[f2fs-dev] [PATCH v2 0/2] ext4, f2fs: stop using PG_error for fscrypt and fsverity

[f2fs-dev] [PATCH v2 0/2] ext4, f2fs: stop using PG_error for fscrypt and fsverity

[Eric Biggers]

This series changes ext4 and f2fs to stop using PG_error to track
decryption and verity errors.  This is a step towards freeing up
PG_error for other uses, as discussed at
https://lore.kernel.org/linux-fsdevel/Yn10Iz1mJX1Mu1rv@casper.infradead.org

Note: due to the interdependencies with fs/crypto/ and fs/verity/,
I couldn't split this up into separate patches for each filesystem.
I'd appreciate Acks from the ext4 and f2fs maintainers so that I can
take these patches.  Otherwise I'm not sure how to move them forward.

Changed v1 => v2:
   - Rebased onto v6.0-rc1 and resolved conflicts in f2fs.

Eric Biggers (2):
  fscrypt: stop using PG_error to track error status
  fsverity: stop using PG_error to track error status

 fs/crypto/bio.c         | 16 +++++++----
 fs/ext4/readpage.c      | 16 +++++------
 fs/f2fs/compress.c      | 64 ++++++++++++++++++++---------------------
 fs/f2fs/data.c          | 64 +++++++++++++++++++++++------------------
 fs/verity/verify.c      | 12 ++++----
 include/linux/fscrypt.h |  5 ++--
 6 files changed, 93 insertions(+), 84 deletions(-)


base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
-- 
2.37.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v2 1/2] fscrypt: stop using PG_error to track error status

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

As a step towards freeing the PG_error flag for other uses, change ext4
and f2fs to stop using PG_error to track decryption errors.  Instead, if
a decryption error occurs, just mark the whole bio as failed.  The
coarser granularity isn't really a problem since it isn't any worse than
what the block layer provides, and errors from a multi-page readahead
aren't reported to applications unless a single-page read fails too.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/crypto/bio.c         | 16 ++++++++++------
 fs/ext4/readpage.c      |  8 +++++---
 fs/f2fs/data.c          | 18 ++++++++++--------
 include/linux/fscrypt.h |  5 +++--
 4 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
index 2217fe5ece6f9b..1b4403136d05c0 100644
--- a/fs/crypto/bio.c
+++ b/fs/crypto/bio.c
@@ -25,21 +25,25 @@
  * then this function isn't applicable.  This function may sleep, so it must be
  * called from a workqueue rather than from the bio's bi_end_io callback.
  *
- * This function sets PG_error on any pages that contain any blocks that failed
- * to be decrypted.  The filesystem must not mark such pages uptodate.
+ * Return: %true on success; %false on failure.  On failure, bio->bi_status is
+ *	   also set to an error status.
  */
-void fscrypt_decrypt_bio(struct bio *bio)
+bool fscrypt_decrypt_bio(struct bio *bio)
 {
 	struct bio_vec *bv;
 	struct bvec_iter_all iter_all;
 
 	bio_for_each_segment_all(bv, bio, iter_all) {
 		struct page *page = bv->bv_page;
-		int ret = fscrypt_decrypt_pagecache_blocks(page, bv->bv_len,
+		int err = fscrypt_decrypt_pagecache_blocks(page, bv->bv_len,
 							   bv->bv_offset);
-		if (ret)
-			SetPageError(page);
+
+		if (err) {
+			bio->bi_status = errno_to_blk_status(err);
+			return false;
+		}
 	}
+	return true;
 }
 EXPORT_SYMBOL(fscrypt_decrypt_bio);
 
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index e02a5f14e0211e..5ce4706f68a7c6 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -96,10 +96,12 @@ static void decrypt_work(struct work_struct *work)
 {
 	struct bio_post_read_ctx *ctx =
 		container_of(work, struct bio_post_read_ctx, work);
+	struct bio *bio = ctx->bio;
 
-	fscrypt_decrypt_bio(ctx->bio);
-
-	bio_post_read_processing(ctx);
+	if (fscrypt_decrypt_bio(bio))
+		bio_post_read_processing(ctx);
+	else
+		__read_end_io(bio);
 }
 
 static void verity_work(struct work_struct *work)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index aa3ccddfa03761..93cc2ec51c2aeb 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -139,7 +139,7 @@ static void f2fs_finish_read_bio(struct bio *bio, bool in_task)
 			continue;
 		}
 
-		/* PG_error was set if decryption or verity failed. */
+		/* PG_error was set if verity failed. */
 		if (bio->bi_status || PageError(page)) {
 			ClearPageUptodate(page);
 			/* will re-read again later */
@@ -185,7 +185,7 @@ static void f2fs_verify_bio(struct work_struct *work)
 			struct page *page = bv->bv_page;
 
 			if (!f2fs_is_compressed_page(page) &&
-			    !PageError(page) && !fsverity_verify_page(page))
+			    !fsverity_verify_page(page))
 				SetPageError(page);
 		}
 	} else {
@@ -236,10 +236,9 @@ static void f2fs_handle_step_decompress(struct bio_post_read_ctx *ctx,
 	bio_for_each_segment_all(bv, ctx->bio, iter_all) {
 		struct page *page = bv->bv_page;
 
-		/* PG_error was set if decryption failed. */
 		if (f2fs_is_compressed_page(page))
-			f2fs_end_read_compressed_page(page, PageError(page),
-						blkaddr, in_task);
+			f2fs_end_read_compressed_page(page, false, blkaddr,
+						      in_task);
 		else
 			all_compressed = false;
 
@@ -259,14 +258,17 @@ static void f2fs_post_read_work(struct work_struct *work)
 {
 	struct bio_post_read_ctx *ctx =
 		container_of(work, struct bio_post_read_ctx, work);
+	struct bio *bio = ctx->bio;
 
-	if (ctx->enabled_steps & STEP_DECRYPT)
-		fscrypt_decrypt_bio(ctx->bio);
+	if ((ctx->enabled_steps & STEP_DECRYPT) && !fscrypt_decrypt_bio(bio)) {
+		f2fs_finish_read_bio(bio, true);
+		return;
+	}
 
 	if (ctx->enabled_steps & STEP_DECOMPRESS)
 		f2fs_handle_step_decompress(ctx, true);
 
-	f2fs_verify_and_finish_bio(ctx->bio, true);
+	f2fs_verify_and_finish_bio(bio, true);
 }
 
 static void f2fs_read_end_io(struct bio *bio)
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 7d2f1e0f23b1fe..10c645685b32c2 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -353,7 +353,7 @@ u64 fscrypt_fname_siphash(const struct inode *dir, const struct qstr *name);
 int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags);
 
 /* bio.c */
-void fscrypt_decrypt_bio(struct bio *bio);
+bool fscrypt_decrypt_bio(struct bio *bio);
 int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 			  sector_t pblk, unsigned int len);
 
@@ -646,8 +646,9 @@ static inline int fscrypt_d_revalidate(struct dentry *dentry,
 }
 
 /* bio.c */
-static inline void fscrypt_decrypt_bio(struct bio *bio)
+static inline bool fscrypt_decrypt_bio(struct bio *bio)
 {
+	return true;
 }
 
 static inline int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
-- 
2.37.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v2 2/2] fsverity: stop using PG_error to track error status

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

As a step towards freeing the PG_error flag for other uses, change ext4
and f2fs to stop using PG_error to track verity errors.  Instead, if a
verity error occurs, just mark the whole bio as failed.  The coarser
granularity isn't really a problem since it isn't any worse than what
the block layer provides, and errors from a multi-page readahead aren't
reported to applications unless a single-page read fails too.

f2fs supports compression, which makes the f2fs changes a bit more
complicated than desired, but the basic premise still works.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/readpage.c |  8 ++----
 fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
 fs/f2fs/data.c     | 52 ++++++++++++++++++++-----------------
 fs/verity/verify.c | 12 ++++-----
 4 files changed, 68 insertions(+), 68 deletions(-)

diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index 5ce4706f68a7c6..e604ea4e102b71 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -75,14 +75,10 @@ static void __read_end_io(struct bio *bio)
 	bio_for_each_segment_all(bv, bio, iter_all) {
 		page = bv->bv_page;
 
-		/* PG_error was set if any post_read step failed */
-		if (bio->bi_status || PageError(page)) {
+		if (bio->bi_status)
 			ClearPageUptodate(page);
-			/* will re-read again later */
-			ClearPageError(page);
-		} else {
+		else
 			SetPageUptodate(page);
-		}
 		unlock_page(page);
 	}
 	if (bio->bi_private)
diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 70e97075e535e5..f54fb3bb74197a 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1715,50 +1715,27 @@ static void f2fs_put_dic(struct decompress_io_ctx *dic, bool in_task)
 	}
 }
 
-/*
- * Update and unlock the cluster's pagecache pages, and release the reference to
- * the decompress_io_ctx that was being held for I/O completion.
- */
-static void __f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
-				bool in_task)
+static void f2fs_verify_cluster(struct work_struct *work)
 {
+	struct decompress_io_ctx *dic =
+		container_of(work, struct decompress_io_ctx, verity_work);
 	int i;
 
+	/* Verify, update, and unlock the decompressed pages. */
 	for (i = 0; i < dic->cluster_size; i++) {
 		struct page *rpage = dic->rpages[i];
 
 		if (!rpage)
 			continue;
 
-		/* PG_error was set if verity failed. */
-		if (failed || PageError(rpage)) {
-			ClearPageUptodate(rpage);
-			/* will re-read again later */
-			ClearPageError(rpage);
-		} else {
+		if (fsverity_verify_page(rpage))
 			SetPageUptodate(rpage);
-		}
+		else
+			ClearPageUptodate(rpage);
 		unlock_page(rpage);
 	}
 
-	f2fs_put_dic(dic, in_task);
-}
-
-static void f2fs_verify_cluster(struct work_struct *work)
-{
-	struct decompress_io_ctx *dic =
-		container_of(work, struct decompress_io_ctx, verity_work);
-	int i;
-
-	/* Verify the cluster's decompressed pages with fs-verity. */
-	for (i = 0; i < dic->cluster_size; i++) {
-		struct page *rpage = dic->rpages[i];
-
-		if (rpage && !fsverity_verify_page(rpage))
-			SetPageError(rpage);
-	}
-
-	__f2fs_decompress_end_io(dic, false, true);
+	f2fs_put_dic(dic, true);
 }
 
 /*
@@ -1768,6 +1745,8 @@ static void f2fs_verify_cluster(struct work_struct *work)
 void f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
 				bool in_task)
 {
+	int i;
+
 	if (!failed && dic->need_verity) {
 		/*
 		 * Note that to avoid deadlocks, the verity work can't be done
@@ -1777,9 +1756,28 @@ void f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
 		 */
 		INIT_WORK(&dic->verity_work, f2fs_verify_cluster);
 		fsverity_enqueue_verify_work(&dic->verity_work);
-	} else {
-		__f2fs_decompress_end_io(dic, failed, in_task);
+		return;
+	}
+
+	/* Update and unlock the cluster's pagecache pages. */
+	for (i = 0; i < dic->cluster_size; i++) {
+		struct page *rpage = dic->rpages[i];
+
+		if (!rpage)
+			continue;
+
+		if (failed)
+			ClearPageUptodate(rpage);
+		else
+			SetPageUptodate(rpage);
+		unlock_page(rpage);
 	}
+
+	/*
+	 * Release the reference to the decompress_io_ctx that was being held
+	 * for I/O completion.
+	 */
+	f2fs_put_dic(dic, in_task);
 }
 
 /*
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 93cc2ec51c2aeb..34af260975a2e6 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -119,34 +119,41 @@ struct bio_post_read_ctx {
 	block_t fs_blkaddr;
 };
 
-static void f2fs_finish_read_bio(struct bio *bio, bool in_task)
+/*
+ * Update and unlock a bio's pages, and free the bio.
+ *
+ * This marks pages up-to-date only if there was no error in the bio (I/O error,
+ * decryption error, or verity error), as indicated by bio->bi_status.
+ *
+ * "Compressed pages" (pagecache pages backed by a compressed cluster on-disk)
+ * aren't marked up-to-date here, as decompression is done on a per-compression-
+ * cluster basis rather than a per-bio basis.  Instead, we only must do two
+ * things for each compressed page here: call f2fs_end_read_compressed_page()
+ * with failed=true if an error occurred before it would have normally gotten
+ * called (i.e., I/O error or decryption error, but *not* verity error), and
+ * release the bio's reference to the decompress_io_ctx of the page's cluster.
+ */
+static void f2fs_finish_read_bio(struct bio *bio, bool in_task,
+				 bool fail_compressed)
 {
 	struct bio_vec *bv;
 	struct bvec_iter_all iter_all;
 
-	/*
-	 * Update and unlock the bio's pagecache pages, and put the
-	 * decompression context for any compressed pages.
-	 */
 	bio_for_each_segment_all(bv, bio, iter_all) {
 		struct page *page = bv->bv_page;
 
 		if (f2fs_is_compressed_page(page)) {
-			if (bio->bi_status)
+			if (fail_compressed)
 				f2fs_end_read_compressed_page(page, true, 0,
 							in_task);
 			f2fs_put_page_dic(page, in_task);
 			continue;
 		}
 
-		/* PG_error was set if verity failed. */
-		if (bio->bi_status || PageError(page)) {
+		if (bio->bi_status)
 			ClearPageUptodate(page);
-			/* will re-read again later */
-			ClearPageError(page);
-		} else {
+		else
 			SetPageUptodate(page);
-		}
 		dec_page_count(F2FS_P_SB(page), __read_io_type(page));
 		unlock_page(page);
 	}
@@ -185,14 +192,17 @@ static void f2fs_verify_bio(struct work_struct *work)
 			struct page *page = bv->bv_page;
 
 			if (!f2fs_is_compressed_page(page) &&
-			    !fsverity_verify_page(page))
-				SetPageError(page);
+			    !fsverity_verify_page(page)) {
+				bio->bi_status = BLK_STS_IOERR;
+				break;
+			}
 		}
 	} else {
 		fsverity_verify_bio(bio);
 	}
 
-	f2fs_finish_read_bio(bio, true);
+	f2fs_finish_read_bio(bio, true /* in_task */,
+			     false /* fail_compressed */);
 }
 
 /*
@@ -212,7 +222,7 @@ static void f2fs_verify_and_finish_bio(struct bio *bio, bool in_task)
 		INIT_WORK(&ctx->work, f2fs_verify_bio);
 		fsverity_enqueue_verify_work(&ctx->work);
 	} else {
-		f2fs_finish_read_bio(bio, in_task);
+		f2fs_finish_read_bio(bio, in_task, false /* fail_compressed */);
 	}
 }
 
@@ -261,7 +271,8 @@ static void f2fs_post_read_work(struct work_struct *work)
 	struct bio *bio = ctx->bio;
 
 	if ((ctx->enabled_steps & STEP_DECRYPT) && !fscrypt_decrypt_bio(bio)) {
-		f2fs_finish_read_bio(bio, true);
+		f2fs_finish_read_bio(bio, true /* in_task */,
+				     true /* fail_compressed */);
 		return;
 	}
 
@@ -286,7 +297,7 @@ static void f2fs_read_end_io(struct bio *bio)
 	}
 
 	if (bio->bi_status) {
-		f2fs_finish_read_bio(bio, intask);
+		f2fs_finish_read_bio(bio, intask, true /* fail_compressed */);
 		return;
 	}
 
@@ -1083,7 +1094,6 @@ static int f2fs_submit_page_read(struct inode *inode, struct page *page,
 		bio_put(bio);
 		return -EFAULT;
 	}
-	ClearPageError(page);
 	inc_page_count(sbi, F2FS_RD_DATA);
 	f2fs_update_iostat(sbi, FS_DATA_READ_IO, F2FS_BLKSIZE);
 	__submit_bio(sbi, bio, DATA);
@@ -2125,7 +2135,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
 
 	inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
 	f2fs_update_iostat(F2FS_I_SB(inode), FS_DATA_READ_IO, F2FS_BLKSIZE);
-	ClearPageError(page);
 	*last_block_in_bio = block_nr;
 	goto out;
 out:
@@ -2274,7 +2283,6 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 		inc_page_count(sbi, F2FS_RD_DATA);
 		f2fs_update_iostat(sbi, FS_DATA_READ_IO, F2FS_BLKSIZE);
 		f2fs_update_iostat(sbi, FS_CDATA_READ_IO, F2FS_BLKSIZE);
-		ClearPageError(page);
 		*last_block_in_bio = blkaddr;
 	}
 
@@ -2291,7 +2299,6 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
 	for (i = 0; i < cc->cluster_size; i++) {
 		if (cc->rpages[i]) {
 			ClearPageUptodate(cc->rpages[i]);
-			ClearPageError(cc->rpages[i]);
 			unlock_page(cc->rpages[i]);
 		}
 	}
@@ -2388,7 +2395,6 @@ static int f2fs_mpage_readpages(struct inode *inode,
 #ifdef CONFIG_F2FS_FS_COMPRESSION
 set_error_page:
 #endif
-			SetPageError(page);
 			zero_user_segment(page, 0, PAGE_SIZE);
 			unlock_page(page);
 		}
diff --git a/fs/verity/verify.c b/fs/verity/verify.c
index 14e2fb49cff561..556dfbd4698dea 100644
--- a/fs/verity/verify.c
+++ b/fs/verity/verify.c
@@ -210,9 +210,8 @@ EXPORT_SYMBOL_GPL(fsverity_verify_page);
  * @bio: the bio to verify
  *
  * Verify a set of pages that have just been read from a verity file.  The pages
- * must be pagecache pages that are still locked and not yet uptodate.  Pages
- * that fail verification are set to the Error state.  Verification is skipped
- * for pages already in the Error state, e.g. due to fscrypt decryption failure.
+ * must be pagecache pages that are still locked and not yet uptodate.  If a
+ * page fails verification, then bio->bi_status is set to an error status.
  *
  * This is a helper function for use by the ->readahead() method of filesystems
  * that issue bios to read data directly into the page cache.  Filesystems that
@@ -254,9 +253,10 @@ void fsverity_verify_bio(struct bio *bio)
 		unsigned long level0_ra_pages =
 			min(max_ra_pages, params->level0_blocks - level0_index);
 
-		if (!PageError(page) &&
-		    !verify_page(inode, vi, req, page, level0_ra_pages))
-			SetPageError(page);
+		if (!verify_page(inode, vi, req, page, level0_ra_pages)) {
+			bio->bi_status = BLK_STS_IOERR;
+			break;
+		}
 	}
 
 	fsverity_free_hash_request(params->hash_alg, req);
-- 
2.37.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 0/2] ext4, f2fs: stop using PG_error for fscrypt and fsverity

[Eric Biggers]

On Mon, Aug 15, 2022 at 04:50:50PM -0700, Eric Biggers wrote:
> This series changes ext4 and f2fs to stop using PG_error to track
> decryption and verity errors.  This is a step towards freeing up
> PG_error for other uses, as discussed at
> https://lore.kernel.org/linux-fsdevel/Yn10Iz1mJX1Mu1rv@casper.infradead.org
> 
> Note: due to the interdependencies with fs/crypto/ and fs/verity/,
> I couldn't split this up into separate patches for each filesystem.
> I'd appreciate Acks from the ext4 and f2fs maintainers so that I can
> take these patches.  Otherwise I'm not sure how to move them forward.
> 
> Changed v1 => v2:
>    - Rebased onto v6.0-rc1 and resolved conflicts in f2fs.
> 
> Eric Biggers (2):
>   fscrypt: stop using PG_error to track error status
>   fsverity: stop using PG_error to track error status
> 
>  fs/crypto/bio.c         | 16 +++++++----
>  fs/ext4/readpage.c      | 16 +++++------
>  fs/f2fs/compress.c      | 64 ++++++++++++++++++++---------------------
>  fs/f2fs/data.c          | 64 +++++++++++++++++++++++------------------
>  fs/verity/verify.c      | 12 ++++----
>  include/linux/fscrypt.h |  5 ++--
>  6 files changed, 93 insertions(+), 84 deletions(-)
> 
> 
> base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868

I'd appreciate review from the f2fs folks on this series, as that's where the
most complex changes are.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 0/2] ext4, f2fs: stop using PG_error for fscrypt and fsverity

[Eric Biggers]

On Mon, Aug 22, 2022 at 11:27:19AM -0700, Eric Biggers wrote:
> On Mon, Aug 15, 2022 at 04:50:50PM -0700, Eric Biggers wrote:
> > This series changes ext4 and f2fs to stop using PG_error to track
> > decryption and verity errors.  This is a step towards freeing up
> > PG_error for other uses, as discussed at
> > https://lore.kernel.org/linux-fsdevel/Yn10Iz1mJX1Mu1rv@casper.infradead.org
> > 
> > Note: due to the interdependencies with fs/crypto/ and fs/verity/,
> > I couldn't split this up into separate patches for each filesystem.
> > I'd appreciate Acks from the ext4 and f2fs maintainers so that I can
> > take these patches.  Otherwise I'm not sure how to move them forward.
> > 
> > Changed v1 => v2:
> >    - Rebased onto v6.0-rc1 and resolved conflicts in f2fs.
> > 
> > Eric Biggers (2):
> >   fscrypt: stop using PG_error to track error status
> >   fsverity: stop using PG_error to track error status
> > 
> >  fs/crypto/bio.c         | 16 +++++++----
> >  fs/ext4/readpage.c      | 16 +++++------
> >  fs/f2fs/compress.c      | 64 ++++++++++++++++++++---------------------
> >  fs/f2fs/data.c          | 64 +++++++++++++++++++++++------------------
> >  fs/verity/verify.c      | 12 ++++----
> >  include/linux/fscrypt.h |  5 ++--
> >  6 files changed, 93 insertions(+), 84 deletions(-)
> > 
> > 
> > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
> 
> I'd appreciate review from the f2fs folks on this series, as that's where the
> most complex changes are.
> 

There's already a merge conflict with f2fs/dev, in the second patch :-(

It's going to be hard get this series merged, due to cross-tree dependencies.

I'll try to take the first patch (which handles decryption only, and is smaller)
through the fscrypt tree for 6.1.  Then maybe the second patch can go through
the f2fs tree later.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 1/2] fscrypt: stop using PG_error to track error status

[Chao Yu]

On 2022/8/16 7:50, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> As a step towards freeing the PG_error flag for other uses, change ext4
> and f2fs to stop using PG_error to track decryption errors.  Instead, if
> a decryption error occurs, just mark the whole bio as failed.  The
> coarser granularity isn't really a problem since it isn't any worse than
> what the block layer provides, and errors from a multi-page readahead
> aren't reported to applications unless a single-page read fails too.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>   fs/crypto/bio.c         | 16 ++++++++++------
>   fs/ext4/readpage.c      |  8 +++++---
>   fs/f2fs/data.c          | 18 ++++++++++--------
>   include/linux/fscrypt.h |  5 +++--
>   4 files changed, 28 insertions(+), 19 deletions(-)

For f2fs part,

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 2/2] fsverity: stop using PG_error to track error status

[Chao Yu]

On 2022/8/16 7:50, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> As a step towards freeing the PG_error flag for other uses, change ext4
> and f2fs to stop using PG_error to track verity errors.  Instead, if a
> verity error occurs, just mark the whole bio as failed.  The coarser
> granularity isn't really a problem since it isn't any worse than what
> the block layer provides, and errors from a multi-page readahead aren't
> reported to applications unless a single-page read fails too.
> 
> f2fs supports compression, which makes the f2fs changes a bit more
> complicated than desired, but the basic premise still works.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>   fs/ext4/readpage.c |  8 ++----
>   fs/f2fs/compress.c | 64 ++++++++++++++++++++++------------------------
>   fs/f2fs/data.c     | 52 ++++++++++++++++++++-----------------
>   fs/verity/verify.c | 12 ++++-----
>   4 files changed, 68 insertions(+), 68 deletions(-)
> 
> diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> index 5ce4706f68a7c6..e604ea4e102b71 100644
> --- a/fs/ext4/readpage.c
> +++ b/fs/ext4/readpage.c
> @@ -75,14 +75,10 @@ static void __read_end_io(struct bio *bio)
>   	bio_for_each_segment_all(bv, bio, iter_all) {
>   		page = bv->bv_page;
>   
> -		/* PG_error was set if any post_read step failed */
> -		if (bio->bi_status || PageError(page)) {
> +		if (bio->bi_status)
>   			ClearPageUptodate(page);
> -			/* will re-read again later */
> -			ClearPageError(page);
> -		} else {
> +		else
>   			SetPageUptodate(page);
> -		}
>   		unlock_page(page);
>   	}
>   	if (bio->bi_private)
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 70e97075e535e5..f54fb3bb74197a 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1715,50 +1715,27 @@ static void f2fs_put_dic(struct decompress_io_ctx *dic, bool in_task)
>   	}
>   }
>   
> -/*
> - * Update and unlock the cluster's pagecache pages, and release the reference to
> - * the decompress_io_ctx that was being held for I/O completion.
> - */
> -static void __f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
> -				bool in_task)
> +static void f2fs_verify_cluster(struct work_struct *work)
>   {
> +	struct decompress_io_ctx *dic =
> +		container_of(work, struct decompress_io_ctx, verity_work);
>   	int i;
>   
> +	/* Verify, update, and unlock the decompressed pages. */
>   	for (i = 0; i < dic->cluster_size; i++) {
>   		struct page *rpage = dic->rpages[i];
>   
>   		if (!rpage)
>   			continue;
>   
> -		/* PG_error was set if verity failed. */
> -		if (failed || PageError(rpage)) {
> -			ClearPageUptodate(rpage);
> -			/* will re-read again later */
> -			ClearPageError(rpage);
> -		} else {
> +		if (fsverity_verify_page(rpage))
>   			SetPageUptodate(rpage);
> -		}
> +		else
> +			ClearPageUptodate(rpage);
>   		unlock_page(rpage);
>   	}
>   
> -	f2fs_put_dic(dic, in_task);
> -}
> -
> -static void f2fs_verify_cluster(struct work_struct *work)
> -{
> -	struct decompress_io_ctx *dic =
> -		container_of(work, struct decompress_io_ctx, verity_work);
> -	int i;
> -
> -	/* Verify the cluster's decompressed pages with fs-verity. */
> -	for (i = 0; i < dic->cluster_size; i++) {
> -		struct page *rpage = dic->rpages[i];
> -
> -		if (rpage && !fsverity_verify_page(rpage))
> -			SetPageError(rpage);
> -	}
> -
> -	__f2fs_decompress_end_io(dic, false, true);
> +	f2fs_put_dic(dic, true);
>   }
>   
>   /*
> @@ -1768,6 +1745,8 @@ static void f2fs_verify_cluster(struct work_struct *work)
>   void f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
>   				bool in_task)
>   {
> +	int i;
> +
>   	if (!failed && dic->need_verity) {
>   		/*
>   		 * Note that to avoid deadlocks, the verity work can't be done
> @@ -1777,9 +1756,28 @@ void f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
>   		 */
>   		INIT_WORK(&dic->verity_work, f2fs_verify_cluster);
>   		fsverity_enqueue_verify_work(&dic->verity_work);
> -	} else {
> -		__f2fs_decompress_end_io(dic, failed, in_task);

Will it be possible to clean up __f2fs_decompress_end_io() and
f2fs_verify_cluster(), they looks almost similar...

> +		return;
> +	}
> +
> +	/* Update and unlock the cluster's pagecache pages. */
> +	for (i = 0; i < dic->cluster_size; i++) {
> +		struct page *rpage = dic->rpages[i];
> +
> +		if (!rpage)
> +			continue;
> +
> +		if (failed)
> +			ClearPageUptodate(rpage);
> +		else
> +			SetPageUptodate(rpage);
> +		unlock_page(rpage);
>   	}
> +
> +	/*
> +	 * Release the reference to the decompress_io_ctx that was being held
> +	 * for I/O completion.
> +	 */
> +	f2fs_put_dic(dic, in_task);
>   }
>   
>   /*
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 93cc2ec51c2aeb..34af260975a2e6 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -119,34 +119,41 @@ struct bio_post_read_ctx {
>   	block_t fs_blkaddr;
>   };
>   
> -static void f2fs_finish_read_bio(struct bio *bio, bool in_task)
> +/*
> + * Update and unlock a bio's pages, and free the bio.
> + *
> + * This marks pages up-to-date only if there was no error in the bio (I/O error,
> + * decryption error, or verity error), as indicated by bio->bi_status.
> + *
> + * "Compressed pages" (pagecache pages backed by a compressed cluster on-disk)
> + * aren't marked up-to-date here, as decompression is done on a per-compression-
> + * cluster basis rather than a per-bio basis.  Instead, we only must do two
> + * things for each compressed page here: call f2fs_end_read_compressed_page()
> + * with failed=true if an error occurred before it would have normally gotten
> + * called (i.e., I/O error or decryption error, but *not* verity error), and
> + * release the bio's reference to the decompress_io_ctx of the page's cluster.
> + */
> +static void f2fs_finish_read_bio(struct bio *bio, bool in_task,
> +				 bool fail_compressed)

Not sure, fail_decompress or fail_decompression may looks more readable?

Thanks,

>   {
>   	struct bio_vec *bv;
>   	struct bvec_iter_all iter_all;
>   
> -	/*
> -	 * Update and unlock the bio's pagecache pages, and put the
> -	 * decompression context for any compressed pages.
> -	 */
>   	bio_for_each_segment_all(bv, bio, iter_all) {
>   		struct page *page = bv->bv_page;
>   
>   		if (f2fs_is_compressed_page(page)) {
> -			if (bio->bi_status)
> +			if (fail_compressed)
>   				f2fs_end_read_compressed_page(page, true, 0,
>   							in_task);
>   			f2fs_put_page_dic(page, in_task);
>   			continue;
>   		}
>   
> -		/* PG_error was set if verity failed. */
> -		if (bio->bi_status || PageError(page)) {
> +		if (bio->bi_status)
>   			ClearPageUptodate(page);
> -			/* will re-read again later */
> -			ClearPageError(page);
> -		} else {
> +		else
>   			SetPageUptodate(page);
> -		}
>   		dec_page_count(F2FS_P_SB(page), __read_io_type(page));
>   		unlock_page(page);
>   	}
> @@ -185,14 +192,17 @@ static void f2fs_verify_bio(struct work_struct *work)
>   			struct page *page = bv->bv_page;
>   
>   			if (!f2fs_is_compressed_page(page) &&
> -			    !fsverity_verify_page(page))
> -				SetPageError(page);
> +			    !fsverity_verify_page(page)) {
> +				bio->bi_status = BLK_STS_IOERR;
> +				break;
> +			}
>   		}
>   	} else {
>   		fsverity_verify_bio(bio);
>   	}
>   
> -	f2fs_finish_read_bio(bio, true);
> +	f2fs_finish_read_bio(bio, true /* in_task */,
> +			     false /* fail_compressed */);
>   }
>   
>   /*
> @@ -212,7 +222,7 @@ static void f2fs_verify_and_finish_bio(struct bio *bio, bool in_task)
>   		INIT_WORK(&ctx->work, f2fs_verify_bio);
>   		fsverity_enqueue_verify_work(&ctx->work);
>   	} else {
> -		f2fs_finish_read_bio(bio, in_task);
> +		f2fs_finish_read_bio(bio, in_task, false /* fail_compressed */);
>   	}
>   }
>   
> @@ -261,7 +271,8 @@ static void f2fs_post_read_work(struct work_struct *work)
>   	struct bio *bio = ctx->bio;
>   
>   	if ((ctx->enabled_steps & STEP_DECRYPT) && !fscrypt_decrypt_bio(bio)) {
> -		f2fs_finish_read_bio(bio, true);
> +		f2fs_finish_read_bio(bio, true /* in_task */,
> +				     true /* fail_compressed */);
>   		return;
>   	}
>   
> @@ -286,7 +297,7 @@ static void f2fs_read_end_io(struct bio *bio)
>   	}
>   
>   	if (bio->bi_status) {
> -		f2fs_finish_read_bio(bio, intask);
> +		f2fs_finish_read_bio(bio, intask, true /* fail_compressed */);
>   		return;
>   	}
>   
> @@ -1083,7 +1094,6 @@ static int f2fs_submit_page_read(struct inode *inode, struct page *page,
>   		bio_put(bio);
>   		return -EFAULT;
>   	}
> -	ClearPageError(page);
>   	inc_page_count(sbi, F2FS_RD_DATA);
>   	f2fs_update_iostat(sbi, FS_DATA_READ_IO, F2FS_BLKSIZE);
>   	__submit_bio(sbi, bio, DATA);
> @@ -2125,7 +2135,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>   
>   	inc_page_count(F2FS_I_SB(inode), F2FS_RD_DATA);
>   	f2fs_update_iostat(F2FS_I_SB(inode), FS_DATA_READ_IO, F2FS_BLKSIZE);
> -	ClearPageError(page);
>   	*last_block_in_bio = block_nr;
>   	goto out;
>   out:
> @@ -2274,7 +2283,6 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>   		inc_page_count(sbi, F2FS_RD_DATA);
>   		f2fs_update_iostat(sbi, FS_DATA_READ_IO, F2FS_BLKSIZE);
>   		f2fs_update_iostat(sbi, FS_CDATA_READ_IO, F2FS_BLKSIZE);
> -		ClearPageError(page);
>   		*last_block_in_bio = blkaddr;
>   	}
>   
> @@ -2291,7 +2299,6 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>   	for (i = 0; i < cc->cluster_size; i++) {
>   		if (cc->rpages[i]) {
>   			ClearPageUptodate(cc->rpages[i]);
> -			ClearPageError(cc->rpages[i]);
>   			unlock_page(cc->rpages[i]);
>   		}
>   	}
> @@ -2388,7 +2395,6 @@ static int f2fs_mpage_readpages(struct inode *inode,
>   #ifdef CONFIG_F2FS_FS_COMPRESSION
>   set_error_page:
>   #endif
> -			SetPageError(page);
>   			zero_user_segment(page, 0, PAGE_SIZE);
>   			unlock_page(page);
>   		}
> diff --git a/fs/verity/verify.c b/fs/verity/verify.c
> index 14e2fb49cff561..556dfbd4698dea 100644
> --- a/fs/verity/verify.c
> +++ b/fs/verity/verify.c
> @@ -210,9 +210,8 @@ EXPORT_SYMBOL_GPL(fsverity_verify_page);
>    * @bio: the bio to verify
>    *
>    * Verify a set of pages that have just been read from a verity file.  The pages
> - * must be pagecache pages that are still locked and not yet uptodate.  Pages
> - * that fail verification are set to the Error state.  Verification is skipped
> - * for pages already in the Error state, e.g. due to fscrypt decryption failure.
> + * must be pagecache pages that are still locked and not yet uptodate.  If a
> + * page fails verification, then bio->bi_status is set to an error status.
>    *
>    * This is a helper function for use by the ->readahead() method of filesystems
>    * that issue bios to read data directly into the page cache.  Filesystems that
> @@ -254,9 +253,10 @@ void fsverity_verify_bio(struct bio *bio)
>   		unsigned long level0_ra_pages =
>   			min(max_ra_pages, params->level0_blocks - level0_index);
>   
> -		if (!PageError(page) &&
> -		    !verify_page(inode, vi, req, page, level0_ra_pages))
> -			SetPageError(page);
> +		if (!verify_page(inode, vi, req, page, level0_ra_pages)) {
> +			bio->bi_status = BLK_STS_IOERR;
> +			break;
> +		}
>   	}
>   
>   	fsverity_free_hash_request(params->hash_alg, req);


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v2 2/2] fsverity: stop using PG_error to track error status

[Eric Biggers]

On Tue, Sep 06, 2022 at 11:43:47PM +0800, Chao Yu wrote:
> > @@ -1768,6 +1745,8 @@ static void f2fs_verify_cluster(struct work_struct *work)
> >   void f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
> >   				bool in_task)
> >   {
> > +	int i;
> > +
> >   	if (!failed && dic->need_verity) {
> >   		/*
> >   		 * Note that to avoid deadlocks, the verity work can't be done
> > @@ -1777,9 +1756,28 @@ void f2fs_decompress_end_io(struct decompress_io_ctx *dic, bool failed,
> >   		 */
> >   		INIT_WORK(&dic->verity_work, f2fs_verify_cluster);
> >   		fsverity_enqueue_verify_work(&dic->verity_work);
> > -	} else {
> > -		__f2fs_decompress_end_io(dic, failed, in_task);
> 
> Will it be possible to clean up __f2fs_decompress_end_io() and
> f2fs_verify_cluster(), they looks almost similar...
> 

I feel that it's simpler to keep them separate.

> > +static void f2fs_finish_read_bio(struct bio *bio, bool in_task,
> > +				 bool fail_compressed)
> 
> Not sure, fail_decompress or fail_decompression may looks more readable?
> 

I'll add a field 'bool decompression_attempted' to struct bio_post_read_ctx so
that this extra argument won't be needed.

- Eric


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel