Re: [PATCH v3] libxfs: add more bounds checking to sb sanity checks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Jul 25, 2018 at 03:48:51PM -0700, Eric Sandeen wrote:
> 
> 
> On 7/25/18 2:33 PM, Bill O'Donnell wrote:
> > +	/* Additional sb sanity checks for writes */
> > +	if (!error) {
> > +		if (sb.sb_fdblocks > sb.sb_dblocks ||
> > +		    sb.sb_ifree > sb.sb_icount) {
> > +			    xfs_notice(mp, "SB sanity check failed");
> > +			    error = -EFSCORRUPTED;
> > +		}
> > +	}
> 
> I had kind of had it in my head that Dave suggested testing not
> only sb_fdblocks & sb_ifree but also validating sb_icount against
> sb_dblocks ... would that make sense?  something like:
> 
> +		if (sb.sb_fdblocks > sb.sb_dblocks ||
> +		    sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks) ||

That would make sense, but perhaps we should have a xfs_verify_icount
instead of open-coding a 64-bit division? :)

Granted, I /was/ planning to add all that as part of fs summary counter
scrubbing next cycle.

--D

> +		    sb.sb_ifree > sb.sb_icount) {
> +			    xfs_notice(mp, "SB sanity check failed");
> 
> because all 3 go into the statfs calculations which went wonky
> in the original report?  (xfs_sb_verify has done some sanity
> checks on sb_inopblock by the time we get here.)
> 
> Also, a comment about why these checks are only for write, and are not
> simply in xfs_mount_validate_sb() would be good, since that obviously
> wasn't obvious to me at first.  o_O :)
> 
> -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