| Message ID | 152401961080.13319.3976228325503296406.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 4/17/18 9:46 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Validate the log space information in a similar manner to the kernel so > that repair will stumble over (and fix) broken log info that prevents > mounting. Fixes logsunit fuzz-and-fix failures in xfs/350. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > repair/sb.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > > diff --git a/repair/sb.c b/repair/sb.c > index 3dc6538..f2968cd 100644 > --- a/repair/sb.c > +++ b/repair/sb.c > @@ -299,6 +299,27 @@ sb_validate_ino_align(struct xfs_sb *sb) > } > > /* > + * Validate the given log space. Derived from xfs_log_mount, though we > + * can't validate the minimum log size until later. > + * Returns false if the log is garbage. > + */ > +static bool > +verify_sb_loginfo( > + struct xfs_sb *sb) > +{ > + if (xfs_sb_version_hascrc(sb) && Hm, this could use a comment about why these things only matter on v5 supers. > + (sb->sb_logblocks == 0 || > + sb->sb_logblocks > XFS_MAX_LOG_BLOCKS || > + (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES)) > + return false; > + > + if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize) > + return false; > + > + return true; > +} > + > +/* > * verify a superblock -- does not verify root inode # > * can only check that geometry info is internally > * consistent. because of growfs, that's no guarantee > @@ -409,7 +430,8 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) > sb->sb_inodesize != (1 << sb->sb_inodelog) || > sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE || > sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) || > - (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog)) > + (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) || > + !verify_sb_loginfo(sb)) > return XR_BAD_INO_SIZE_DATA; do you really want to return XR_BAD_INO_SIZE_DATA for a bad logblocks count, which will yield "bad inode size or inconsistent with number of inodes/block") This might even want a new XR_BAD_LOG_FOO_BAR error...? -Eric > > if (xfs_sb_version_hassector(sb)) { > > -- > 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 Fri, May 04, 2018 at 02:29:55PM -0500, Eric Sandeen wrote: > On 4/17/18 9:46 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Validate the log space information in a similar manner to the kernel so > > that repair will stumble over (and fix) broken log info that prevents > > mounting. Fixes logsunit fuzz-and-fix failures in xfs/350. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > repair/sb.c | 24 +++++++++++++++++++++++- > > 1 file changed, 23 insertions(+), 1 deletion(-) > > > > > > diff --git a/repair/sb.c b/repair/sb.c > > index 3dc6538..f2968cd 100644 > > --- a/repair/sb.c > > +++ b/repair/sb.c > > @@ -299,6 +299,27 @@ sb_validate_ino_align(struct xfs_sb *sb) > > } > > > > /* > > + * Validate the given log space. Derived from xfs_log_mount, though we > > + * can't validate the minimum log size until later. > > + * Returns false if the log is garbage. > > + */ > > +static bool > > +verify_sb_loginfo( > > + struct xfs_sb *sb) > > +{ > > + if (xfs_sb_version_hascrc(sb) && > > Hm, this could use a comment about why these things only matter on v5 supers. /* * We only do this validation on v5 filesystems because the kernel * didn't reject too-small logs in the past. */ ? > > > + (sb->sb_logblocks == 0 || > > + sb->sb_logblocks > XFS_MAX_LOG_BLOCKS || > > + (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES)) > > + return false; > > + > > + if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize) > > + return false; > > + > > + return true; > > +} > > + > > +/* > > * verify a superblock -- does not verify root inode # > > * can only check that geometry info is internally > > * consistent. because of growfs, that's no guarantee > > @@ -409,7 +430,8 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) > > sb->sb_inodesize != (1 << sb->sb_inodelog) || > > sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE || > > sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) || > > - (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog)) > > + (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) || > > + !verify_sb_loginfo(sb)) > > return XR_BAD_INO_SIZE_DATA; > > do you really want to return XR_BAD_INO_SIZE_DATA for a bad logblocks count, > which will yield > > "bad inode size or inconsistent with number of inodes/block") > > This might even want a new XR_BAD_LOG_FOO_BAR error...? Sure. "bad log geometry information in superblock" ? --D > > -Eric > > > > > if (xfs_sb_version_hassector(sb)) { > > > > -- > > 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
On 5/4/18 3:25 PM, Darrick J. Wong wrote: > On Fri, May 04, 2018 at 02:29:55PM -0500, Eric Sandeen wrote: >> On 4/17/18 9:46 PM, Darrick J. Wong wrote: >>> From: Darrick J. Wong <darrick.wong@oracle.com> >>> >>> Validate the log space information in a similar manner to the kernel so >>> that repair will stumble over (and fix) broken log info that prevents >>> mounting. Fixes logsunit fuzz-and-fix failures in xfs/350. ... >> Hm, this could use a comment about why these things only matter on v5 supers. > > /* > * We only do this validation on v5 filesystems because the kernel > * didn't reject too-small logs in the past. > */ > > ? "... doesn't reject malformed logs on older revision filesystems" also, it'll probably happen anyway, but the new check-stuff function should probably be called on its own and not tacked on via || to the inode checks. -Eric -- 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/repair/sb.c b/repair/sb.c index 3dc6538..f2968cd 100644 --- a/repair/sb.c +++ b/repair/sb.c @@ -299,6 +299,27 @@ sb_validate_ino_align(struct xfs_sb *sb) } /* + * Validate the given log space. Derived from xfs_log_mount, though we + * can't validate the minimum log size until later. + * Returns false if the log is garbage. + */ +static bool +verify_sb_loginfo( + struct xfs_sb *sb) +{ + if (xfs_sb_version_hascrc(sb) && + (sb->sb_logblocks == 0 || + sb->sb_logblocks > XFS_MAX_LOG_BLOCKS || + (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES)) + return false; + + if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize) + return false; + + return true; +} + +/* * verify a superblock -- does not verify root inode # * can only check that geometry info is internally * consistent. because of growfs, that's no guarantee @@ -409,7 +430,8 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb) sb->sb_inodesize != (1 << sb->sb_inodelog) || sb->sb_logsunit > XLOG_MAX_RECORD_BSIZE || sb->sb_inopblock != howmany(sb->sb_blocksize, sb->sb_inodesize) || - (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog)) + (sb->sb_blocklog - sb->sb_inodelog != sb->sb_inopblog) || + !verify_sb_loginfo(sb)) return XR_BAD_INO_SIZE_DATA; if (xfs_sb_version_hassector(sb)) {