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

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

[Eric Sandeen]

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@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

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

[Eric Sandeen]

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@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
> 

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

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

[Dave Chinner]

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

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

[Eric Sandeen]

On 2/13/14, 4:08 PM, Dave Chinner wrote:
> 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.

Oh!

        if (read && bp->b_ops && !bp->b_error && (bp->b_flags & XBF_DONE))
                bp->b_ops->verify_read(bp);

Doh, ok, yeah that is simpler.  Didn't think about that.  :/

> 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. :)

Hum, ok, let me ponder that.

-Eric

> 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
>>
> 

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

[PATCH V2] xfs: skip verification if initial sb read is wrong length

[Eric Sandeen]

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; if
it's too short, we can't do a proper validity check on it.

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.

Work around this by setting the verifier ops to NULL
on the first read; if the guess was right, hook up the
normal ops and verify it directly, otherwise reread
with the newly discovered proper length.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 02df7b4..ff5ed4a 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -282,22 +282,29 @@ 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 fs 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 if necessary.
+	 * We don't verify it yet, because it may not be complete.
+	 */
+	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
+	buf_ops = NULL;
+
+	/*
 	 * 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");
@@ -334,7 +341,12 @@ reread:
 	if (sector_size < sbp->sb_sectsize) {
 		xfs_buf_relse(bp);
 		sector_size = sbp->sb_sectsize;
+		buf_ops = loud ? &xfs_sb_buf_ops : &xfs_sb_quiet_buf_ops;
 		goto reread;
+	} else {
+		/* We guessed right!  Verify it. */
+		bp->b_ops = &xfs_sb_buf_ops;
+		bp->b_ops->verify_read(bp);
 	}
 
 	/* Initialize per-cpu counters */
diff --git a/fs/xfs/xfs_sb.c b/fs/xfs/xfs_sb.c
index 5071ccb..1b0b503 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -644,7 +644,6 @@ xfs_sb_quiet_read_verify(
 {
 	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
 
-
 	if (dsb->sb_magicnum == cpu_to_be32(XFS_SB_MAGIC)) {
 		/* XFS filesystem, verify noisily! */
 		xfs_sb_read_verify(bp);

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

Re: [PATCH V2] xfs: skip verification if initial sb read is wrong length

[Dave Chinner]

On Thu, Feb 13, 2014 at 04:45:36PM -0600, Eric Sandeen wrote:
> @@ -334,7 +341,12 @@ reread:
>  	if (sector_size < sbp->sb_sectsize) {
>  		xfs_buf_relse(bp);
>  		sector_size = sbp->sb_sectsize;
> +		buf_ops = loud ? &xfs_sb_buf_ops : &xfs_sb_quiet_buf_ops;
>  		goto reread;
> +	} else {
> +		/* We guessed right!  Verify it. */
> +		bp->b_ops = &xfs_sb_buf_ops;
> +		bp->b_ops->verify_read(bp);
>  	}

One needs to check bp->b_error after verification ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Eric Sandeen]

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.

So always read twice; the first time we read with NULL
buffer ops to skip verification; then set the proper
read length, hook up the proper verifier, and give it
another go.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V3: Go back to always reading twice, to avoid the need for
duplicating the code that handles verifier errors.

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 02df7b4..f96c056 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -282,22 +282,29 @@ 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 we can reread with the proper length.
+	 * We don't verify it yet, because it may not be complete.
+	 */
+	sector_size = xfs_getsize_buftarg(mp->m_ddev_targp);
+	buf_ops = NULL;
+
+	/*
 	 * 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 +335,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 == NULL) {
 		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..1b0b503 100644
--- a/fs/xfs/xfs_sb.c
+++ b/fs/xfs/xfs_sb.c
@@ -644,7 +644,6 @@ xfs_sb_quiet_read_verify(
 {
 	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
 
-
 	if (dsb->sb_magicnum == cpu_to_be32(XFS_SB_MAGIC)) {
 		/* XFS filesystem, verify noisily! */
 		xfs_sb_read_verify(bp);


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

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

[Dave Chinner]

On Thu, Feb 13, 2014 at 05:32:12PM -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.
> 
> So always read twice; the first time we read with NULL
> buffer ops to skip verification; then set the proper
> read length, hook up the proper verifier, and give it
> another go.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks fine. I'll run it through some testing and we can go from
there. FWIW, does this make your "check the sector size in the sb
verifier" patch redundant?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Eric Sandeen]

On 2/13/14, 6:27 PM, Dave Chinner wrote:

> Looks fine. I'll run it through some testing and we can go from
> there. FWIW, does this make your "check the sector size in the sb
> verifier" patch redundant?

Well, it'll be safe for the primary SB, but I assume that a corrupted
secondary would still trip things up.

I think that at this point we can switch to using BBTOB(bp->b_length)
in the verifier and be safe.  I'll look into it.

-Eric
 
> Cheers,
> 
> Dave.
> 

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