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@xxxxxxxxxx>
>
> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
