Re: [PATCH] libxfs: add more bounds checking to sb sanity checks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Jul 13, 2018 at 08:10:03AM -0500, Bill O'Donnell wrote:
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 350119eeaecb..cdede769ab88 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -261,7 +261,9 @@ xfs_mount_validate_sb(
>  	    sbp->sb_dblocks == 0					||
>  	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
>  	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp)			||
> -	    sbp->sb_shared_vn != 0)) {
> +	    sbp->sb_shared_vn != 0					||
> +	    sbp->sb_fdblocks > sbp->sb_dblocks				||
> +	    sbp->sb_ifree > sbp->sb_icount)) {

Hmm.  On its face this seems reasonable for the superblock verifier, but
then I started wondering, since these are /summary/ counters.

If the free counts are off by this much, the admin won't be able to
mount the fs, and xfs_repair is the only other tool that can fix the
summary counts.  However, if the log is dirty, the mount won't succeed
to recover the fs, which is too bad since we can reinitialize the
summary counts after log recovery.  xfs_repair -L will be the only way
out, which will wreak havoc on the filesystem from discarding the log
contents.

So, would it be preferable to split this into two parts?  For example,
have this as a corruption check in _sb_write_verify to prevent us from
writing out garbage counters and a clamp in _reinit_percpu_counters so
that we never present ridiculous free counts to users?

(Does any of this make sense with !haslazysbcount filesystems?)

Bonus question: What about checking frextents/rextents?

--D

>  		xfs_notice(mp, "SB sanity check failed");
>  		return -EFSCORRUPTED;
>  	}
> -- 
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html