fs-verity support for XFS with post EOF merkle tree

To: "Darrick J. Wong" <[email protected]>

Provide a new function call so that validation errors can be reported back to the filesystem.

Signed-off-by Darrick J. Wong <[email protected]>

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/verity/verify.c              |  4 ++++
 include/linux/fsverity.h        | 14 ++++++++++++++
 include/trace/events/fsverity.h | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

fs/verity/verify.c +0 -0

index 47a66f088f..ef411cf5d8 100644

fs/verity/verify.c +0 -0

 error:
 	for (; level > 0; level--) {
 		kunmap_local(hblocks[level - 1].addr);

include/linux/fsverity.h +14 -0

index 5bc7280425..b75e232890 100644
@@ -128,6 +128,20 @@
 	 */
 	int (*write_merkle_tree_block)(struct inode *inode, const void *buf,
 				       u64 pos, unsigned int size);
+
+	/**
+	 * Notify the filesystem that file data is corrupt.
+	 *
+	 * @inode: the inode being validated
+	 * @pos: the file position of the invalid data
+	 * @len: the length of the invalid data
+	 *
+	 * This function is called when fs-verity detects that a portion of a
+	 * file's data is inconsistent with the Merkle tree, or a Merkle tree
+	 * block needed to validate the data is inconsistent with the level
+	 * above it.
+	 */
+	void (*file_corrupt)(struct inode *inode, loff_t pos, size_t len);
 };
 
 #ifdef CONFIG_FS_VERITY

include/trace/events/fsverity.h +19 -0

index dab220884b..375fdddac6 100644
@@ -137,6 +137,25 @@
 		__entry->hidx)
 );
 
+TRACE_EVENT(fsverity_file_corrupt,
+	TP_PROTO(const struct inode *inode, loff_t pos, size_t len),
+	TP_ARGS(inode, pos, len),
+	TP_STRUCT__entry(
+		__field(ino_t, ino)
+		__field(loff_t, pos)
+		__field(size_t, len)
+	),
+	TP_fast_assign(
+		__entry->ino = inode->i_ino;
+		__entry->pos = pos;
+		__entry->len = len;
+	),
+	TP_printk("ino %lu pos %llu len %zu",
+		(unsigned long) __entry->ino,
+		__entry->pos,
+		__entry->len)
+);
+
 #endif /* _TRACE_FSVERITY_H */
 
 /* This part must be outside protection */

Signature · 2 lines

-- 
- Andrey

Quoted 1 line

To: "Darrick J. Wong" <[email protected]>

Say ^^^^^^^ what?

Quoted 1 line

To: "Darrick J. Wong" <[email protected]>

Say ^^^^^^^ what?

Quoted 16 lines

Provide a new function call so that validation errors can be reported back to the filesystem.

Signed-off-by Darrick J. Wong <[email protected]>

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/verity/verify.c              |  4 ++++
 include/linux/fsverity.h        | 14 ++++++++++++++
 include/trace/events/fsverity.h | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

fs/verity/verify.c +0 -0

index 47a66f088f..ef411cf5d8 100644

fs/verity/verify.c line 277

Quoted 7 lines

fs/verity/verify.c +4 -0

@@ -271,6 +271,10 @@
 		data_pos, level - 1, params->hash_alg->name, hsize, want_hash,
 		params->hash_alg->name, hsize,
 		level == 0 ? dblock->real_hash : real_hash);
+	trace_fsverity_file_corrupt(inode, data_pos, params->block_size);
+	if (inode->i_sb->s_vop->file_corrupt)
+		inode->i_sb->s_vop->file_corrupt(inode, data_pos,
+						 params->block_size);

If fserror_report[1] gets merged before this series, I think we should add a new FSERR_ type and call fserror_report instead.

https://lore.kernel.org/linux-fsdevel/176826402610.3490369.4378391061533403171.stgit@frogsfrogsfrogs/T/#u

Quoted 3 lines · Darrick J. Wong

Quoted 1 line

To: "Darrick J. Wong" <[email protected]>

Say ^^^^^^^ what?

oh damn, sorry, that's my broken script

Quoted 29 lines · Darrick J. Wong

Quoted 16 lines

Provide a new function call so that validation errors can be reported back to the filesystem.

Signed-off-by Darrick J. Wong <[email protected]>

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/verity/verify.c              |  4 ++++
 include/linux/fsverity.h        | 14 ++++++++++++++
 include/trace/events/fsverity.h | 19 +++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)

fs/verity/verify.c +0 -0

index 47a66f088f..ef411cf5d8 100644

fs/verity/verify.c line 277

Quoted 7 lines

fs/verity/verify.c +4 -0

@@ -271,6 +271,10 @@
 		data_pos, level - 1, params->hash_alg->name, hsize, want_hash,
 		params->hash_alg->name, hsize,
 		level == 0 ? dblock->real_hash : real_hash);
+	trace_fsverity_file_corrupt(inode, data_pos, params->block_size);
+	if (inode->i_sb->s_vop->file_corrupt)
+		inode->i_sb->s_vop->file_corrupt(inode, data_pos,
+						 params->block_size);

If fserror_report[1] gets merged before this series, I think we should add a new FSERR_ type and call fserror_report instead.

Quoted 1 line

https://lore.kernel.org/linux-fsdevel/176826402610.3490369.4378391061533403171.stgit@frogsfrogsfrogs/T/#u

I see, will health monitoring also use these events? I mean if XFS's fsverity need to report corrupting error then

Signature · 1 line

- Andrey

This function will be used by XFS's scrub to force fsverity activation, therefore, to read fsverity context.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/verity/open.c         | 4 ++--
 include/linux/fsverity.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

fs/verity/open.c +0 -0

index 77b1c977af..956ddaffbf 100644

