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

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 19, 2023 at 01:59:57PM +0530, Ritesh Harjani wrote:
> Dave Chinner <david@fromorbit.com> writes:
> > +	if (sbp->sb_logsunit > 1) {
> > +		if (sbp->sb_logsunit % sbp->sb_blocksize) {
> > +			xfs_notice(mp,
> > +		"log stripe unit %u bytes must be a multiple of block size",
> > +				sbp->sb_logsunit);
> > +			return -EFSCORRUPTED;
> > +		}
> > +		if (sbp->sb_logsunit > XLOG_MAX_RECORD_BSIZE) {
> > +			xfs_notice(mp,
> > +		"log stripe unit %u bytes must be a multiple of block size",
> > +				sbp->sb_logsunit);
> 
> I guess this xfs_notice message needs to be corrected.
> 
> > +			return -EFSCORRUPTED;
> > +		}
> > +	}
> > +
> > +
> > +
> > +
> 
> too many new lines here ^^^

Fixed.

> 
> > -	 * We can, however, reject mounts for CRC format filesystems, as the
> > +	 * We can, however, reject mounts for V5 format filesystems, as the
> >  	 * mkfs binary being used to make the filesystem should never create a
> >  	 * filesystem with a log that is too small.
> >  	 */
> >  	min_logfsbs = xfs_log_calc_minimum_size(mp);
> > -
> >  	if (mp->m_sb.sb_logblocks < min_logfsbs) {
> >  		xfs_warn(mp,
> >  		"Log size %d blocks too small, minimum size is %d blocks",
> >  			 mp->m_sb.sb_logblocks, min_logfsbs);
> >  		error = -EINVAL;
> 
> Are we using this error now?

I'm not changing the existing error for this case.

> 
> > -	} else if (mp->m_sb.sb_logblocks > XFS_MAX_LOG_BLOCKS) {
> > -		xfs_warn(mp,
> > -		"Log size %d blocks too large, maximum size is %lld blocks",
> > -			 mp->m_sb.sb_logblocks, XFS_MAX_LOG_BLOCKS);
> > -		error = -EINVAL;
> > -	} else if (XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks) > XFS_MAX_LOG_BYTES) {
> > -		xfs_warn(mp,
> > -		"log size %lld bytes too large, maximum size is %lld bytes",
> > -			 XFS_FSB_TO_B(mp, mp->m_sb.sb_logblocks),
> > -			 XFS_MAX_LOG_BYTES);
> > -		error = -EINVAL;
> > -	} else if (mp->m_sb.sb_logsunit > 1 &&
> > -		   mp->m_sb.sb_logsunit % mp->m_sb.sb_blocksize) {
> > -		xfs_warn(mp,
> > -		"log stripe unit %u bytes must be a multiple of block size",
> > -			 mp->m_sb.sb_logsunit);
> > -		error = -EINVAL;
> > -		fatal = true;
> > -	}
> > -	if (error) {
> > +
> >  		/*
> >  		 * Log check errors are always fatal on v5; or whenever bad
> >  		 * metadata leads to a crash.
> >  		 */
> > -		if (fatal) {
> > +		if (xfs_has_crc(mp)) {
> >  			xfs_crit(mp, "AAIEEE! Log failed size checks. Abort!");
> >  			ASSERT(0);
> >  			goto out_free_log;
> 
> yes, only here in goto out_free_log we will return "error".
> Then why not shift error = -EINVAL in this if block?

Because I tried to change as little as needed to fix the issue. I
didn't think it was necessary to move it from where it was....

Fixed.

-Dave.
-- 
Dave Chinner
david@fromorbit.com