Re: [PATCH 6/8] xfs: journal geometry is not properly bounds checked

Dave Chinner
2023-06-28T09:02:20Z

On Tue, Jun 27, 2023 at 11:08:46PM -0700, Christoph Hellwig wrote:
> > + if (sbp->sb_logblocks > XFS_MAX_LOG_BLOCKS) {
> > + xfs_notice(mp,
> > + "Log size 0x%x blocks too large, maximum size is 0x%llx blocks",
> > + sbp->sb_logblocks, XFS_MAX_LOG_BLOCKS);
>
> Just a little nitpick, didn't we traditionally align the overly
> long format strings with a single tab and not to the same line as
> the xfs_notice/warn/etc?

I don't think we have a particluarly formal definition of that.

I mean, just look at all the different variants in
xfs_sb_validate_common:

if (xfs_sb_is_v5(sbp)) {
if (sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
xfs_notice(mp,
"Block size (%u bytes) too small for Version 5 superblock (minimum %d bytes)",
sbp->sb_blocksize, XFS_MIN_CRC_BLOCKSIZE);
return -EFSCORRUPTED;
}

/* V5 has a separate project quota inode */
if (sbp->sb_qflags & (XFS_OQUOTA_ENFD | XFS_OQUOTA_CHKD)) {
xfs_notice(mp,
"Version 5 of Super block has XFS_OQUOTA bits.");
return -EFSCORRUPTED;
}

/*
* Full inode chunks must be aligned to inode chunk size when
* sparse inodes are enabled to support the sparse chunk
* allocation algorithm and prevent overlapping inode records.
*/
if (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_SPINODES) {
uint32_t align;

align = XFS_INODES_PER_CHUNK * sbp->sb_inodesize
>> sbp->sb_blocklog;
if (sbp->sb_inoalignmt != align) {
xfs_warn(mp,
"Inode block alignment (%u) must match chunk size (%u) for sparse inodes.",
sbp->sb_inoalignmt, align);
return -EINVAL;
}
}
} else if (sbp->sb_qflags & (XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD |
XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD)) {
xfs_notice(mp,
"Superblock earlier than Version 5 has XFS_{P|G}QUOTA_{ENFD|CHKD} bits.");
return -EFSCORRUPTED;
}

if (unlikely(
sbp->sb_logstart == 0 && mp->m_logdev_targp == mp->m_ddev_targp)) {
xfs_warn(mp,
"filesystem is marked as having an external log; "
"specify logdev on the mount command line.");
return -EINVAL;
}

if (unlikely(
sbp->sb_logstart != 0 && mp->m_logdev_targp != mp->m_ddev_targp)) {
xfs_warn(mp,
"filesystem is marked as having an internal log; "
"do not specify logdev on the mount command line.");
return -EINVAL;
}

There's 4-5 different variations in indenting in 6-7 warning
messages. And there are more variations in that function, too.

There's no consistent rule to follow, so I just set the indenting
such that the format string didn't run over 80 columns and didn't
think any further..

I think that if we really care, a separate cleanup patch to make all
the long format strings use consistent zero-indenting and the
variables one tab like this one:

xfs_notice(mp,
"Block size (%u bytes) too small for Version 5 superblock (minimum %d bytes)",
sbp->sb_blocksize, XFS_MIN_CRC_BLOCKSIZE);

Could be done. If I'm ever lacking for something to do...

> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks!

_dave.
--
Dave Chinner
david@fromorbit.com