diff mbox series

[31/40] xfs: better reporting and error handling in xfs_drop_merkle_tree

Message ID 171069246408.2684506.9245902616854244173.stgit@frogsfrogsfrogs (mailing list archive)
State Superseded
Headers show
Series [01/40] fsverity: remove hash page spin lock | expand

Commit Message

Darrick J. Wong March 17, 2024, 4:31 p.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

xfs_drop_merkle_tree is responsible for removing the fsverity metadata
after a failed attempt to enable fsverity for a file.  However, if the
enablement process fails before the verity descriptor is written to the
file, the cleanup function will trip the WARN_ON.  The error code in
that case is ENOATTR, which isn't worth logging about.

Fix that return code handling, fix the tree block removal loop not to
return early with ENOATTR, and improve the logging so that we actually
capture what kind of error occurred.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_verity.c |   25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Comments

Andrey Albershteyn March 18, 2024, 5:51 p.m. UTC | #1
On 2024-03-17 09:31:28, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> xfs_drop_merkle_tree is responsible for removing the fsverity metadata
> after a failed attempt to enable fsverity for a file.  However, if the
> enablement process fails before the verity descriptor is written to the
> file, the cleanup function will trip the WARN_ON.  The error code in
> that case is ENOATTR, which isn't worth logging about.
> 
> Fix that return code handling, fix the tree block removal loop not to
> return early with ENOATTR, and improve the logging so that we actually
> capture what kind of error occurred.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Looks good to me:
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>

> ---
>  fs/xfs/xfs_verity.c |   25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_verity.c b/fs/xfs/xfs_verity.c
> index db43e017f10e..32891ae42c47 100644
> --- a/fs/xfs/xfs_verity.c
> +++ b/fs/xfs/xfs_verity.c
> @@ -472,15 +472,14 @@ xfs_verity_begin_enable(
>  			tree_blocksize);
>  }
>  
> +/* Try to remove all the fsverity metadata after a failed enablement. */
>  static int
> -xfs_drop_merkle_tree(
> +xfs_verity_drop_incomplete_tree(
>  	struct xfs_inode		*ip,
>  	u64				merkle_tree_size,
>  	unsigned int			tree_blocksize)
>  {
>  	struct xfs_verity_merkle_key	name;
> -	int				error = 0;
> -	u64				offset = 0;
>  	struct xfs_da_args		args = {
>  		.dp			= ip,
>  		.whichfork		= XFS_ATTR_FORK,
> @@ -491,6 +490,8 @@ xfs_drop_merkle_tree(
>  		/* NULL value make xfs_attr_set remove the attr */
>  		.value			= NULL,
>  	};
> +	u64				offset;
> +	int				error;
>  
>  	if (!merkle_tree_size)
>  		return 0;
> @@ -498,6 +499,8 @@ xfs_drop_merkle_tree(
>  	for (offset = 0; offset < merkle_tree_size; offset += tree_blocksize) {
>  		xfs_verity_merkle_key_to_disk(&name, offset);
>  		error = xfs_attr_set(&args);
> +		if (error == -ENOATTR)
> +			error = 0;
>  		if (error)
>  			return error;
>  	}
> @@ -505,7 +508,8 @@ xfs_drop_merkle_tree(
>  	args.name = (const uint8_t *)XFS_VERITY_DESCRIPTOR_NAME;
>  	args.namelen = XFS_VERITY_DESCRIPTOR_NAME_LEN;
>  	error = xfs_attr_set(&args);
> -
> +	if (error == -ENOATTR)
> +		return 0;
>  	return error;
>  }
>  
> @@ -564,9 +568,16 @@ xfs_verity_end_enable(
>  		inode->i_flags |= S_VERITY;
>  
>  out:
> -	if (error)
> -		WARN_ON_ONCE(xfs_drop_merkle_tree(ip, merkle_tree_size,
> -						  tree_blocksize));
> +	if (error) {
> +		int	error2;
> +
> +		error2 = xfs_verity_drop_incomplete_tree(ip, merkle_tree_size,
> +				tree_blocksize);
> +		if (error2)
> +			xfs_alert(ip->i_mount,
> + "ino 0x%llx failed to clean up new fsverity metadata, err %d",
> +					ip->i_ino, error2);
> +	}
>  
>  	xfs_iflags_clear(ip, XFS_VERITY_CONSTRUCTION);
>  	return error;
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_verity.c b/fs/xfs/xfs_verity.c
index db43e017f10e..32891ae42c47 100644
--- a/fs/xfs/xfs_verity.c
+++ b/fs/xfs/xfs_verity.c
@@ -472,15 +472,14 @@  xfs_verity_begin_enable(
 			tree_blocksize);
 }
 
+/* Try to remove all the fsverity metadata after a failed enablement. */
 static int
-xfs_drop_merkle_tree(
+xfs_verity_drop_incomplete_tree(
 	struct xfs_inode		*ip,
 	u64				merkle_tree_size,
 	unsigned int			tree_blocksize)
 {
 	struct xfs_verity_merkle_key	name;
-	int				error = 0;
-	u64				offset = 0;
 	struct xfs_da_args		args = {
 		.dp			= ip,
 		.whichfork		= XFS_ATTR_FORK,
@@ -491,6 +490,8 @@  xfs_drop_merkle_tree(
 		/* NULL value make xfs_attr_set remove the attr */
 		.value			= NULL,
 	};
+	u64				offset;
+	int				error;
 
 	if (!merkle_tree_size)
 		return 0;
@@ -498,6 +499,8 @@  xfs_drop_merkle_tree(
 	for (offset = 0; offset < merkle_tree_size; offset += tree_blocksize) {
 		xfs_verity_merkle_key_to_disk(&name, offset);
 		error = xfs_attr_set(&args);
+		if (error == -ENOATTR)
+			error = 0;
 		if (error)
 			return error;
 	}
@@ -505,7 +508,8 @@  xfs_drop_merkle_tree(
 	args.name = (const uint8_t *)XFS_VERITY_DESCRIPTOR_NAME;
 	args.namelen = XFS_VERITY_DESCRIPTOR_NAME_LEN;
 	error = xfs_attr_set(&args);
-
+	if (error == -ENOATTR)
+		return 0;
 	return error;
 }
 
@@ -564,9 +568,16 @@  xfs_verity_end_enable(
 		inode->i_flags |= S_VERITY;
 
 out:
-	if (error)
-		WARN_ON_ONCE(xfs_drop_merkle_tree(ip, merkle_tree_size,
-						  tree_blocksize));
+	if (error) {
+		int	error2;
+
+		error2 = xfs_verity_drop_incomplete_tree(ip, merkle_tree_size,
+				tree_blocksize);
+		if (error2)
+			xfs_alert(ip->i_mount,
+ "ino 0x%llx failed to clean up new fsverity metadata, err %d",
+					ip->i_ino, error2);
+	}
 
 	xfs_iflags_clear(ip, XFS_VERITY_CONSTRUCTION);
 	return error;