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