fs/verity/open.c +1 -1

 {
 	struct fsverity_info *vi = fsverity_get_info(inode);
 	struct fsverity_descriptor *desc;

@@ -380,7 +380,7 @@
 {
 	if (filp->f_mode & FMODE_WRITE)
 		return -EPERM;
-	return ensure_verity_info(inode);
+	return fsverity_ensure_verity_info(inode);
 }
 EXPORT_SYMBOL_GPL(__fsverity_file_open);
 

include/linux/fsverity.h +2 -0

index b75e232890..16a6e51705 100644
@@ -196,6 +196,8 @@
 int __fsverity_prepare_setattr(struct dentry *dentry, struct iattr *attr);
 void __fsverity_cleanup_inode(struct inode *inode);
 
+int fsverity_ensure_verity_info(struct inode *inode);
+
 /**
  * fsverity_cleanup_inode() - free the inode's verity info, if present
  * @inode: an inode being evicted

Signature · 2 lines

-- 
- Andrey

fs/iomap/buffered-io.c +3 -2

 			folio_zero_range(folio, poff, plen);
 			iomap_set_range_uptodate(folio, poff, plen);
 		} else {

@@ -1130,13 +1131,14 @@
 		 * unlock and release the folio.
 		 */
 		old_size = iter->inode->i_size;
-		if (pos + written > old_size) {
+		if (pos + written > old_size &&
+		    !(iter->flags & IOMAP_F_BEYOND_EOF)) {
 			i_size_write(iter->inode, pos + written);
 			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
 		}
 		__iomap_put_folio(iter, write_ops, written, folio);
 
-		if (old_size < pos)
+		if (old_size < pos && !(iter->flags & IOMAP_F_BEYOND_EOF))
 			pagecache_isize_extended(iter->inode, old_size, pos);
 
 		cond_resched();

fs/iomap/buffered-io.c +1 -0

+ 		return 0;
 	WARN_ON_ONCE(end_pos <= pos);
 
 	if (i_blocks_per_folio(inode, folio) > 1) {

fs/iomap/trace.h +2 -1

index 532787277b..f1895f7ae5 100644
@@ -118,7 +118,8 @@
 	{ IOMAP_F_ATOMIC_BIO,	"ATOMIC_BIO" }, \
 	{ IOMAP_F_PRIVATE,	"PRIVATE" }, \
 	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }, \
-	{ IOMAP_F_STALE,	"STALE" }
+	{ IOMAP_F_STALE,	"STALE" }, \
+	{ IOMAP_F_BEYOND_EOF,	"BEYOND_EOF" }
 
 
 #define IOMAP_DIO_STRINGS \

include/linux/iomap.h +0 -0

index 520e967cb5..7a7e31c499 100644

include/linux/iomap.h +4 -0

+ */
+#define IOMAP_F_BEYOND_EOF	(1U << 13)
+
+/*
  * Flags set by the core iomap code during operations:
  *
  * IOMAP_F_SIZE_CHANGED indicates to the iomap_end method that the file size

Signature · 2 lines

-- 
- Andrey

fs/iomap/buffered-io.c

Quoted 15 lines · Andrey Albershteyn

Flag to indicate to iomap that read/write is happening beyond EOF and no isize checks/update is needed.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/buffered-io.c | 13 ++++++++-----
 fs/iomap/trace.h       |  3 ++-
 include/linux/iomap.h  |  5 +++++
 3 files changed, 15 insertions(+), 6 deletions(-)

fs/iomap/buffered-io.c +0 -0

index e5c1ca440d..cc1cbf2a4c 100644
@@ -533,7 +533,8 @@

(Does your diff program not set --show-c-function? That makes reviewing harder because I have to search on the comment text to figure out which function this is)

fs/iomap/buffered-io.c line 537

Quoted 6 lines

fs/iomap/buffered-io.c +2 -1

 			return 0;
 
 		/* zero post-eof blocks as the page may be mapped */
-		if (iomap_block_needs_zeroing(iter, pos)) {
+		if (iomap_block_needs_zeroing(iter, pos) &&
+		    !(iomap->flags & IOMAP_F_BEYOND_EOF)) {

Hrm. The last test in iomap_block_needs_zeroing is if pos is at or beyond EOF, and iomap_adjust_read_range takes great pains to reduce plen so that poff/plen never cross EOF. I think the intent of that code is to ensure that we always zero the post-EOF part of a folio when reading it in from disk.

For verity I can see why you don't want to zero the merkle tree blocks beyond EOF, but I think this code can expose unwritten junk in the post-EOF part of the EOF block on disk.

Would it be more correct to do:

static inline bool
iomap_block_needs_zeroing(
const struct iomap_iter *iter,
struct folio *folio,
loff_t pos)

{
	const struct iomap *srcmap = iomap_iter_srcmap(iter);

	if (srcmap->type != IOMAP_MAPPED)
		return true;
	if (srcmap->flags & IOMAP_F_NEW);
		return true;

	/*
	 * Merkle tree exists in a separate folio beyond EOF, so
	 * only zero if this is the EOF folio.
	 */
	if (iomap->flags & IOMAP_F_BEYOND_EOF)
		return folio_pos(folio) == i_size_read(iter->inode);

	return pos >= i_size_read(iter->inode);
}

Quoted 21 lines

fs/iomap/buffered-io.c +3 -2

 			folio_zero_range(folio, poff, plen);
 			iomap_set_range_uptodate(folio, poff, plen);
 		} else {

@@ -1130,13 +1131,14 @@
 		 * unlock and release the folio.
 		 */
 		old_size = iter->inode->i_size;
-		if (pos + written > old_size) {
+		if (pos + written > old_size &&
+		    !(iter->flags & IOMAP_F_BEYOND_EOF)) {
 			i_size_write(iter->inode, pos + written);
 			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
 		}
 		__iomap_put_folio(iter, write_ops, written, folio);
 
-		if (old_size < pos)
+		if (old_size < pos && !(iter->flags & IOMAP_F_BEYOND_EOF))
 			pagecache_isize_extended(iter->inode, old_size, pos);
 
 		cond_resched();

fs/iomap/buffered-io.c line 1821

Quoted 7 lines

fs/iomap/buffered-io.c +2 -2

@@ -1815,8 +1817,9 @@
 
 	trace_iomap_writeback_folio(inode, pos, folio_size(folio));
 
-	if (!iomap_writeback_handle_eof(folio, inode, &end_pos))
-		return 0;
+	if (!(wpc->iomap.flags & IOMAP_F_BEYOND_EOF) &&
+	    !iomap_writeback_handle_eof(folio, inode, &end_pos))

Hrm. I /think/ this might break post-eof zeroing on writeback if BEYOND_EOF is set. For verity this isn't a problem because there's no writeback, but it's a bit of a logic bomb if someone ever tries to set BEYOND_EOF on a non-verity file.

--D

Quoted 23 lines

fs/iomap/buffered-io.c +1 -0

+ 		return 0;
 	WARN_ON_ONCE(end_pos <= pos);
 
 	if (i_blocks_per_folio(inode, folio) > 1) {

fs/iomap/trace.h +2 -1

index 532787277b..f1895f7ae5 100644
@@ -118,7 +118,8 @@
 	{ IOMAP_F_ATOMIC_BIO,	"ATOMIC_BIO" }, \
 	{ IOMAP_F_PRIVATE,	"PRIVATE" }, \
 	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }, \
-	{ IOMAP_F_STALE,	"STALE" }
+	{ IOMAP_F_STALE,	"STALE" }, \
+	{ IOMAP_F_BEYOND_EOF,	"BEYOND_EOF" }
 
 
 #define IOMAP_DIO_STRINGS \

include/linux/iomap.h +0 -0

index 520e967cb5..7a7e31c499 100644

include/linux/iomap.h line 89

Quoted 4 lines

include/linux/iomap.h +1 -0

@@ -86,6 +86,11 @@
 #define IOMAP_F_PRIVATE		(1U << 12)
 
 /*
+ * IO happens beyound inode EOF

s/beyound/beyond/

Quoted 12 lines

include/linux/iomap.h +4 -0

+ */
+#define IOMAP_F_BEYOND_EOF	(1U << 13)
+
+/*
  * Flags set by the core iomap code during operations:
  *
  * IOMAP_F_SIZE_CHANGED indicates to the iomap_end method that the file size

Signature · 2 lines

-- 
- Andrey

Quoted 34 lines · Darrick J. Wong

fs/iomap/buffered-io.c

Quoted 15 lines · Andrey Albershteyn

Flag to indicate to iomap that read/write is happening beyond EOF and no isize checks/update is needed.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/buffered-io.c | 13 ++++++++-----
 fs/iomap/trace.h       |  3 ++-
 include/linux/iomap.h  |  5 +++++
 3 files changed, 15 insertions(+), 6 deletions(-)

fs/iomap/buffered-io.c +0 -0

index e5c1ca440d..cc1cbf2a4c 100644
@@ -533,7 +533,8 @@

(Does your diff program not set --show-c-function? That makes reviewing harder because I have to search on the comment text to figure out which function this is)

fs/iomap/buffered-io.c line 537

Quoted 6 lines

fs/iomap/buffered-io.c +2 -1

 			return 0;
 
 		/* zero post-eof blocks as the page may be mapped */
-		if (iomap_block_needs_zeroing(iter, pos)) {
+		if (iomap_block_needs_zeroing(iter, pos) &&
+		    !(iomap->flags & IOMAP_F_BEYOND_EOF)) {

Hrm. The last test in iomap_block_needs_zeroing is if pos is at or beyond EOF, and iomap_adjust_read_range takes great pains to reduce plen so that poff/plen never cross EOF. I think the intent of that code is to ensure that we always zero the post-EOF part of a folio when reading it in from disk.

Quoted 3 lines

For verity I can see why you don't want to zero the merkle tree blocks beyond EOF, but I think this code can expose unwritten junk in the post-EOF part of the EOF block on disk.

Oh wait, is IOMAP_F_BEYOND_EOF only set on mappings that are entirely beyond EOF, aka the merkle tree extents?

--D

Quoted 58 lines

Would it be more correct to do:

static inline bool
iomap_block_needs_zeroing(
const struct iomap_iter *iter,
struct folio *folio,
loff_t pos)

{
	const struct iomap *srcmap = iomap_iter_srcmap(iter);

	if (srcmap->type != IOMAP_MAPPED)
		return true;
	if (srcmap->flags & IOMAP_F_NEW);
		return true;

	/*
	 * Merkle tree exists in a separate folio beyond EOF, so
	 * only zero if this is the EOF folio.
	 */
	if (iomap->flags & IOMAP_F_BEYOND_EOF)
		return folio_pos(folio) == i_size_read(iter->inode);

	return pos >= i_size_read(iter->inode);
}

Quoted 21 lines

fs/iomap/buffered-io.c +3 -2

 			folio_zero_range(folio, poff, plen);
 			iomap_set_range_uptodate(folio, poff, plen);
 		} else {

@@ -1130,13 +1131,14 @@
 		 * unlock and release the folio.
 		 */
 		old_size = iter->inode->i_size;
-		if (pos + written > old_size) {
+		if (pos + written > old_size &&
+		    !(iter->flags & IOMAP_F_BEYOND_EOF)) {
 			i_size_write(iter->inode, pos + written);
 			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
 		}
 		__iomap_put_folio(iter, write_ops, written, folio);
 
-		if (old_size < pos)
+		if (old_size < pos && !(iter->flags & IOMAP_F_BEYOND_EOF))
 			pagecache_isize_extended(iter->inode, old_size, pos);
 
 		cond_resched();

fs/iomap/buffered-io.c line 1821

Quoted 7 lines

fs/iomap/buffered-io.c +2 -2

@@ -1815,8 +1817,9 @@
 
 	trace_iomap_writeback_folio(inode, pos, folio_size(folio));
 
-	if (!iomap_writeback_handle_eof(folio, inode, &end_pos))
-		return 0;
+	if (!(wpc->iomap.flags & IOMAP_F_BEYOND_EOF) &&
+	    !iomap_writeback_handle_eof(folio, inode, &end_pos))

Hrm. I /think/ this might break post-eof zeroing on writeback if BEYOND_EOF is set. For verity this isn't a problem because there's no writeback, but it's a bit of a logic bomb if someone ever tries to set BEYOND_EOF on a non-verity file.

Quoted 28 lines

--D

Quoted 25 lines

fs/iomap/buffered-io.c +1 -0

+ 		return 0;
 	WARN_ON_ONCE(end_pos <= pos);
 
 	if (i_blocks_per_folio(inode, folio) > 1) {

fs/iomap/trace.h +2 -1

index 532787277b..f1895f7ae5 100644
@@ -118,7 +118,8 @@
 	{ IOMAP_F_ATOMIC_BIO,	"ATOMIC_BIO" }, \
 	{ IOMAP_F_PRIVATE,	"PRIVATE" }, \
 	{ IOMAP_F_SIZE_CHANGED,	"SIZE_CHANGED" }, \
-	{ IOMAP_F_STALE,	"STALE" }
+	{ IOMAP_F_STALE,	"STALE" }, \
+	{ IOMAP_F_BEYOND_EOF,	"BEYOND_EOF" }
 
 
 #define IOMAP_DIO_STRINGS \

include/linux/iomap.h +0 -0

index 520e967cb5..7a7e31c499 100644
@@ -86,6 +86,11 @@
 #define IOMAP_F_PRIVATE		(1U << 12)
 

Quoted 4 lines

include/linux/iomap.h line 89

Quoted 2 lines

include/linux/iomap.h +1 -0

 /*
+ * IO happens beyound inode EOF

s/beyound/beyond/

Quoted 10 lines · Andrey Albershteyn

Flag to indicate to iomap that read/write is happening beyond EOF and no isize checks/update is needed.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/buffered-io.c | 13 ++++++++-----
 fs/iomap/trace.h       |  3 ++-
 include/linux/iomap.h  |  5 +++++
 3 files changed, 15 insertions(+), 6 deletions(-)

fs/iomap/buffered-io.c

Quoted 5 lines

fs/iomap/buffered-io.c +0 -0

index e5c1ca440d..cc1cbf2a4c 100644
@@ -533,7 +533,8 @@

(Does your diff program not set --show-c-function? That makes reviewing harder because I have to search on the comment text to figure out which function this is)

Hrm. The last test in iomap_block_needs_zeroing is if pos is at or beyond EOF, and iomap_adjust_read_range takes great pains to reduce plen so that poff/plen never cross EOF. I think the intent of that code is to ensure that we always zero the post-EOF part of a folio when reading it in from disk.

For verity I can see why you don't want to zero the merkle tree blocks beyond EOF, but I think this code can expose unwritten junk in the post-EOF part of the EOF block on disk.

Would it be more correct to do:

Quoted 4 lines

fs/iomap/buffered-io.c +0 -0

@@ -1815,8 +1817,9 @@
 
 	trace_iomap_writeback_folio(inode, pos, folio_size(folio));
 

fs/iomap/buffered-io.c line 1821

Quoted 4 lines

fs/iomap/buffered-io.c +2 -2

-	if (!iomap_writeback_handle_eof(folio, inode, &end_pos))
-		return 0;
+	if (!(wpc->iomap.flags & IOMAP_F_BEYOND_EOF) &&
+	    !iomap_writeback_handle_eof(folio, inode, &end_pos))

Hrm. I /think/ this might break post-eof zeroing on writeback if BEYOND_EOF is set. For verity this isn't a problem because there's no writeback, but it's a bit of a logic bomb if someone ever tries to set BEYOND_EOF on a non-verity file.

Quoted 11 lines · Darrick J. Wong

Quoted 10 lines · Andrey Albershteyn

Flag to indicate to iomap that read/write is happening beyond EOF and no isize checks/update is needed.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/buffered-io.c | 13 ++++++++-----
 fs/iomap/trace.h       |  3 ++-
 include/linux/iomap.h  |  5 +++++
 3 files changed, 15 insertions(+), 6 deletions(-)

Quoted 9 lines

fs/iomap/buffered-io.c

Quoted 5 lines

fs/iomap/buffered-io.c +0 -0

index e5c1ca440d..cc1cbf2a4c 100644
@@ -533,7 +533,8 @@

(Does your diff program not set --show-c-function? That makes reviewing harder because I have to search on the comment text to figure out which function this is)

Or use git-send-email which sidesteps all these issues :)

Quoted 6 lines

Hrm. The last test in iomap_block_needs_zeroing is if pos is at or beyond EOF, and iomap_adjust_read_range takes great pains to reduce plen so that poff/plen never cross EOF. I think the intent of that code is to ensure that we always zero the post-EOF part of a folio when reading it in from disk.

Quoted 5 lines

For verity I can see why you don't want to zero the merkle tree blocks beyond EOF, but I think this code can expose unwritten junk in the post-EOF part of the EOF block on disk.

Would it be more correct to do:

Or replace the generic past EOF flag with a FSVERITY flag making the use case clear?

Quoted 4 lines

Quoted 4 lines

fs/iomap/buffered-io.c +0 -0

@@ -1815,8 +1817,9 @@
 
 	trace_iomap_writeback_folio(inode, pos, folio_size(folio));
 

Quoted 9 lines

fs/iomap/buffered-io.c line 1821

Quoted 4 lines

fs/iomap/buffered-io.c +2 -2

-	if (!iomap_writeback_handle_eof(folio, inode, &end_pos))
-		return 0;
+	if (!(wpc->iomap.flags & IOMAP_F_BEYOND_EOF) &&
+	    !iomap_writeback_handle_eof(folio, inode, &end_pos))

Hrm. I /think/ this might break post-eof zeroing on writeback if BEYOND_EOF is set. For verity this isn't a problem because there's no writeback, but it's a bit of a logic bomb if someone ever tries to set BEYOND_EOF on a non-verity file.

Maybe we should not even support the flag for writeback?

Hmm not sure how you see this, the verity pages need to get written somehow and as they are beyond EOF they will be zeroed out here.

Regarding your comment in the thread to use direct IO, fsverity wants to use page cache as cache for verified pages (already verified page will have "verified" flag set, freshly read won't).

Quoted 7 lines · Andrey Albershteyn

Hmm not sure how you see this, the verity pages need to get written somehow and as they are beyond EOF they will be zeroed out here.

Regarding your comment in the thread to use direct IO, fsverity wants to use page cache as cache for verified pages (already verified page will have "verified" flag set, freshly read won't).

Ok, I guess we need to stick to the pagecache based path then. The upside is less differences from the existing file systems.

Flag to indicate to iomap that read/write is happening beyond EOF and no isize checks/update is needed.

Quoted 2 lines · Andrey Albershteyn

Flag to indicate to iomap that read/write is happening beyond EOF and no isize checks/update is needed.

Darrick and I talked to Ted about this yesterday, and we think you don't need the writeback portions of this. (As Darrick said, it's hard to tell if this is all writeback or just some of it).

The flow for creating a verity file should be:

write data to pagecache
calculate merkle tree data, write it to pagecache
write_and_wait the entire file
do verity magic

If you follow this flow, there's never a need to do writeback past EOF. Or indeed writeback on a verity file at all, since it's now immutable.

This will be necessary for XFS to use iomap_file_buffered_write() in context without file pointer.

As the only user of this is XFS fsverity let's set necessary IOMAP_F_BEYOND_EOF flag if no file provided instead of adding new flags to iocb->ki_flags.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/buffered-io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

fs/iomap/buffered-io.c +0 -1

index cc1cbf2a4c..79d1c97f02 100644
@@ -1173,7 +1173,6 @@
 		const struct iomap_write_ops *write_ops, void *private)
 {
 	struct iomap_iter iter = {
-		.inode		= iocb->ki_filp->f_mapping->host,
 		.pos		= iocb->ki_pos,
 		.len		= iov_iter_count(i),
 		.flags		= IOMAP_WRITE,

fs/iomap/buffered-io.c +2 -0

+	}
+
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		iter.flags |= IOMAP_NOWAIT;
 	if (iocb->ki_flags & IOCB_DONTCACHE)

Signature · 2 lines

-- 
- Andrey

fs/iomap/buffered-io.c line 1186

Quoted 3 lines

fs/iomap/buffered-io.c +3 -0

+		iter.inode = iocb->ki_filp->f_mapping->host;
+	} else {
+		iter.inode = (struct inode *)private;

@private is for the filesystem implementation to access, not the generic iomap code. If this is intended for fsverity, then shouldn't merkle tree construction be the only time that fsverity writes to the file? And shouldn't fsverity therefore have access to the struct file?

fs/iomap/buffered-io.c line 1187

Quoted 2 lines

fs/iomap/buffered-io.c +1 -0

+		iter.flags |= IOMAP_F_BEYOND_EOF;

IOMAP_F_ flags are mapping state flags for struct iomap::flags, not the iomap_iter.

Quoted 8 lines · Darrick J. Wong

fs/iomap/buffered-io.c line 1186

Quoted 3 lines

fs/iomap/buffered-io.c +3 -0

+		iter.inode = iocb->ki_filp->f_mapping->host;
+	} else {
+		iter.inode = (struct inode *)private;

@private is for the filesystem implementation to access, not the generic iomap code. If this is intended for fsverity, then shouldn't merkle tree construction be the only time that fsverity writes to the file? And shouldn't fsverity therefore have access to the struct file?

It's not passed down, but I think it could easily.

Quoted 5 lines

fs/iomap/buffered-io.c line 1187

Quoted 2 lines

fs/iomap/buffered-io.c +1 -0

+		iter.flags |= IOMAP_F_BEYOND_EOF;

IOMAP_F_ flags are mapping state flags for struct iomap::flags, not the iomap_iter.

But we could fix this part as well by having a specific helper for fsverity that is called instead of iomap_file_buffered_write. Neither the iocb, nor struct file are used inside of iomap_write_iter. So just add a new helper that takes an inode, sets the past-EOF/verify flag and open codes the iomap_iter/iomap_write_iter loop.

Quoted 8 lines · Darrick J. Wong

fs/iomap/buffered-io.c line 1186

Quoted 3 lines

fs/iomap/buffered-io.c +3 -0

+		iter.inode = iocb->ki_filp->f_mapping->host;
+	} else {
+		iter.inode = (struct inode *)private;

@private is for the filesystem implementation to access, not the generic iomap code. If this is intended for fsverity, then shouldn't merkle tree construction be the only time that fsverity writes to the file? And shouldn't fsverity therefore have access to the struct file?

It's not passed down, but I think it could easily.

Quoted 11 lines · Christoph Hellwig

Quoted 8 lines · Darrick J. Wong

fs/iomap/buffered-io.c line 1186

Quoted 3 lines

fs/iomap/buffered-io.c +3 -0

+		iter.inode = iocb->ki_filp->f_mapping->host;
+	} else {
+		iter.inode = (struct inode *)private;

@private is for the filesystem implementation to access, not the generic iomap code. If this is intended for fsverity, then shouldn't merkle tree construction be the only time that fsverity writes to the file? And shouldn't fsverity therefore have access to the struct file?

It's not passed down, but I think it could easily.

willy@deadly:~/kernel/linux$ git grep ki_filp |grep file_inode | wc
    109     575    7766
willy@deadly:~/kernel/linux$ git grep ki_filp |wc
    367    1920   23371

I think there's a pretty strong argument for adding ki_inode to struct kiocb. What do you think?

Quoted 1 line

It's not passed down, but I think it could easily.

willy@deadly:~/kernel/linux$ git grep ki_filp |grep file_inode | wc
    109     575    7766
willy@deadly:~/kernel/linux$ git grep ki_filp |wc
    367    1920   23371

I think there's a pretty strong argument for adding ki_inode to struct kiocb. What do you think?

Quoted 7 lines · Matthew Wilcox

Quoted 1 line

It's not passed down, but I think it could easily.

willy@deadly:~/kernel/linux$ git grep ki_filp |grep file_inode | wc
    109     575    7766
willy@deadly:~/kernel/linux$ git grep ki_filp |wc
    367    1920   23371

Quoted 2 lines

I think there's a pretty strong argument for adding ki_inode to struct kiocb. What do you think?

That assumes the reduced pointer dereference is worth bloating the structure. Feel free to give it a try.

Note that we'd still require the file to be set, otherwise we're going to have calling conventions from hell.

This patch adds fs-verity verification into iomap's read path. After BIO's io operation is complete the data are verified against fs-verity's Merkle tree. Verification work is done in a separate workqueue.

The read path ioend iomap_read_ioend are stored side by side with BIOs if FS_VERITY is enabled.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/bio.c         | 66 ++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/iomap/buffered-io.c | 12 ++++++++-
 fs/iomap/ioend.c       | 41 +++++++++++++++++++++++++++++++-
 include/linux/iomap.h  | 11 ++++++++
 4 files changed, 123 insertions(+), 7 deletions(-)

fs/iomap/bio.c +1 -0

index fc045f2e4c..ac6c16b1f8 100644
@@ -5,6 +5,7 @@
  */
 #include <linux/iomap.h>
 #include <linux/pagemap.h>
+#include <linux/fsverity.h>
 #include "internal.h"
 #include "trace.h"
 

fs/iomap/bio.c +59 -5

+static void
+iomap_read_fsverify_end_io_work(struct work_struct *work)
+{
+	struct iomap_fsverity_bio *fbio =
+		container_of(work, struct iomap_fsverity_bio, work);
+
+	fsverity_verify_bio(&fbio->bio);
+	iomap_read_end_io(&fbio->bio);
+}
+
+static void
+iomap_read_fsverity_end_io(struct bio *bio)
+{
+	struct iomap_fsverity_bio *fbio =
+		container_of(bio, struct iomap_fsverity_bio, bio);
+
+	INIT_WORK(&fbio->work, iomap_read_fsverify_end_io_work);
+	fsverity_enqueue_verify_work(&fbio->work);
+}
+
+static struct bio *
+iomap_fsverity_read_bio_alloc(struct inode *inode, struct block_device *bdev,
+			    int nr_vecs, gfp_t gfp)
+{
+	struct bio *bio;
+
+	bio = bio_alloc_bioset(bdev, nr_vecs, REQ_OP_READ, gfp,
+			iomap_fsverity_bioset);
+	if (bio)
+		bio->bi_end_io = iomap_read_fsverity_end_io;
+	return bio;
+}
+
+#else
+# define iomap_fsverity_read_bio_alloc(...)	(NULL)
+# define iomap_fsverity_tree_end_align(...)	(false)
+#endif /* CONFIG_FS_VERITY */
+
+static struct bio *iomap_read_bio_alloc(struct inode *inode,
+		const struct iomap *iomap, int nr_vecs, gfp_t gfp)
+{
+	struct bio *bio;
+	struct block_device *bdev = iomap->bdev;
+
+	if (!(iomap->flags & IOMAP_F_BEYOND_EOF) && fsverity_active(inode))
+		return iomap_fsverity_read_bio_alloc(inode, bdev, nr_vecs, gfp);
+
+	bio = bio_alloc(bdev, nr_vecs, REQ_OP_READ, gfp);
+	if (bio)
+		bio->bi_end_io = iomap_read_end_io;
+	return bio;
+}
+
 static void iomap_bio_submit_read(struct iomap_read_folio_ctx *ctx)
 {
 	struct bio *bio = ctx->read_ctx;

@@ -42,26 +97,27 @@
 	    !bio_add_folio(bio, folio, plen, poff)) {
 		gfp_t gfp = mapping_gfp_constraint(folio->mapping, GFP_KERNEL);
 		gfp_t orig_gfp = gfp;
-		unsigned int nr_vecs = DIV_ROUND_UP(length, PAGE_SIZE);
 
 		if (bio)
 			submit_bio(bio);
 
 		if (ctx->rac) /* same as readahead_gfp_mask */
 			gfp |= __GFP_NORETRY | __GFP_NOWARN;
-		bio = bio_alloc(iomap->bdev, bio_max_segs(nr_vecs), REQ_OP_READ,
-				     gfp);
+		bio = iomap_read_bio_alloc(iter->inode, iomap,
+				bio_max_segs(DIV_ROUND_UP(length, PAGE_SIZE)),
+				gfp);
+
 		/*
 		 * If the bio_alloc fails, try it again for a single page to
 		 * avoid having to deal with partial page reads.  This emulates
 		 * what do_mpage_read_folio does.
 		 */
 		if (!bio)
-			bio = bio_alloc(iomap->bdev, 1, REQ_OP_READ, orig_gfp);
+			bio = iomap_read_bio_alloc(iter->inode, iomap, 1,
+						   orig_gfp);
 		if (ctx->rac)
 			bio->bi_opf |= REQ_RAHEAD;
 		bio->bi_iter.bi_sector = sector;
-		bio->bi_end_io = iomap_read_end_io;
 		bio_add_folio_nofail(bio, folio, plen, poff);
 		ctx->read_ctx = bio;
 	}

fs/iomap/buffered-io.c +1 -0

index 79d1c97f02..481f7e1cff 100644
@@ -8,6 +8,7 @@
 #include <linux/writeback.h>
 #include <linux/swap.h>
 #include <linux/migrate.h>
+#include <linux/fsverity.h>
 #include "internal.h"
 #include "trace.h"
 

fs/iomap/buffered-io.c +4 -0

 		    !(iomap->flags & IOMAP_F_BEYOND_EOF)) {
 			folio_zero_range(folio, poff, plen);
+			if (fsverity_active(iter->inode) &&
+			    !fsverity_verify_blocks(folio, plen, poff)) {
+				return -EIO;
+			}
 			iomap_set_range_uptodate(folio, poff, plen);
 		} else {
 			if (!*bytes_submitted)

fs/iomap/ioend.c +0 -0

index 86f44922ed..30c0de3c75 100644

fs/iomap/ioend.c +39 -1

+
 struct bio_set iomap_ioend_bioset;
 EXPORT_SYMBOL_GPL(iomap_ioend_bioset);
 

@@ -423,9 +425,46 @@
 }
 EXPORT_SYMBOL_GPL(iomap_split_ioend);
 
+#ifdef CONFIG_FS_VERITY
+struct bio_set *iomap_fsverity_bioset;
+EXPORT_SYMBOL_GPL(iomap_fsverity_bioset);
+int iomap_fsverity_init_bioset(void)
+{
+	struct bio_set *bs, *old;
+	int error;
+
+	bs = kzalloc(sizeof(*bs), GFP_KERNEL);
+	if (!bs)
+		return -ENOMEM;
+
+	error = bioset_init(bs, IOMAP_POOL_SIZE,
+			    offsetof(struct iomap_fsverity_bio, bio),
+			    BIOSET_NEED_BVECS);
+	if (error) {
+		kfree(bs);
+		return error;
+	}
+
+	/*
+	 * This has to be atomic as readaheads can race to create the
+	 * bioset.  If someone set the pointer before us, we drop ours.
+	 */
+	old = cmpxchg(&iomap_fsverity_bioset, NULL, bs);
+	if (old) {
+		bioset_exit(bs);
+		kfree(bs);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iomap_fsverity_init_bioset);
+#else
+# define iomap_fsverity_init_bioset(...)	(-EOPNOTSUPP)
+#endif
+
 static int __init iomap_ioend_init(void)
 {
-	return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
+	return bioset_init(&iomap_ioend_bioset, IOMAP_POOL_SIZE,
 			   offsetof(struct iomap_ioend, io_bio),
 			   BIOSET_NEED_BVECS);
 }

include/linux/iomap.h +11 -0

index 7a7e31c499..b451ab3426 100644
@@ -342,6 +342,17 @@
 		iter->srcmap.type == IOMAP_MAPPED;
 }
 
+#ifdef CONFIG_FS_VERITY
+extern struct bio_set *iomap_fsverity_bioset;
+
+struct iomap_fsverity_bio {
+	struct work_struct	work;
+	struct bio		bio;
+};
+
+int iomap_fsverity_init_bioset(void);
+#endif
+
 ssize_t iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *from,
 		const struct iomap_ops *ops,
 		const struct iomap_write_ops *write_ops, void *private);

Signature · 2 lines

-- 
- Andrey

Quoted 32 lines · Andrey Albershteyn

This patch adds fs-verity verification into iomap's read path. After BIO's io operation is complete the data are verified against fs-verity's Merkle tree. Verification work is done in a separate workqueue.

The read path ioend iomap_read_ioend are stored side by side with BIOs if FS_VERITY is enabled.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/iomap/bio.c         | 66 ++++++++++++++++++++++++++++++++++++++++++++++++----
 fs/iomap/buffered-io.c | 12 ++++++++-
 fs/iomap/ioend.c       | 41 +++++++++++++++++++++++++++++++-
 include/linux/iomap.h  | 11 ++++++++
 4 files changed, 123 insertions(+), 7 deletions(-)

fs/iomap/bio.c +1 -0

index fc045f2e4c..ac6c16b1f8 100644
@@ -5,6 +5,7 @@
  */
 #include <linux/iomap.h>
 #include <linux/pagemap.h>
+#include <linux/fsverity.h>
 #include "internal.h"
 #include "trace.h"
 

@@ -18,6 +19,60 @@
 	bio_put(bio);
 }
 

fs/iomap/bio.c line 22

Quoted 1 line

fs/iomap/bio.c +1 -0

+#ifdef CONFIG_FS_VERITY

Should all this stuff go into fs/iomap/fsverity.c instead of ifdef'd around the iomap code?

<shrug>

Quoted 13 lines

fs/iomap/buffered-io.c +1 -0

index 79d1c97f02..481f7e1cff 100644
@@ -8,6 +8,7 @@
 #include <linux/writeback.h>
 #include <linux/swap.h>
 #include <linux/migrate.h>
+#include <linux/fsverity.h>
 #include "internal.h"
 #include "trace.h"
 

fs/iomap/buffered-io.c line 537

Quoted 6 lines

fs/iomap/buffered-io.c +2 -0

@@ -532,10 +533,19 @@
 		if (plen == 0)
 			return 0;
 
+		/* end of fs-verity region*/
+		if ((iomap->flags & IOMAP_F_BEYOND_EOF) && (iomap->type == IOMAP_HOLE)) {

Overly long line.

Also, when do we get the combination of BEYOND_EOF && HOLE? Is that for sparse regions in only the merkle tree? IIRC (and I could be wrong) fsverity still wants to checksum sparse holes in the regular file data, right?

fs/iomap/buffered-io.c line 542

Quoted 7 lines

fs/iomap/buffered-io.c +4 -1

+			folio_zero_range(folio, poff, plen);
+			iomap_set_range_uptodate(folio, poff, plen);
+		}
 		/* zero post-eof blocks as the page may be mapped */
-		if (iomap_block_needs_zeroing(iter, pos) &&
+		else if (iomap_block_needs_zeroing(iter, pos) &&

		} else if (...

(nitpicking indentation)

Quoted 17 lines

fs/iomap/buffered-io.c +4 -0

 		    !(iomap->flags & IOMAP_F_BEYOND_EOF)) {
 			folio_zero_range(folio, poff, plen);
+			if (fsverity_active(iter->inode) &&
+			    !fsverity_verify_blocks(folio, plen, poff)) {
+				return -EIO;
+			}
 			iomap_set_range_uptodate(folio, poff, plen);
 		} else {
 			if (!*bytes_submitted)

fs/iomap/ioend.c +0 -0

index 86f44922ed..30c0de3c75 100644
@@ -9,6 +9,8 @@
 #include "internal.h"
 #include "trace.h"
 

fs/iomap/ioend.c line 12

Quoted 1 line

fs/iomap/ioend.c +1 -0

+#define IOMAP_POOL_SIZE		(4 * (PAGE_SIZE / SECTOR_SIZE))

How do we arrive at this pool size? How is it important to have a larger bio reserve pool for *larger* base page sizes?

Quoted 4 lines

Also, when do we get the combination of BEYOND_EOF && HOLE? Is that for sparse regions in only the merkle tree? IIRC (and I could be wrong) fsverity still wants to checksum sparse holes in the regular file data, right?

The _BEYOUND_EOF is only for fsverity metadata. This case handles the merkle tree tail case/end of tree. 1k fs bs 4k page 1k fsverity blocks, fsverity requests the page with a tree which is smaller than 4 fsverity blocks (e.g. 3072b). The last 1k block in the page will be hole. So, just zero out the rest and mark uptodate.

Quoted 4 lines

Quoted 1 line

+#define IOMAP_POOL_SIZE (4 * (PAGE_SIZE / SECTOR_SIZE))

How do we arrive at this pool size? How is it important to have a larger bio reserve pool for *larger* base page sizes?

Well, this is just a one which iomap uses by default for read pool. I'm not sure I know enough to optimize pool size here :)

fs/xfs/xfs_file.c +1 -2

 	if (IS_DAX(inode))
 		ret = xfs_file_dax_read(iocb, to);
-	else if (iocb->ki_flags & IOCB_DIRECT)
+	else if ((iocb->ki_flags & IOCB_DIRECT) && !fsverity_active(inode))
 		ret = xfs_file_dio_read(iocb, to);
-	else

Quoted 6 lines · Andrey Albershteyn

fs/xfs/xfs_file.c +1 -2

 	if (IS_DAX(inode))
 		ret = xfs_file_dax_read(iocb, to);
-	else if (iocb->ki_flags & IOCB_DIRECT)
+	else if ((iocb->ki_flags & IOCB_DIRECT) && !fsverity_active(inode))
 		ret = xfs_file_dio_read(iocb, to);
-	else

fs/xfs/xfs_file.c line 322

Quoted 10 lines

fs/xfs/xfs_file.c +9 -0

+	else {
+		/*
+		 * In case fs-verity is enabled, we also fallback to the
+		 * buffered read from the direct read path. Therefore,
+		 * IOCB_DIRECT is set and need to be cleared (see
+		 * generic_file_read_iter())
+		 */
+		iocb->ki_flags &= ~IOCB_DIRECT;
 		ret = xfs_file_buffered_read(iocb, to);
+	}

I think this might actuall be easier as:

	if (fsverity_active(inode))
		iocb->ki_flags &= ~IOCB_DIRECT;

	...
	<existing if/else>

Add the fsverity_info pointer into the filesystem-specific part of the inode by adding the field xfs_inode->i_verity_info and configuring fsverity_operations::inode_info_offs accordingly.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/xfs_icache.c | 3 +++
 fs/xfs/xfs_inode.h  | 5 +++++
 2 files changed, 8 insertions(+), 0 deletions(-)

fs/xfs/xfs_icache.c +3 -0

index 23a920437f..872785c68a 100644
@@ -130,6 +130,9 @@
 	spin_lock_init(&ip->i_ioend_lock);
 	ip->i_next_unlinked = NULLAGINO;
 	ip->i_prev_unlinked = 0;
+#ifdef CONFIG_FS_VERITY
+	ip->i_verity_info = NULL;
+#endif
 
 	return ip;
 }

fs/xfs/xfs_inode.h +5 -0

index bd6d335571..f149cb1eb5 100644
@@ -9,6 +9,7 @@
 #include "xfs_inode_buf.h"
 #include "xfs_inode_fork.h"
 #include "xfs_inode_util.h"
+#include <linux/fsverity.h>
 
 /*
  * Kernel only inode definitions

@@ -99,6 +100,10 @@
 	spinlock_t		i_ioend_lock;
 	struct work_struct	i_ioend_work;
 	struct list_head	i_ioend_list;
+
+#ifdef CONFIG_FS_VERITY
+	struct fsverity_info *i_verity_info;
+#endif
 } xfs_inode_t;
 
 static inline bool xfs_inode_on_unlinked_list(const struct xfs_inode *ip)

Signature · 2 lines

-- 
- Andrey

Quoted 5 lines · Andrey Albershteyn

Add the fsverity_info pointer into the filesystem-specific part of the inode by adding the field xfs_inode->i_verity_info and configuring fsverity_operations::inode_info_offs accordingly.

Signed-off-by Andrey Albershteyn <[email protected]>

I kinda don't like adding another pointer to struct xfs_inode

but I can't see a better solution.

Quoted 8 lines · Darrick J. Wong

Quoted 5 lines · Andrey Albershteyn

Add the fsverity_info pointer into the filesystem-specific part of the inode by adding the field xfs_inode->i_verity_info and configuring fsverity_operations::inode_info_offs accordingly.

Signed-off-by Andrey Albershteyn <[email protected]>

I kinda don't like adding another pointer to struct xfs_inode

Me neither.

Quoted 1 line

but I can't see a better solution.

Well, I suggested just making the fsverity_info a standalone object, and looking it up using a rhastable based on the ino. Eric didn't seem overly existed about that, but I think it would be really helpful to not bloat all the common inodes for fsverity.

Quoted 4 lines

Well, I suggested just making the fsverity_info a standalone object, and looking it up using a rhastable based on the ino. Eric didn't seem overly existed about that, but I think it would be really helpful to not bloat all the common inodes for fsverity.

Oh, right, now I remember you asking about using rhashtable to map an xfs_inode to a fsverity_info. I wonder how much of a performance overhead that lookup (vs. the pointer deref that's used now) would have for loading folios into the pagecache.

Add new flag meaning that merkle tree is being build on the inode.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/xfs_inode.h | 6 ++++++
 1 file changed, 6 insertions(+), 0 deletions(-)

fs/xfs/xfs_inode.h +0 -0

index f149cb1eb5..9373bf146a 100644

fs/xfs/xfs_inode.h +3 -0

+ */
+#define XFS_VERITY_CONSTRUCTION	(1U << 16)
+
 /* All inode state flags related to inode reclaim. */
 #define XFS_ALL_IRECLAIM_FLAGS	(XFS_IRECLAIMABLE | \
 				 XFS_IRECLAIM | \

Signature · 2 lines

-- 
- Andrey

Quoted 3 lines · Andrey Albershteyn

Add new flag meaning that merkle tree is being build on the inode.

Signed-off-by Andrey Albershteyn <[email protected]>

Seems fine to me, with one nit below

Reviewed-by "Darrick J. Wong" <[email protected]>

--D

Quoted 12 lines

---
 fs/xfs/xfs_inode.h | 6 ++++++
 1 file changed, 6 insertions(+), 0 deletions(-)

fs/xfs/xfs_inode.h +0 -0

index f149cb1eb5..9373bf146a 100644
@@ -420,6 +420,12 @@
  */
 #define XFS_IREMAPPING		(1U << 15)
 

fs/xfs/xfs_inode.h line 425

Quoted 3 lines

fs/xfs/xfs_inode.h +3 -0

+/*
+ * fs-verity's Merkle tree is under construction. The file is read-only, the
+ * only writes happening is the ones with Merkle tree blocks.

"..the only writes happening are for the fsverity metadata."

since fsverity could in theory someday write more than just a merkle tree and a descriptor.

This constant defines location of fsverity metadata in page cache of an inode.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/libxfs/xfs_fs.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+), 0 deletions(-)

fs/xfs/libxfs/xfs_fs.h +0 -0

index 12463ba766..b73458a7c2 100644

fs/xfs/libxfs/xfs_fs.h +10 -0

+ * address space for Merkle tree.
+ *
+ * At maximum of 8 levels with 128 hashes per block (32 bytes SHA-256) maximum
+ * tree size is ((128^8 − 1)/(128 − 1)) = 567*10^12 blocks. This should fit in 53
+ * bits address space.
+ *
+ * At this Merkle tree size we can cover 295EB large file. This is much larger
+ * than the currently supported file size.
+ *
+ * For sha512 the largest file we can cover ends at 1 << 50 offset, this is also

fs/xfs/libxfs/xfs_fs.h +1 -0

+
 #endif	/* __XFS_FS_H__ */

Signature · 2 lines

-- 
- Andrey

Quoted 13 lines · Andrey Albershteyn

This constant defines location of fsverity metadata in page cache of an inode.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/libxfs/xfs_fs.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+), 0 deletions(-)

fs/xfs/libxfs/xfs_fs.h +0 -0

index 12463ba766..b73458a7c2 100644

fs/xfs/libxfs/xfs_fs.h line 1109

Quoted 4 lines

fs/xfs/libxfs/xfs_fs.h +1 -0

@@ -1106,4 +1106,26 @@
 #define BBTOB(bbs)	((bbs) << BBSHIFT)
 #endif
 
+/* Merkle tree location in page cache. We take memory region from the inode's

Dumb nit: new line after opening the multiline comment.

/*
* Merkle tree location in page cache...

also, isn't (1U<<53) the location of the Merkle tree ondisk in addition to its location in the page cache?

That occurs to me, what happens on 32-bit systems where the pagecache can only address up to 16T of data? Maybe we just don't allow fsverity on 32-bit xfs.

Quoted 11 lines

fs/xfs/libxfs/xfs_fs.h +10 -0

+ * address space for Merkle tree.
+ *
+ * At maximum of 8 levels with 128 hashes per block (32 bytes SHA-256) maximum
+ * tree size is ((128^8 − 1)/(128 − 1)) = 567*10^12 blocks. This should fit in 53
+ * bits address space.
+ *
+ * At this Merkle tree size we can cover 295EB large file. This is much larger
+ * than the currently supported file size.
+ *
+ * For sha512 the largest file we can cover ends at 1 << 50 offset, this is also

fs/xfs/libxfs/xfs_fs.h line 1129

Quoted 10 lines

fs/xfs/libxfs/xfs_fs.h +10 -0

+ * good.
+ *
+ * The metadata is stored on disk as follows:
+ *
+ *	[merkle tree...][descriptor.............desc_size]
+ *	^ (1 << 53)     ^ (block border)                 ^ (end of the block)
+ *	                ^--------------------------------^
+ *	                Can be FS_VERITY_MAX_DESCRIPTOR_SIZE
+ */
+#define XFS_FSVERITY_REGION_START (1ULL << 53)

Is this in fsblocks or in bytes? I think the comment should state that explicitly.

Quoted 21 lines · Darrick J. Wong

Quoted 13 lines · Andrey Albershteyn

This constant defines location of fsverity metadata in page cache of an inode.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/libxfs/xfs_fs.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+), 0 deletions(-)

fs/xfs/libxfs/xfs_fs.h +0 -0

index 12463ba766..b73458a7c2 100644

fs/xfs/libxfs/xfs_fs.h line 1109

Quoted 4 lines

fs/xfs/libxfs/xfs_fs.h +1 -0

@@ -1106,4 +1106,26 @@
 #define BBTOB(bbs)	((bbs) << BBSHIFT)
 #endif
 
+/* Merkle tree location in page cache. We take memory region from the inode's

Dumb nit: new line after opening the multiline comment.

Quoted 5 lines

/*
* Merkle tree location in page cache...

also, isn't (1U<<53) the location of the Merkle tree ondisk in addition to its location in the page cache?

yes, it's file offset

Quoted 4 lines

That occurs to me, what happens on 32-bit systems where the pagecache can only address up to 16T of data? Maybe we just don't allow fsverity on 32-bit xfs.

hmm right, check in begin_enable() will be probably enough

Quoted 21 lines

Quoted 10 lines

fs/xfs/libxfs/xfs_fs.h +10 -0

+ * address space for Merkle tree.
+ *
+ * At maximum of 8 levels with 128 hashes per block (32 bytes SHA-256) maximum
+ * tree size is ((128^8 − 1)/(128 − 1)) = 567*10^12 blocks. This should fit in 53
+ * bits address space.
+ *
+ * At this Merkle tree size we can cover 295EB large file. This is much larger
+ * than the currently supported file size.
+ *
+ * For sha512 the largest file we can cover ends at 1 << 50 offset, this is also

fs/xfs/libxfs/xfs_fs.h line 1129

Quoted 10 lines

fs/xfs/libxfs/xfs_fs.h +10 -0

+ * good.
+ *
+ * The metadata is stored on disk as follows:
+ *
+ *	[merkle tree...][descriptor.............desc_size]
+ *	^ (1 << 53)     ^ (block border)                 ^ (end of the block)
+ *	                ^--------------------------------^
+ *	                Can be FS_VERITY_MAX_DESCRIPTOR_SIZE
+ */
+#define XFS_FSVERITY_REGION_START (1ULL << 53)

Quoted 2 lines

Is this in fsblocks or in bytes? I think the comment should state that explicitly.

sure, will add it

Signature · 1 line

- Andrey

Quoted 1 line

hmm right, check in begin_enable() will be probably enough

I think that would probably be more of a mount-time prohibition?

Which would be worse -- any fsverity filesystem refuses to mount on 32-bit; or it mounts but none of the fsverity files are readable?

Alternately I guess for 32-bit you could cheat in ->iomap_begin by loading the fsverity artifacts into the pagecache at 1<<39 instead of 1<<53, provided the file is smaller than 1<<39 bytes. Writing the fsverity metadata would perform the reverse translation.

(Or again we just don't allow mounting of fsverity on 32-bit kernels.)

Quoted 12 lines · Darrick J. Wong

Quoted 1 line

hmm right, check in begin_enable() will be probably enough

I think that would probably be more of a mount-time prohibition?

Which would be worse -- any fsverity filesystem refuses to mount on 32-bit; or it mounts but none of the fsverity files are readable?

Alternately I guess for 32-bit you could cheat in ->iomap_begin by loading the fsverity artifacts into the pagecache at 1<<39 instead of 1<<53, provided the file is smaller than 1<<39 bytes. Writing the fsverity metadata would perform the reverse translation.

Quoted 1 line

(Or again we just don't allow mounting of fsverity on 32-bit kernels.)

What are the other file systems doing here? Unless we have a good reason to differ we should just follow the precedence.

Quoted 12 lines · Darrick J. Wong

Quoted 1 line

hmm right, check in begin_enable() will be probably enough

I think that would probably be more of a mount-time prohibition?

Which would be worse -- any fsverity filesystem refuses to mount on 32-bit; or it mounts but none of the fsverity files are readable?

Alternately I guess for 32-bit you could cheat in ->iomap_begin by loading the fsverity artifacts into the pagecache at 1<<39 instead of 1<<53, provided the file is smaller than 1<<39 bytes. Writing the fsverity metadata would perform the reverse translation.

Quoted 1 line

(Or again we just don't allow mounting of fsverity on 32-bit kernels.)

What are the other file systems doing here? Unless we have a good reason to differ we should just follow the precedence.

While writing Merkle tree, file is read-only and there's no further writes except Merkle tree building. The file is truncated beforehand to remove any preallocated extents.

The Merkle tree is the only data XFS will write. As we don't want XFS to truncate file after we done writing, let's also skip truncation on fsverity files. Therefore, we also need to disable preallocations while writing merkle tree as we don't want any unused extents past the tree.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/xfs_iomap.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

fs/xfs/xfs_iomap.c +12 -3

index 04f39ea158..61aab5617f 100644
@@ -1938,7 +1938,9 @@
 		 * Determine the initial size of the preallocation.
 		 * We clean up any extra preallocation when the file is closed.
 		 */
-		if (xfs_has_allocsize(mp))
+		if (xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION))
+			prealloc_blocks = 0;
+		else if (xfs_has_allocsize(mp))
 			prealloc_blocks = mp->m_allocsize_blocks;
 		else if (allocfork == XFS_DATA_FORK)
 			prealloc_blocks = xfs_iomap_prealloc_size(ip, allocfork,

@@ -2065,6 +2067,13 @@
 	if (flags & IOMAP_FAULT)
 		return 0;
 
+	/*
+	 * While writing Merkle tree to disk we would not have any other
+	 * delayed allocations
+	 */
+	if (xfs_iflags_test(XFS_I(inode), XFS_VERITY_CONSTRUCTION))
+		return 0;
+
 	/* Nothing to do if we've written the entire delalloc extent */
 	start_byte = iomap_last_written_block(inode, offset, written);
 	end_byte = round_up(offset + length, i_blocksize(inode));

@@ -2109,6 +2118,7 @@
 	bool			shared = false;
 	unsigned int		lockmode = XFS_ILOCK_SHARED;
 	u64			seq;
+	int			iomap_flags;
 
 	ASSERT(!(flags & (IOMAP_WRITE | IOMAP_ZERO)));
 

@@ -2128,8 +2138,20 @@
 	if (error)
 		return error;
 	trace_xfs_iomap_found(ip, offset, length, XFS_DATA_FORK, &imap);
-	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags,
-				 shared ? IOMAP_F_SHARED : 0, seq);
+	iomap_flags = shared ? IOMAP_F_SHARED : 0;

fs/xfs/xfs_iomap.c +2 -0

+
+	return xfs_bmbt_to_iomap(ip, iomap, &imap, flags, iomap_flags, seq);
 }
 
 const struct iomap_ops xfs_read_iomap_ops = {

Signature · 2 lines

-- 
- Andrey

In the writeback path use unbound write interface, meaning that inode size is not updated and none of the file size checks are applied.

In read path let iomap know that data is stored beyond EOF via flag. This leads to skipping of post EOF zeroing.

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/xfs_aops.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

fs/xfs/xfs_aops.c +8 -1

index 56a5446384..30e38d5322 100644
@@ -22,6 +22,7 @@
 #include "xfs_icache.h"
 #include "xfs_zone_alloc.h"
 #include "xfs_rtgroup.h"
+#include "xfs_fsverity.h"
 
 struct xfs_writepage_ctx {
 	struct iomap_writepage_ctx ctx;

@@ -334,6 +335,7 @@
 	int			retries = 0;
 	int			error = 0;
 	unsigned int		*seq;
+	unsigned int		iomap_flags = 0;
 
 	if (xfs_is_shutdown(mp))
 		return -EIO;

@@ -427,7 +429,9 @@
 	    isnullstartblock(imap.br_startblock))
 		goto allocate_blocks;
 
-	xfs_bmbt_to_iomap(ip, &wpc->iomap, &imap, 0, 0, XFS_WPC(wpc)->data_seq);
+	if (xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION))
+		iomap_flags |= IOMAP_F_BEYOND_EOF;
+	xfs_bmbt_to_iomap(ip, &wpc->iomap, &imap, 0, iomap_flags, XFS_WPC(wpc)->data_seq);
 	trace_xfs_map_blocks_found(ip, offset, count, whichfork, &imap);
 	return 0;
 allocate_blocks:

@@ -470,6 +474,9 @@
 			wpc->iomap.length = cow_offset - wpc->iomap.offset;
 	}
 
+	if (offset >= XFS_FSVERITY_REGION_START)
+		wpc->iomap.flags |= IOMAP_F_BEYOND_EOF;
+
 	ASSERT(wpc->iomap.offset <= offset);
 	ASSERT(wpc->iomap.offset + wpc->iomap.length > offset);
 	trace_xfs_map_blocks_alloc(ip, offset, count, whichfork, &imap);

@@ -698,6 +705,17 @@
 			},
 		};
 

fs/xfs/xfs_aops.c +1 -0

+		}
 		return iomap_writepages(&wpc.ctx);
 	}
 }

