fs-verity fixups

PATCH In review Eric Biggers 2 participants 13 messages Aug 11, 2019 – Aug 18, 2019

Subsystems (4)

ext4 file system F2FS file system fscrypt: file system level encryption support fs-verity: read-only file-based authenticity protection

lore

Share & tools

lore

Eric Biggers

author maintainer

2019-08-11 21:37 UTC #0 lore raw

cover fs-verity fixups

A few fixes and cleanups for fs-verity.

If there are no objections, I'll fold these into the original patches.

Eric Biggers (6):
fs-verity: fix crash on read error in build_merkle_tree_level()
ext4: skip truncate when verity in progress in ->write_begin()
f2fs: skip truncate when verity in progress in ->write_begin()
ext4: remove ext4_bio_encrypted()
ext4: fix comment in ext4_end_enable_verity()
f2fs: use EFSCORRUPTED in f2fs_get_verity_descriptor()

 fs/ext4/inode.c    |  7 +++++--
 fs/ext4/readpage.c |  9 ---------
 fs/ext4/verity.c   |  2 +-
 fs/f2fs/data.c     |  2 +-
 fs/f2fs/verity.c   |  2 +-
 fs/verity/enable.c | 24 ++++++++++++++++--------
 6 files changed, 24 insertions(+), 22 deletions(-)

Signature · 1 line

2.22.0

Eric Biggers

author maintainer

2019-08-11 21:37 UTC #1 lore raw

PATCH 1/6 fs-verity: fix crash on read error in build_merkle_tree_level()

From: Eric Biggers <[email protected]>

In build_merkle_tree_level(), use a separate fsverity_err() statement when failing to read a data page, rather than incorrectly reusing the one for failure to read a Merkle tree page and accessing level_start[] out of bounds due to 'params->level_start[level - 1]' when level == 0.

Fixes 248676649d53 ("fs-verity: implement FS_IOC_ENABLE_VERITY ioctl")

Signed-off-by Eric Biggers <[email protected]>

---
 fs/verity/enable.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

fs/verity/enable.c +16 -8

