Re: [PATCH 12/12] xfs: remove the NULL fork handling in xfs_bmapi_read

[Brian Foster <bfoster@redhat.com>]

On Fri, May 01, 2020 at 10:14:24AM +0200, Christoph Hellwig wrote:
> Now that we fully verify the inode forks before they are added to the
> inode cache, the crash reported in
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=204031
> 
> can't happen anymore, as we'll never let an inode that has inconsistent
> nextents counts vs the presence of an in-core attr fork leak into the
> inactivate code path.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 76be1a18e2442..4246f2fd5b144 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3891,7 +3891,8 @@ xfs_bmapi_read(
>  	int			flags)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
> -	struct xfs_ifork	*ifp;
> +	int			whichfork = xfs_bmapi_whichfork(flags);
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
>  	struct xfs_bmbt_irec	got;
>  	xfs_fileoff_t		obno;
>  	xfs_fileoff_t		end;
> @@ -3899,7 +3900,6 @@ xfs_bmapi_read(
>  	int			error;
>  	bool			eof = false;
>  	int			n = 0;
> -	int			whichfork = xfs_bmapi_whichfork(flags);
>  
>  	ASSERT(*nmap >= 1);
>  	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
> @@ -3915,21 +3915,6 @@ xfs_bmapi_read(
>  
>  	XFS_STATS_INC(mp, xs_blk_mapr);
>  
> -	ifp = XFS_IFORK_PTR(ip, whichfork);
> -	if (!ifp) {
> -		/*
> -		 * A missing attr ifork implies that the inode says we're in
> -		 * extents or btree format but failed to pass the inode fork
> -		 * verifier while trying to load it.  Treat that as a file
> -		 * corruption too.
> -		 */
> -#ifdef DEBUG
> -		xfs_alert(mp, "%s: inode %llu missing fork %d",
> -				__func__, ip->i_ino, whichfork);
> -#endif /* DEBUG */
> -		return -EFSCORRUPTED;
> -	}
> -

Well that addresses my thought on the previous patch, but I don't see
the value in removing the check entirely. It might be safe for the inode
from disk path, but that doesn't preclude current or future runtime bugs
associated with xattr removal (i.e. fork removal) or inappropriate use
of XFS_BMAPI_ATTRFORK, for example. In fact, I think it makes sense for
any inappropriate use of xfs_bmapi_read() due to lack of the associated
fork to return an error rather than explode.

Brian

>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(NULL, ip, whichfork);
>  		if (error)
> -- 
> 2.26.2
>