xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: don't leak EFSBADCRC to userspace

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: don't leak EFSBADCRC to userspace
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 5 Mar 2014 10:48:56 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1393825194-1719-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1393825194-1719-1-git-send-email-david@xxxxxxxxxxxxx> <1393825194-1719-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Mar 03, 2014 at 04:39:53PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> While the verifier reoutines may return EFSBADCRC when a buffer ahs
> a bad CRC, we need to translate that to EFSCORRUPTED so that the
> higher layers treat the error appropriately and so we return a
> consistent error to userspace. This fixes a xfs/005 regression.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

Looking through again, I don't see any obvious cases where EFSBADCRC
could slip through. The verifier users mostly call
xfs_trans_read_buf_map(). The readahead case looks like it should be
handled by the xfs_buf_iowait() checks therein.

The one case I came across that looked suspicious is the readahead/read
cycle for inode log recovery in xlog_recover_commit_trans(). This calls
into xfs_buf_read() and ultimately uses bp->b_error directly in
xlog_recover_inode_pass2(). That said, I don't see any crc checking in
these verifiers and thus they don't source EFSBADCRC. Looks good to
me...

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

>  fs/xfs/xfs_mount.c     |  3 +++
>  fs/xfs/xfs_symlink.c   |  4 ++++
>  fs/xfs/xfs_trans_buf.c | 11 +++++++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index f96c056..993cb19 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -314,6 +314,9 @@ reread:
>               error = bp->b_error;
>               if (loud)
>                       xfs_warn(mp, "SB validate failed with error %d.", 
> error);
> +             /* bad CRC means corrupted metadata */
> +             if (error == EFSBADCRC)
> +                     error = EFSCORRUPTED;
>               goto release_buf;
>       }
>  
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 14e58f2..5fda189 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -80,6 +80,10 @@ xfs_readlink_bmap(
>               if (error) {
>                       xfs_buf_ioerror_alert(bp, __func__);
>                       xfs_buf_relse(bp);
> +
> +                     /* bad CRC means corrupted metadata */
> +                     if (error == EFSBADCRC)
> +                             error = EFSCORRUPTED;
>                       goto out;
>               }
>               byte_cnt = XFS_SYMLINK_BUF_SPACE(mp, byte_cnt);
> diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
> index 647b6f1..b8eef05 100644
> --- a/fs/xfs/xfs_trans_buf.c
> +++ b/fs/xfs/xfs_trans_buf.c
> @@ -275,6 +275,10 @@ xfs_trans_read_buf_map(
>                       XFS_BUF_UNDONE(bp);
>                       xfs_buf_stale(bp);
>                       xfs_buf_relse(bp);
> +
> +                     /* bad CRC means corrupted metadata */
> +                     if (error == EFSBADCRC)
> +                             error = EFSCORRUPTED;
>                       return error;
>               }
>  #ifdef DEBUG
> @@ -338,6 +342,9 @@ xfs_trans_read_buf_map(
>                               if (tp->t_flags & XFS_TRANS_DIRTY)
>                                       xfs_force_shutdown(tp->t_mountp,
>                                                       SHUTDOWN_META_IO_ERROR);
> +                             /* bad CRC means corrupted metadata */
> +                             if (error == EFSBADCRC)
> +                                     error = EFSCORRUPTED;
>                               return error;
>                       }
>               }
> @@ -375,6 +382,10 @@ xfs_trans_read_buf_map(
>               if (tp->t_flags & XFS_TRANS_DIRTY)
>                       xfs_force_shutdown(tp->t_mountp, 
> SHUTDOWN_META_IO_ERROR);
>               xfs_buf_relse(bp);
> +
> +             /* bad CRC means corrupted metadata */
> +             if (error == EFSBADCRC)
> +                     error = EFSCORRUPTED;
>               return error;
>       }
>  #ifdef DEBUG
> -- 
> 1.9.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>