Re: [f2fs-dev] [PATCH 2/2] f2fs: fix error handling in f2fs_end_enable_verity()

[Chao Yu <yuchao0@huawei.com>]

On 2021/3/3 4:04, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> f2fs didn't properly clean up if verity failed to be enabled on a file:
> 
> - It left verity metadata (pages past EOF) in the page cache, which
>    would be exposed to userspace if the file was later extended.
> 
> - It didn't truncate the verity metadata at all (either from cache or
>    from disk) if an error occurred while setting the verity bit.
> 
> Fix these bugs by adding a call to truncate_inode_pages() and ensuring
> that we truncate the verity metadata (both from cache and from disk) in
> all error paths.  Also rework the code to cleanly separate the success
> path from the error paths, which makes it much easier to understand.
> 
> Reported-by: Yunlei He <heyunlei@hihonor.com>
> Fixes: 95ae251fe828 ("f2fs: add fs-verity support")
> Cc: <stable@vger.kernel.org> # v5.4+
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>   fs/f2fs/verity.c | 61 ++++++++++++++++++++++++++++++++----------------
>   1 file changed, 41 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
> index 054ec852b5ea4..2db89967fde37 100644
> --- a/fs/f2fs/verity.c
> +++ b/fs/f2fs/verity.c
> @@ -160,31 +160,52 @@ static int f2fs_end_enable_verity(struct file *filp, const void *desc,
>   	};
>   	int err = 0;
>   
> -	if (desc != NULL) {
> -		/* Succeeded; write the verity descriptor. */
> -		err = pagecache_write(inode, desc, desc_size, desc_pos);
> +	/*
> +	 * If an error already occurred (which fs/verity/ signals by passing
> +	 * desc == NULL), then only clean-up is needed.
> +	 */
> +	if (desc == NULL)
> +		goto cleanup;
>   
> -		/* Write all pages before clearing FI_VERITY_IN_PROGRESS. */
> -		if (!err)
> -			err = filemap_write_and_wait(inode->i_mapping);
> -	}
> +	/* Append the verity descriptor. */
> +	err = pagecache_write(inode, desc, desc_size, desc_pos);
> +	if (err)
> +		goto cleanup;
>   
> -	/* If we failed, truncate anything we wrote past i_size. */
> -	if (desc == NULL || err)
> -		f2fs_truncate(inode);
> +	/*
> +	 * Write all pages (both data and verity metadata).  Note that this must
> +	 * happen before clearing FI_VERITY_IN_PROGRESS; otherwise pages beyond
> +	 * i_size won't be written properly.  For crash consistency, this also
> +	 * must happen before the verity inode flag gets persisted.
> +	 */
> +	err = filemap_write_and_wait(inode->i_mapping);
> +	if (err)
> +		goto cleanup;
> +
> +	/* Set the verity xattr. */
> +	err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_VERITY,
> +			    F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc),
> +			    NULL, XATTR_CREATE);
> +	if (err)
> +		goto cleanup;
> +
> +	/* Finally, set the verity inode flag. */
> +	file_set_verity(inode);
> +	f2fs_set_inode_flags(inode);
> +	f2fs_mark_inode_dirty_sync(inode, true);
>   
>   	clear_inode_flag(inode, FI_VERITY_IN_PROGRESS);
> +	return 0;
>   
> -	if (desc != NULL && !err) {
> -		err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_VERITY,
> -				    F2FS_XATTR_NAME_VERITY, &dloc, sizeof(dloc),
> -				    NULL, XATTR_CREATE);
> -		if (!err) {
> -			file_set_verity(inode);
> -			f2fs_set_inode_flags(inode);
> -			f2fs_mark_inode_dirty_sync(inode, true);
> -		}
> -	}
> +cleanup:
> +	/*
> +	 * Verity failed to be enabled, so clean up by truncating any verity
> +	 * metadata that was written beyond i_size (both from cache and from
> +	 * disk) and clearing FI_VERITY_IN_PROGRESS.
> +	 */
> +	truncate_inode_pages(inode->i_mapping, inode->i_size);
> +	f2fs_truncate(inode);

Eric,

Truncation can fail due to a lot of reasons, if we fail in f2fs_truncate(),
do we need to at least print a message here? or it allows to keep those
meta/data silently.

One other concern is that how do you think of covering truncate_inode_pages &
f2fs_truncate with F2FS_I(inode)->i_gc_rwsem[WRITE] lock to avoid racing with
GC, so that page cache won't be revalidated after truncate_inode_pages().

Thanks,

> +	clear_inode_flag(inode, FI_VERITY_IN_PROGRESS); >   	return err;
>   }
>   
>