Signature · 2 lines

-- 
- Andrey

fs/xfs/xfs_aops.c line 717

Quoted 1 line

fs/xfs/xfs_aops.c +1 -0

+			wpc.ctx.iomap.flags |= IOMAP_F_BEYOND_EOF;

But won't xfs_map_blocks reset wpc.ctx.iomap.flags?

/me realizes that you /are/ using writeback for writing the fsverity metadata now, so he'll go back and look at the iomap patches a little closer.

Quoted 3 lines · Darrick J. Wong

fs/xfs/xfs_aops.c line 717

Quoted 1 line

fs/xfs/xfs_aops.c +1 -0

+			wpc.ctx.iomap.flags |= IOMAP_F_BEYOND_EOF;

But won't xfs_map_blocks reset wpc.ctx.iomap.flags?

Quoted 4 lines

/me realizes that you /are/ using writeback for writing the fsverity metadata now, so he'll go back and look at the iomap patches a little closer.

The real question to me is why we're doing that, instead of doing a simple direct I/O-style I/O (or even doing actual dio using a bvec buffer)?

Add integration with fs-verity. XFS stores fs-verity descriptor and Merkle tree in the inode data fork at offset file offset (1 << 53).

The Merkle tree reading/writing is done through iomap interface. The data itself are read to the inode's page cache. When XFS reads from this region iomap doesn't call into fsverity to verify it against Merkle tree. For data, verification is done on BIO completion in a workqueue.