index 3371d51563962..eabc6ac199064 100644
@@ -43,19 +43,27 @@ static int build_merkle_tree_level(struct inode *inode, unsigned int level,
 			pr_debug("Hashing block %llu of %llu for level %u\n",
 				 i + 1, num_blocks_to_hash, level);
 
-		if (level == 0)
+		if (level == 0) {
 			/* Leaf: hashing a data block */
 			src_page = read_mapping_page(inode->i_mapping, i, NULL);
-		else
+			if (IS_ERR(src_page)) {
+				err = PTR_ERR(src_page);
+				fsverity_err(inode,
+					     "Error %d reading data page %llu",
+					     err, i);
+				return err;
+			}
+		} else {
 			/* Non-leaf: hashing hash block from level below */
 			src_page = vops->read_merkle_tree_page(inode,
 					params->level_start[level - 1] + i);
-		if (IS_ERR(src_page)) {
-			err = PTR_ERR(src_page);
-			fsverity_err(inode,
-				     "Error %d reading Merkle tree page %llu",
-				     err, params->level_start[level - 1] + i);
-			return err;
+			if (IS_ERR(src_page)) {
+				err = PTR_ERR(src_page);
+				fsverity_err(inode,
+					     "Error %d reading Merkle tree page %llu",
+					     err, params->level_start[level - 1] + i);
+				return err;
+			}
 		}
 
 		err = fsverity_hash_page(params, inode, req, src_page,

Signature · 1 line

2.22.0

Eric Biggers

author maintainer

2019-08-11 21:35 UTC #2 lore raw

PATCH 2/6 ext4: skip truncate when verity in progress in ->write_begin()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

When an error (e.g. ENOSPC) occurs during ext4_write_begin() when called from ext4_write_merkle_tree_block(), skip truncating the file. i_size is not meaningful in this case, and the truncation is handled by ext4_end_enable_verity() instead. Also, this was triggering the WARN_ON(!inode_is_locked(inode)) in ext4_truncate().

Fixes ea54d7e4c0f8 ("ext4: add basic fs-verity support")

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

fs/ext4/inode.c +5 -2

index b2c8d09acf652..cf0fce1173a4c 100644
@@ -1340,6 +1340,9 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 	}
 
 	if (ret) {
+		bool extended = (pos + len > inode->i_size) &&
+				!ext4_verity_in_progress(inode);
+
 		unlock_page(page);
 		/*
 		 * __block_write_begin may have instantiated a few blocks

@@ -1349,11 +1352,11 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
 		 * Add inode to orphan list in case we crash before
 		 * truncate finishes
 		 */
-		if (pos + len > inode->i_size && ext4_can_truncate(inode))
+		if (extended && ext4_can_truncate(inode))
 			ext4_orphan_add(handle, inode);
 
 		ext4_journal_stop(handle);
-		if (pos + len > inode->i_size) {
+		if (extended) {
 			ext4_truncate_failed_write(inode);
 			/*
 			 * If truncate failed early the inode might

Signature · 8 lines

2.22.0



_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author maintainer

2019-08-11 21:37 UTC #3 lore raw

PATCH 3/6 f2fs: skip truncate when verity in progress in ->write_begin()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called from f2fs_write_merkle_tree_block(), skip truncating the file. i_size is not meaningful in this case, and the truncation is handled by f2fs_end_enable_verity() instead.

Fixes 60d7bf0f790f ("f2fs: add fs-verity support")

Signed-off-by Eric Biggers <[email protected]>

---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/f2fs/data.c +1 -1

index 3f525f8a3a5fa..00b03fb87bd9b 100644
@@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 	loff_t i_size = i_size_read(inode);
 
-	if (to > i_size) {
+	if (to > i_size && !f2fs_verity_in_progress(inode)) {
 		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 		down_write(&F2FS_I(inode)->i_mmap_sem);

Signature · 1 line

2.22.0

Chao Yu

2019-08-12 12:25 UTC #4 lore raw

Re: [PATCH 3/6] f2fs: skip truncate when verity in progress in ->write_begin()

Hi Eric,

Quoted 18 lines · Eric Biggers

From: Eric Biggers <[email protected]>
When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called from f2fs_write_merkle_tree_block(), skip truncating the file. i_size is not meaningful in this case, and the truncation is handled by f2fs_end_enable_verity() instead.
Fixes 60d7bf0f790f ("f2fs: add fs-verity support")
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/data.c +0 -0
index 3f525f8a3a5fa..00b03fb87bd9b 100644

fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2479

Quoted 4 lines

fs/f2fs/data.c +0 -1

@@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 	loff_t i_size = i_size_read(inode);
 
-	if (to > i_size) {

Maybe adding one single line comment to mention that it's redundant/unnecessary truncation here is better, if I didn't misunderstand this.

Thanks,

Quoted 5 lines

fs/f2fs/data.c +1 -0

+	if (to > i_size && !f2fs_verity_in_progress(inode)) {
 		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 		down_write(&F2FS_I(inode)->i_mmap_sem);

Eric Biggers

author maintainer

2019-08-12 22:58 UTC #5 lore raw

Re: [PATCH 3/6] f2fs: skip truncate when verity in progress in ->write_begin()

Hi Chao,

Quoted 29 lines · Chao Yu

Hi Eric,
Quoted 18 lines · Eric Biggers
From: Eric Biggers <[email protected]>
When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called from f2fs_write_merkle_tree_block(), skip truncating the file. i_size is not meaningful in this case, and the truncation is handled by f2fs_end_enable_verity() instead.
Fixes 60d7bf0f790f ("f2fs: add fs-verity support")
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/data.c +0 -0
index 3f525f8a3a5fa..00b03fb87bd9b 100644
fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2479
Quoted 4 lines
fs/f2fs/data.c +0 -1
@@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 	loff_t i_size = i_size_read(inode);
 
-	if (to > i_size) {
Maybe adding one single line comment to mention that it's redundant/unnecessary truncation here is better, if I didn't misunderstand this.

Quoted 6 lines

Thanks,
fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2482
Quoted 4 lines
fs/f2fs/data.c +1 -0
+	if (to > i_size && !f2fs_verity_in_progress(inode)) {
 		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 		down_write(&F2FS_I(inode)->i_mmap_sem);
 

Do you mean add a comment instead of the !f2fs_verity_in_progress() check, or in addition to it? ->write_begin(), ->writepages(), and ->write_end() are all supposed to ignore i_size when verity is in progress, so I don't think this particular part should be different, even if technically it's still correct to truncate twice. Also, ext4 needs this check in its ->write_begin() for locking reasons; we should keep f2fs similar.

How about having both a comment and the check, like:

        /* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
        if (to > i_size && !f2fs_verity_in_progress(inode)) {

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Chao Yu

2019-08-13 02:40 UTC #6 lore raw

Re: [PATCH 3/6] f2fs: skip truncate when verity in progress in ->write_begin()

Hi Eric,

Quoted 34 lines · Eric Biggers

Hi Chao,
Quoted 31 lines · Chao Yu
Hi Eric,
Quoted 18 lines · Eric Biggers
From: Eric Biggers <[email protected]>
When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called from f2fs_write_merkle_tree_block(), skip truncating the file. i_size is not meaningful in this case, and the truncation is handled by f2fs_end_enable_verity() instead.
Fixes 60d7bf0f790f ("f2fs: add fs-verity support")
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/data.c +0 -0
index 3f525f8a3a5fa..00b03fb87bd9b 100644
fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2479
Quoted 4 lines
fs/f2fs/data.c +0 -1
@@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 	loff_t i_size = i_size_read(inode);
 
-	if (to > i_size) {
Maybe adding one single line comment to mention that it's redundant/unnecessary truncation here is better, if I didn't misunderstand this.
Thanks,

Quoted 7 lines

Quoted 4 lines
fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2482
Quoted 4 lines
fs/f2fs/data.c +1 -0
+	if (to > i_size && !f2fs_verity_in_progress(inode)) {
 		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 		down_write(&F2FS_I(inode)->i_mmap_sem);
 
Do you mean add a comment instead of the !f2fs_verity_in_progress() check, or in addition to it? ->write_begin(), ->writepages(), and ->write_end() are all

Sorry, I didn't make this very clear, I meant adding the comment in addition on above change.

Quoted 4 lines

supposed to ignore i_size when verity is in progress, so I don't think this particular part should be different, even if technically it's still correct to truncate twice. Also, ext4 needs this check in its ->write_begin() for locking reasons; we should keep f2fs similar.

Agreed.

Quoted 5 lines

How about having both a comment and the check, like:

        /* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
        if (to > i_size && !f2fs_verity_in_progress(inode)) {

The comment looks good to me. :)

Thanks,

Quoted 4 lines

- Eric
.

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Eric Biggers

author maintainer

2019-08-18 20:24 UTC #7 lore raw

Re: [f2fs-dev] [PATCH 3/6] f2fs: skip truncate when verity in progress in ->write_begin()

Quoted 56 lines · Chao Yu

Hi Eric,
Quoted 34 lines · Eric Biggers
Hi Chao,
Quoted 31 lines · Chao Yu
Hi Eric,
Quoted 18 lines · Eric Biggers
From: Eric Biggers <[email protected]>
When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called from f2fs_write_merkle_tree_block(), skip truncating the file. i_size is not meaningful in this case, and the truncation is handled by f2fs_end_enable_verity() instead.
Fixes 60d7bf0f790f ("f2fs: add fs-verity support")
Signed-off-by Eric Biggers <[email protected]>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/data.c +0 -0
index 3f525f8a3a5fa..00b03fb87bd9b 100644
fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2479
Quoted 4 lines
fs/f2fs/data.c +0 -1
@@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 	loff_t i_size = i_size_read(inode);
 
-	if (to > i_size) {
Maybe adding one single line comment to mention that it's redundant/unnecessary truncation here is better, if I didn't misunderstand this.
Thanks,
Quoted 7 lines
Quoted 4 lines
fs/f2fs/data.c static void f2fs_write_failed(struct address_space *mapping, loff_t to) · line 2482
Quoted 4 lines
fs/f2fs/data.c +1 -0
+	if (to > i_size && !f2fs_verity_in_progress(inode)) {
 		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 		down_write(&F2FS_I(inode)->i_mmap_sem);
 
Do you mean add a comment instead of the !f2fs_verity_in_progress() check, or in addition to it? ->write_begin(), ->writepages(), and ->write_end() are all
Sorry, I didn't make this very clear, I meant adding the comment in addition on above change.
Quoted 4 lines
supposed to ignore i_size when verity is in progress, so I don't think this particular part should be different, even if technically it's still correct to truncate twice. Also, ext4 needs this check in its ->write_begin() for locking reasons; we should keep f2fs similar.
Agreed.
Quoted 1 line

Quoted 14 lines

Quoted 4 lines
How about having both a comment and the check, like:
        /* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
        if (to > i_size && !f2fs_verity_in_progress(inode)) {
The comment looks good to me. :)
Thanks,
Quoted 4 lines
- Eric
.

Okay, this is what I applied:

fs/f2fs/data.c +2 -1

index 3f525f8a3a5f..54cad80acb7d 100644
@@ -2476,7 +2476,8 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
 	struct inode *inode = mapping->host;
 	loff_t i_size = i_size_read(inode);
 
-	if (to > i_size) {
+	/* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
+	if (to > i_size && !f2fs_verity_in_progress(inode)) {
 		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
 		down_write(&F2FS_I(inode)->i_mmap_sem);

Eric Biggers

author maintainer

2019-08-11 21:37 UTC #8 lore raw

PATCH 4/6 ext4: remove ext4_bio_encrypted()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

ext4_bio_encrypted() is unused following commit 4e47a0d40dac ("ext4: add fs-verity read support"), so remove it.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/readpage.c | 9 ---------
 1 file changed, 9 deletions(-)

fs/ext4/readpage.c +0 -9

index ec8aeab3af65a..a30b203fa461c 100644
@@ -52,15 +52,6 @@
 static struct kmem_cache *bio_post_read_ctx_cache;
 static mempool_t *bio_post_read_ctx_pool;
 
-static inline bool ext4_bio_encrypted(struct bio *bio)
-{
-#ifdef CONFIG_FS_ENCRYPTION
-	return unlikely(bio->bi_private != NULL);
-#else
-	return false;
-#endif
-}
-
 /* postprocessing steps for read bios */
 enum bio_post_read_step {
 	STEP_INITIAL = 0,

Signature · 1 line

2.22.0

Eric Biggers

author maintainer

2019-08-11 21:37 UTC #9 lore raw

PATCH 5/6 ext4: fix comment in ext4_end_enable_verity()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

Signed-off-by Eric Biggers <[email protected]>

---
 fs/ext4/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/ext4/verity.c +1 -1

index bb0a3b8e6ea71..d0d8a9795dd62 100644
@@ -196,7 +196,7 @@ static int ext4_end_enable_verity(struct file *filp, const void *desc,
 				  size_t desc_size, u64 merkle_tree_size)
 {
 	struct inode *inode = file_inode(filp);
-	const int credits = 2; /* superblock and inode for ext4_orphan_add() */
+	const int credits = 2; /* superblock and inode for ext4_orphan_del() */
 	handle_t *handle;
 	int err = 0;
 	int err2;

Signature · 1 line

2.22.0

Eric Biggers

author maintainer

2019-08-11 21:37 UTC #10 lore raw

PATCH 6/6 f2fs: use EFSCORRUPTED in f2fs_get_verity_descriptor()

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

EFSCORRUPTED is now defined in f2fs.h, so use it instead of EUCLEAN.

Signed-off-by Eric Biggers <[email protected]>

---
 fs/f2fs/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/f2fs/verity.c +1 -1

index 6bc3470d99d00..a401ef72bc821 100644
@@ -210,7 +210,7 @@ static int f2fs_get_verity_descriptor(struct inode *inode, void *buf,
 	if (pos + size < pos || pos + size > inode->i_sb->s_maxbytes ||
 	    pos < f2fs_verity_metadata_pos(inode) || size > INT_MAX) {
 		f2fs_warn(F2FS_I_SB(inode), "invalid verity xattr");
-		return -EUCLEAN; /* EFSCORRUPTED */
+		return -EFSCORRUPTED;
 	}
 	if (buf_size) {
 		if (size > buf_size)

Signature · 1 line

2.22.0

Chao Yu

Acked-by on v1 6/6

2019-08-12 12:26 UTC #11 lore raw

Re: [PATCH 6/6] f2fs: use EFSCORRUPTED in f2fs_get_verity_descriptor()

Quoted 5 lines · Eric Biggers

From: Eric Biggers <[email protected]>
EFSCORRUPTED is now defined in f2fs.h, so use it instead of EUCLEAN.
Signed-off-by Eric Biggers <[email protected]>

Acked-by Chao Yu <[email protected]>

Thanks,

Eric Biggers

author maintainer

2019-08-18 20:23 UTC #12 lore raw

In reply to Eric Biggers

Quoted 12 lines · Eric Biggers

A few fixes and cleanups for fs-verity.
If there are no objections, I'll fold these into the original patches.
Eric Biggers (6):
fs-verity: fix crash on read error in build_merkle_tree_level()
ext4: skip truncate when verity in progress in ->write_begin()
f2fs: skip truncate when verity in progress in ->write_begin()
ext4: remove ext4_bio_encrypted()
ext4: fix comment in ext4_end_enable_verity()
f2fs: use EFSCORRUPTED in f2fs_get_verity_descriptor()

Quoted 11 lines

 fs/ext4/inode.c    |  7 +++++--
 fs/ext4/readpage.c |  9 ---------
 fs/ext4/verity.c   |  2 +-
 fs/f2fs/data.c     |  2 +-
 fs/f2fs/verity.c   |  2 +-
 fs/verity/enable.c | 24 ++++++++++++++++--------
 6 files changed, 24 insertions(+), 22 deletions(-)

Signature · 1 line

2.22.0

I've gone ahead and folded in these fixes.

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel