[PATCH 0/6] fs-verity fixups

[PATCH 0/6] fs-verity fixups

[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()

 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(-)

-- 
2.22.0

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

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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 <ebiggers@google.com>
---
 fs/verity/enable.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/verity/enable.c b/fs/verity/enable.c
index 3371d51563962..eabc6ac199064 100644
--- a/fs/verity/enable.c
+++ b/fs/verity/enable.c
@@ -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,
-- 
2.22.0

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

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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 <ebiggers@google.com>
---
 fs/ext4/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index b2c8d09acf652..cf0fce1173a4c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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
-- 
2.22.0



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

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

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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 <ebiggers@google.com>
---
 fs/f2fs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3f525f8a3a5fa..00b03fb87bd9b 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -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);
 
-- 
2.22.0

[PATCH 4/6] ext4: remove ext4_bio_encrypted()

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/readpage.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index ec8aeab3af65a..a30b203fa461c 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -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,
-- 
2.22.0

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

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index bb0a3b8e6ea71..d0d8a9795dd62 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -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;
-- 
2.22.0

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

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

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

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
index 6bc3470d99d00..a401ef72bc821 100644
--- a/fs/f2fs/verity.c
+++ b/fs/f2fs/verity.c
@@ -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)
-- 
2.22.0

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

[Chao Yu]

Hi Eric,

On 2019/8/12 5:35, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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 <ebiggers@google.com>
> ---
>  fs/f2fs/data.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 3f525f8a3a5fa..00b03fb87bd9b 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -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,

> +	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);
>  
> 

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

[Chao Yu]

On 2019/8/12 5:35, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> EFSCORRUPTED is now defined in f2fs.h, so use it instead of EUCLEAN.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,

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

[Eric Biggers]

Hi Chao,

On Mon, Aug 12, 2019 at 08:25:33PM +0800, Chao Yu wrote:
> Hi Eric,
> 
> On 2019/8/12 5:35, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > 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 <ebiggers@google.com>
> > ---
> >  fs/f2fs/data.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 3f525f8a3a5fa..00b03fb87bd9b 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -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,
> 
> > +	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
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Chao Yu]

Hi Eric,

On 2019/8/13 6:58, Eric Biggers wrote:
> Hi Chao,
> 
> On Mon, Aug 12, 2019 at 08:25:33PM +0800, Chao Yu wrote:
>> Hi Eric,
>>
>> On 2019/8/12 5:35, Eric Biggers wrote:
>>> From: Eric Biggers <ebiggers@google.com>
>>>
>>> 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 <ebiggers@google.com>
>>> ---
>>>  fs/f2fs/data.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 3f525f8a3a5fa..00b03fb87bd9b 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -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,
>>
>>> +	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.

> 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.

> 
> 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,

> 
> - 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 0/6] fs-verity fixups

[Eric Biggers]

On Sun, Aug 11, 2019 at 02:35:51PM -0700, Eric Biggers wrote:
> 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(-)
> 
> -- 
> 2.22.0
> 

I've gone ahead and folded in these fixes.

- 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 3/6] f2fs: skip truncate when verity in progress in ->write_begin()

[Eric Biggers]

On Tue, Aug 13, 2019 at 10:40:39AM +0800, Chao Yu wrote:
> Hi Eric,
> 
> On 2019/8/13 6:58, Eric Biggers wrote:
> > Hi Chao,
> > 
> > On Mon, Aug 12, 2019 at 08:25:33PM +0800, Chao Yu wrote:
> >> Hi Eric,
> >>
> >> On 2019/8/12 5:35, Eric Biggers wrote:
> >>> From: Eric Biggers <ebiggers@google.com>
> >>>
> >>> 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 <ebiggers@google.com>
> >>> ---
> >>>  fs/f2fs/data.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 3f525f8a3a5fa..00b03fb87bd9b 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -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,
> >>
> >>> +	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.
> 
> > 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.
> 
> > 
> > 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,
> 
> > 
> > - Eric
> > .
> > 
> 

Okay, this is what I applied:

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 3f525f8a3a5f..54cad80acb7d 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -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);