When fs-verity is enabled on an inode, the XFS_IVERITY_CONSTRUCTION flag is set meaning that the Merkle tree is being build. The initialization ends with storing of verity descriptor and setting inode on-disk flag (XFS_DIFLAG2_VERITY).

The descriptor is stored in a new block after the last Merkle tree block. The size of the descriptor is stored at the end of the last descriptor block (descriptor can be multiple blocks).

Signed-off-by Andrey Albershteyn <[email protected]>

---
 fs/xfs/Makefile        |   1 +
 fs/xfs/xfs_bmap_util.c |   7 +
 fs/xfs/xfs_fsverity.c  | 376 ++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fsverity.h  |  12 +
 fs/xfs/xfs_message.c   |   4 +
 fs/xfs/xfs_message.h   |   1 +
 fs/xfs/xfs_super.c     |  14 +
 7 files changed, 415 insertions(+), 0 deletions(-)

fs/xfs/Makefile +1 -0

index 5bf501cf82..ad66439db7 100644
@@ -147,6 +147,7 @@
 xfs-$(CONFIG_SYSCTL)		+= xfs_sysctl.o
 xfs-$(CONFIG_COMPAT)		+= xfs_ioctl32.o
 xfs-$(CONFIG_EXPORTFS_BLOCK_OPS)	+= xfs_pnfs.o
