Re: [PATCH] xfs: do not propagate ENODATA disk errors into xattr code

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Aug 21, 2025 at 04:36:10PM -0500, Eric Sandeen wrote:
> ENODATA (aka ENOATTR) has a very specific meaning in the xfs xattr code;
> namely, that the requested attribute name could not be found.
> 
> However, a medium error from disk may also return ENODATA. At best,
> this medium error may escape to userspace as "attribute not found"
> when in fact it's an IO (disk) error.
> 
> At worst, we may oops in xfs_attr_leaf_get() when we do:
> 
> 	error = xfs_attr_leaf_hasname(args, &bp);
> 	if (error == -ENOATTR)  {
> 		xfs_trans_brelse(args->trans, bp);
> 		return error;
> 	}
> 
> because an ENODATA/ENOATTR error from disk leaves us with a null bp,
> and the xfs_trans_brelse will then null-deref it.
> 
> As discussed on the list, we really need to modify the lower level
> IO functions to trap all disk errors and ensure that we don't let
> unique errors like this leak up into higher xfs functions - many
> like this should be remapped to EIO.
> 
> However, this patch directly addresses a reported bug in the xattr
> code, and should be safe to backport to stable kernels. A larger-scope
> patch to handle more unique errors at lower levels can follow later.
> 
> (Note, prior to 07120f1abdff we did not oops, but we did return the
> wrong error code to userspace.)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> Cc: <stable@vger.kernel.org> # v5.9+
> ---
> 
> (I get it that sprinkling this around to 3 places might have an ick
> factor but I think it's necessary to make a surgical strike on this bug
> before we address the general problem.)
> 
> Thanks,
> -Eric
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fddb55605e0c..9b54cfb0e13d 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -478,6 +478,12 @@ xfs_attr3_leaf_read(
>  
>  	err = xfs_da_read_buf(tp, dp, bno, 0, bpp, XFS_ATTR_FORK,
>  			&xfs_attr3_leaf_buf_ops);
> +	/*
> +	 * ENODATA from disk implies a disk medium failure; ENODATA for
> +	 * xattrs means attribute not found, so disambiguate that here.
> +	 */
> +	if (err == -ENODATA)
> +		err = -EIO;

I think this first chunk isn't needed since you also changed
xfs_da_read_buf to filter the error code, right?

(Shifting towards the giant reconsideration of ENODATA -> EIO filtering)

Do we now also need to adjust the online fsck code to turn ENODATA into
a corruption report?  From __xchk_process_error:

	case -EFSBADCRC:
	case -EFSCORRUPTED:
		/* Note the badness but don't abort. */
		sc->sm->sm_flags |= errflag;
		*error = 0;
		fallthrough;
	default:
		trace_xchk_op_error(sc, agno, bno, *error, ret_ip);
		break;
	}

We only flag corruptions for these two error codes, but ENODATA from the
block layer means "critical medium error".  I take that to mean the
media has permanently lost whatever was persisted there, right?

--D

>  	if (err || !(*bpp))
>  		return err;
>  
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 4c44ce1c8a64..bff3dc226f81 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -435,6 +435,13 @@ xfs_attr_rmtval_get(
>  					0, &bp, &xfs_attr3_rmt_buf_ops);
>  			if (xfs_metadata_is_sick(error))
>  				xfs_dirattr_mark_sick(args->dp, XFS_ATTR_FORK);
> +			/*
> +			 * ENODATA from disk implies a disk medium failure;
> +			 * ENODATA for xattrs means attribute not found, so
> +			 * disambiguate that here.
> +			 */
> +			if (error == -ENODATA)
> +				error = -EIO;
>  			if (error)
>  				return error;
>  
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 17d9e6154f19..723a0643b838 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -2833,6 +2833,12 @@ xfs_da_read_buf(
>  			&bp, ops);
>  	if (xfs_metadata_is_sick(error))
>  		xfs_dirattr_mark_sick(dp, whichfork);
> +	/*
> +	 * ENODATA from disk implies a disk medium failure; ENODATA for
> +	 * xattrs means attribute not found, so disambiguate that here.
> +	 */
> +	if (error == -ENODATA && whichfork == XFS_ATTR_FORK)
> +		error = -EIO;
>  	if (error)
>  		goto out_free;
>  
> 
>