Re: [PATCH] xfs: skip verification on initial "guess" superblock read

[Dave Chinner <david@fromorbit.com>]

On Thu, Feb 13, 2014 at 03:42:49PM -0600, Eric Sandeen wrote:
> When xfs_readsb() does the very first read of the superblock,
> it makes a guess at the length of the buffer, based on the
> sector size of the underlying storage.  This may or may
> not match the filesystem sector size in sb_sectsize, so
> we can't i.e. do a CRC check on it; it might be too short.
> 
> In fact, mounting a filesystem with sb_sectsize larger
> than the device sector size will cause a mount failure
> if CRCs are enabled, because we are checksumming a length
> which exceeds the buffer passed to it.
> 
> The only really foolproof way I see around this is to
> *always* read the superblock twice in xfs_readsb();
> first with a guess, and again with a length as indicated
> by the superblock's sb_sectsize.  The verifier for
> this guessed length buffer is a no-op; we'll do proper
> verification on the 2nd time around.

We don't need a verifier for a "don't verify" read. Just pass in
NULL to avoid verification completely.

Also, you don't need to re-read the buffer to verify it unless the
sector size was not what was predicted. All you need to do is call
the verifier directly on the buffer in question, then attach it
directly to the buffer. So no need for @xfs_sb_guess_buf_ops. :)

Cheers,

Dave.


> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 02df7b4..b4413fe 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -282,22 +282,28 @@ xfs_readsb(
>  	struct xfs_sb	*sbp = &mp->m_sb;
>  	int		error;
>  	int		loud = !(flags & XFS_MFSI_QUIET);
> +	const struct xfs_buf_ops *buf_ops;
>  
>  	ASSERT(mp->m_sb_bp == NULL);
>  	ASSERT(mp->m_ddev_targp != NULL);
>  
>  	/*
> +	 * For the initial read, we must guess at the sector
> +	 * size based on the block device.  It's enough to
> +	 * get the sb_sectsize out of the superblock and
> +	 * then reread with the proper length.
> +	 */
> +	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
> +	buf_ops = &xfs_sb_guess_buf_ops;
> +
> +	/*
>  	 * Allocate a (locked) buffer to hold the superblock.
>  	 * This will be kept around at all times to optimize
>  	 * access to the superblock.
>  	 */
> -	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
> -
>  reread:
>  	bp = xfs_buf_read_uncached(mp->m_ddev_targp, XFS_SB_DADDR,
> -				   BTOBB(sector_size), 0,
> -				   loud ? &xfs_sb_buf_ops
> -				        : &xfs_sb_quiet_buf_ops);
> +				   BTOBB(sector_size), 0, buf_ops);
>  	if (!bp) {
>  		if (loud)
>  			xfs_warn(mp, "SB buffer read failed");
> @@ -328,12 +334,13 @@ reread:
>  	}
>  
>  	/*
> -	 * If device sector size is smaller than the superblock size,
> -	 * re-read the superblock so the buffer is correctly sized.
> +	 * Re-read the superblock so the buffer is correctly sized,
> +	 * and properly verified.
>  	 */
> -	if (sector_size < sbp->sb_sectsize) {
> +	if (buf_ops == &xfs_sb_guess_buf_ops) {
>  		xfs_buf_relse(bp);
>  		sector_size = sbp->sb_sectsize;
> +		buf_ops = loud ? &xfs_sb_buf_ops : &xfs_sb_quiet_buf_ops;
>  		goto reread;
>  	}
>  
> diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
> index 5071ccb..abbbbb7 100644
> --- a/fs/xfs/xfs_sb.c
> +++ b/fs/xfs/xfs_sb.c
> @@ -653,6 +653,20 @@ xfs_sb_quiet_read_verify(
>  	/* quietly fail */
>  	xfs_buf_ioerror(bp, EWRONGFS);
>  }
> +/*
> + * The initial superblock read from xfs_readsb only guesses at
> + * the sector size based on the block device sector size, so
> + * we may get here with a buffer length shorter than the filesystem
> + * sb_sectsize.  We can't properly verify it, so just return,
> + * and xfs_readsb will call the proper verifer with a real length
> + * on the 2nd time around.
> + */
> +static void
> +xfs_sb_guess_read_verify(
> +	struct xfs_buf	*bp)
> +{
> +	return;
> +}
>  
>  static void
>  xfs_sb_write_verify(
> @@ -680,16 +694,24 @@ xfs_sb_write_verify(
>  			 offsetof(struct xfs_sb, sb_crc));
>  }
>  
> +/* Normal read verifier */
>  const struct xfs_buf_ops xfs_sb_buf_ops = {
>  	.verify_read = xfs_sb_read_verify,
>  	.verify_write = xfs_sb_write_verify,
>  };
>  
> +/* Quiet verifier for MS_SILENT mounts; ignore non-XFS magic */
>  const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
>  	.verify_read = xfs_sb_quiet_read_verify,
>  	.verify_write = xfs_sb_write_verify,
>  };
>  
> +/* The very first superblock read must guess at the size */
> +const struct xfs_buf_ops xfs_sb_guess_buf_ops = {
> +	.verify_read = xfs_sb_guess_read_verify,
> +	.verify_write = xfs_sb_write_verify,
> +};
> +
>  /*
>   * xfs_mount_common
>   *
> diff --git a/fs/xfs/xfs_shared.h b/fs/xfs/xfs_shared.h
> index 8c5035a..a4294a6 100644
> --- a/fs/xfs/xfs_shared.h
> +++ b/fs/xfs/xfs_shared.h
> @@ -51,6 +51,7 @@ extern const struct xfs_buf_ops xfs_inode_buf_ra_ops;
>  extern const struct xfs_buf_ops xfs_dquot_buf_ops;
>  extern const struct xfs_buf_ops xfs_sb_buf_ops;
>  extern const struct xfs_buf_ops xfs_sb_quiet_buf_ops;
> +extern const struct xfs_buf_ops xfs_sb_guess_buf_ops;
>  extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>  
>  /*
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs