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

Brian Foster
2020-05-08T15:06:14Z

On Fri, May 08, 2020 at 08:34:23AM +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. So remove the work around to try to handle the
> case, and just return an error and warn if the fork is not present.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

> fs/xfs/libxfs/xfs_bmap.c | 22 +++++-----------------
> 1 file changed, 5 insertions(+), 17 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 76be1a18e2442..34518a6dc7376 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,12 +3900,14 @@ 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)));
> ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
>
> + if (WARN_ON_ONCE(!ifp))
> + return -EFSCORRUPTED;
> +
> if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
> XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
> return -EFSCORRUPTED;
> @@ -3915,21 +3918,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;
> - }
> -
> if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> error = xfs_iread_extents(NULL, ip, whichfork);
> if (error)
> --
> 2.26.2
>