+xfs-$(CONFIG_FS_VERITY)		+= xfs_fsverity.o
 
 # notify failure
 ifeq ($(CONFIG_MEMORY_FAILURE),y)

fs/xfs/xfs_bmap_util.c +7 -0

index 2208a720ec..79a255a3ac 100644
@@ -31,6 +31,7 @@
 #include "xfs_rtbitmap.h"
 #include "xfs_rtgroup.h"
 #include "xfs_zone_alloc.h"
+#include <linux/fsverity.h>
 
 /* Kernel only BMAP related definitions and functions */
 

@@ -554,6 +555,12 @@
 		return false;
 
 	/*
+	 * Nothing to clean on fsverity inodes as they are read-only
+	 */
+	if (IS_VERITY(VFS_I(ip)))
+		return false;
+
+	/*
 	 * Check if there is an post-EOF extent to free.  If there are any
 	 * delalloc blocks attached to the inode (data fork delalloc
 	 * reservations or CoW extents of any kind), we need to free them so

fs/xfs/xfs_fsverity.c new file +58 -0

new file mode 100644
index 0000000000..691dc60778
@@ -0,0 +1,376 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2025 Red Hat, Inc.
+ */
+#include "xfs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_mount.h"
+#include "xfs_da_format.h"
+#include "xfs_da_btree.h"
+#include "xfs_inode.h"
+#include "xfs_log_format.h"
+#include "xfs_bmap_util.h"
+#include "xfs_log_format.h"
+#include "xfs_trans.h"
+#include "xfs_trace.h"
+#include "xfs_quota.h"
+#include "xfs_fsverity.h"
+#include "xfs_iomap.h"
+#include <linux/fsverity.h>
+#include <linux/pagemap.h>
+
+static int
+xfs_fsverity_read(
+	struct inode	*inode,
+	void		*buf,
+	size_t		count,
+	loff_t		pos)
+{
+	struct folio	*folio;
+	size_t		n;
+
+	while (count) {
+		folio = read_mapping_folio(inode->i_mapping, pos >> PAGE_SHIFT,
+					 NULL);
+		if (IS_ERR(folio))
+			return PTR_ERR(folio);
+
+		n = memcpy_from_file_folio(buf, folio, pos, count);
+		folio_put(folio);
+
+		buf += n;
+		pos += n;
+		count -= n;
+	}
+	return 0;
+}
+
+static int
+xfs_fsverity_write(
+	struct xfs_inode	*ip,
+	loff_t			pos,
+	size_t			length,
+	const void		*buf)
+{
+	int			ret;
+	struct iov_iter		iter;

fs/xfs/xfs_fsverity.c new file +58 -0

+		.ki_filp = NULL,
+		.ki_ioprio = get_current_ioprio(),
+		.ki_pos = pos,
+	};
+
+	iov_iter_kvec(&iter, WRITE, &kvec, 1, length);
+	ret = iomap_file_buffered_write(&iocb, &iter,
+			&xfs_buffered_write_iomap_ops, &xfs_iomap_write_ops,
+			VFS_I(ip));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Retrieve the verity descriptor.
+ */
+static int
+xfs_fsverity_get_descriptor(
+	struct inode		*inode,
+	void			*buf,
+	size_t			buf_size)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+	__be32			d_desc_size;
+	u32			desc_size;
+	u64			desc_size_pos;
+	int			error;
+	u64			desc_pos;
+	struct xfs_bmbt_irec	rec;
+	int			is_empty;
+	uint32_t		blocksize = i_blocksize(VFS_I(ip));
+	xfs_fileoff_t		last_block;
+
+	ASSERT(inode->i_flags & S_VERITY);
+	error = xfs_bmap_last_extent(NULL, ip, XFS_DATA_FORK, &rec, &is_empty);
+	if (error)
+		return error;
+
+	if (is_empty)
+		return -ENODATA;
+
+	last_block = (rec.br_startoff + rec.br_blockcount);
+	desc_size_pos = (last_block << ip->i_mount->m_sb.sb_blocklog) -
+			sizeof(__be32);
+	error = xfs_fsverity_read(inode, (char *)&d_desc_size,
+				  sizeof(d_desc_size), desc_size_pos);
+	if (error)
+		return error;
+
+	desc_size = be32_to_cpu(d_desc_size);
+	if (desc_size > FS_VERITY_MAX_DESCRIPTOR_SIZE || desc_size > desc_size_pos)
+		return -ERANGE;
+
+	if (!buf_size)
+		return desc_size;
+

fs/xfs/xfs_fsverity.c new file +35 -0

+
+static int
+xfs_fsverity_write_descriptor(
+	struct xfs_inode	*ip,
+	const void		*desc,
+	u32			desc_size,
+	u64			merkle_tree_size)
+{
+	int			error;
+	unsigned int		blksize = ip->i_mount->m_attr_geo->blksize;
+	u64			desc_pos = round_up(
+			XFS_FSVERITY_REGION_START | merkle_tree_size, blksize);
+	u64			desc_end = desc_pos + desc_size;
+	__be32			desc_size_disk = cpu_to_be32(desc_size);
+	u64			desc_size_pos =
+			round_up(desc_end + sizeof(desc_size_disk), blksize) -
+			sizeof(desc_size_disk);
+
+	error = xfs_fsverity_write(ip, desc_size_pos,
+				   sizeof(__be32),
+				   (const void *)&desc_size_disk);
+	if (error)
+		return error;
+
+	error = xfs_fsverity_write(ip, desc_pos, desc_size, desc);
+
+	return error;
+}
+
+/*
+ * Try to remove all the fsverity metadata after a failed enablement.
+ */
+static int
+xfs_fsverity_delete_metadata(
+	struct xfs_inode	*ip)