| Message ID | 59cde702-21d5-3465-12b0-29dad978186d@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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
On Wed, Apr 04, 2018 at 02:00:22PM -0500, Eric Sandeen wrote: > Today the xfs_dqblk UUID is only checked in > xfs_dquot_buf_verify_crc, which is a bit odd in itself; normally > a CRC is checked on its own, separate from other metadata. > > And by not checking the UUID in xfs_dquot_verify, this means > that future read/write verifiers will continue to choke on > mismatched UUID errors which are never seen or repaired when > quotacheck calls xfs_dquot_verify to validate a disk dquot. > > Move the uuid check from xfs_dquot_buf_verify_crc to > xfs_dquot_verify so that this piece of metadata is more consistently > checked. If we have a type sent in as well, validate that too - > it was unused until now. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/libxfs/xfs_dquot_buf.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c > index f94e8c2..9f8b2c5 100644 > --- a/fs/xfs/libxfs/xfs_dquot_buf.c > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c > @@ -66,11 +66,20 @@ > * This is all fine; things are still consistent, and we haven't lost > * any quota information. Just don't complain about bad dquot blks. > */ > + if (xfs_sb_version_hascrc(&mp->m_sb)) { > + struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddq; > + > + if (!uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid)) > + return __this_address; > + } > + > if (ddq->d_magic != cpu_to_be16(XFS_DQUOT_MAGIC)) > return __this_address; > if (ddq->d_version != XFS_DQUOT_VERSION) > return __this_address; > > + if (type && ddq->d_flags != type) > + return __this_address; > if (ddq->d_flags != XFS_DQ_USER && > ddq->d_flags != XFS_DQ_PROJ && > ddq->d_flags != XFS_DQ_GROUP) > @@ -156,8 +165,6 @@ > if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk), > XFS_DQUOT_CRC_OFF)) > return false; > - if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid)) > - return false; > } > return true; > } > -- > 1.8.3.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 -- 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
On Tue, May 01, 2018 at 09:13:37AM -0700, Darrick J. Wong wrote: > On Wed, Apr 04, 2018 at 02:00:22PM -0500, Eric Sandeen wrote: > > Today the xfs_dqblk UUID is only checked in > > xfs_dquot_buf_verify_crc, which is a bit odd in itself; normally > > a CRC is checked on its own, separate from other metadata. > > > > And by not checking the UUID in xfs_dquot_verify, this means > > that future read/write verifiers will continue to choke on > > mismatched UUID errors which are never seen or repaired when > > quotacheck calls xfs_dquot_verify to validate a disk dquot. > > > > Move the uuid check from xfs_dquot_buf_verify_crc to > > xfs_dquot_verify so that this piece of metadata is more consistently > > checked. If we have a type sent in as well, validate that too - > > it was unused until now. > > > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > > Looks ok, > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> I take this back... > --D > > > --- > > fs/xfs/libxfs/xfs_dquot_buf.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c > > index f94e8c2..9f8b2c5 100644 > > --- a/fs/xfs/libxfs/xfs_dquot_buf.c > > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c > > @@ -66,11 +66,20 @@ > > * This is all fine; things are still consistent, and we haven't lost > > * any quota information. Just don't complain about bad dquot blks. > > */ > > + if (xfs_sb_version_hascrc(&mp->m_sb)) { > > + struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddq; > > + > > + if (!uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid)) > > + return __this_address; > > + } generic/055 regresses when quotas are enabled and log recovery has to process a dquot: xfs_dquot_verify+0x48/0x120 [xfs] xlog_recover_dquot_pass2.isra.40+0x98/0x340 [xfs] xlog_recover_items_pass2+0x3c/0x60 [xfs] xlog_recover_commit_trans+0x178/0x2a0 [xfs] xlog_recovery_process_trans+0x84/0xd0 [xfs] xlog_recover_process_data+0x93/0x230 [xfs] xlog_do_recovery_pass+0x453/0x6d0 [xfs] xlog_do_log_recovery+0x8c/0x140 [xfs] xlog_do_recover+0x43/0x2b0 [xfs] xlog_recover+0xac/0x140 [xfs] xfs_log_mount+0x1bb/0x2f0 [xfs] xfs_mountfs+0x520/0xa60 [xfs] ? xfs_filestream_get_parent+0x70/0x70 [xfs] ? xfs_mru_cache_create+0x18b/0x1f0 [xfs] xfs_fs_fill_super+0x4ce/0x650 [xfs] ? xfs_test_remount_options+0x60/0x60 [xfs] mount_bdev+0x17b/0x1b0 mount_fs+0x15/0x80 vfs_kern_mount.part.34+0x54/0x150 do_mount+0x21f/0xd30 ? rcu_read_lock_sched_held+0x6b/0x80 ksys_mount+0x80/0xd0 __x64_sys_mount+0x21/0x30 do_syscall_64+0x56/0x180 entry_SYSCALL_64_after_hwframe+0x49/0xbe ...because we only log struct xfs_disk_dquot, not struct xfs_dqblk. The logged buffer isn't long enough to contain dd_uuid so we trip over the uuid check in xfs_dquot_verify and log recovery fails. XFS (pmem1): corrupt dquot ID 0x0 in log at xfs_dquot_verify+0x43/0x120 [xfs] 00000000411604c1: 44 51 01 01 00 00 00 00 00 00 00 00 00 00 00 00 DQ.............. 00000000f11219a7: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00000000a8451ea9: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 0000000060077c63: 00 00 00 00 00 00 00 f5 00 00 00 00 00 00 00 00 ................ 00000000efc4e1e2: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 000000000b057484: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 000000005fffbba2: 00 00 00 00 00 00 00 00 00 01 00 00 00 00 ad de ................ 00000000960b6207: 00 02 00 00 00 00 ad de 00 00 00 00 00 00 00 00 ................ 00000000c16a5600: 41 42 33 43 00 00 00 01 AB3C.... (Yikes, we've overrun into a cntbt block header!) --D > > + > > if (ddq->d_magic != cpu_to_be16(XFS_DQUOT_MAGIC)) > > return __this_address; > > if (ddq->d_version != XFS_DQUOT_VERSION) > > return __this_address; > > > > + if (type && ddq->d_flags != type) > > + return __this_address; > > if (ddq->d_flags != XFS_DQ_USER && > > ddq->d_flags != XFS_DQ_PROJ && > > ddq->d_flags != XFS_DQ_GROUP) > > @@ -156,8 +165,6 @@ > > if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk), > > XFS_DQUOT_CRC_OFF)) > > return false; > > - if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid)) > > - return false; > > } > > return true; > > } > > -- > > 1.8.3.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 > -- > 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 -- 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
diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c index f94e8c2..9f8b2c5 100644 --- a/fs/xfs/libxfs/xfs_dquot_buf.c +++ b/fs/xfs/libxfs/xfs_dquot_buf.c @@ -66,11 +66,20 @@ * This is all fine; things are still consistent, and we haven't lost * any quota information. Just don't complain about bad dquot blks. */ + if (xfs_sb_version_hascrc(&mp->m_sb)) { + struct xfs_dqblk *dqb = (struct xfs_dqblk *)ddq; + + if (!uuid_equal(&dqb->dd_uuid, &mp->m_sb.sb_meta_uuid)) + return __this_address; + } + if (ddq->d_magic != cpu_to_be16(XFS_DQUOT_MAGIC)) return __this_address; if (ddq->d_version != XFS_DQUOT_VERSION) return __this_address; + if (type && ddq->d_flags != type) + return __this_address; if (ddq->d_flags != XFS_DQ_USER && ddq->d_flags != XFS_DQ_PROJ && ddq->d_flags != XFS_DQ_GROUP) @@ -156,8 +165,6 @@ if (!xfs_verify_cksum((char *)d, sizeof(struct xfs_dqblk), XFS_DQUOT_CRC_OFF)) return false; - if (!uuid_equal(&d->dd_uuid, &mp->m_sb.sb_meta_uuid)) - return false; } return true; }
Today the xfs_dqblk UUID is only checked in xfs_dquot_buf_verify_crc, which is a bit odd in itself; normally a CRC is checked on its own, separate from other metadata. And by not checking the UUID in xfs_dquot_verify, this means that future read/write verifiers will continue to choke on mismatched UUID errors which are never seen or repaired when quotacheck calls xfs_dquot_verify to validate a disk dquot. Move the uuid check from xfs_dquot_buf_verify_crc to xfs_dquot_verify so that this piece of metadata is more consistently checked. If we have a type sent in as well, validate that too - it was unused until now. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/xfs/libxfs/xfs_dquot_buf.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)