Note, this one might need to be in 3.14 fixes as well, if
[PATCH] xfs: xfs_sb_read_verify() doesn't flag bad crcs on primary sb
is queued up; with that patch in place, we'll fail mounting
i.e. a 4K sector size FS on a 512 sector disk, because we'll
try to do a 4K metadata checksum on a 512 byte buffer... :/
I had thought about just skipping the CRC if BBTOB(bp->b_length) !=
sb->sb_sectsize, but it seemed like other superblock read paths
would wind up skipping CRCs based what would be corruption.
By doing this (ugly?) no-op verifier, at least this first
(short?) sb read is under complete control, with no guessing required
in the normal verifier.
-Eric
On 2/13/14, 3:42 PM, 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.
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
