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

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

[Bill O'Donnell]

Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
Add sanity checks for these parameters.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/libxfs/xfs_sb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 350119eeaecb..cdede769ab88 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -261,7 +261,9 @@ xfs_mount_validate_sb(
 	    sbp->sb_dblocks == 0					||
 	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
 	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp)			||
-	    sbp->sb_shared_vn != 0)) {
+	    sbp->sb_shared_vn != 0					||
+	    sbp->sb_fdblocks > sbp->sb_dblocks				||
+	    sbp->sb_ifree > sbp->sb_icount)) {
 		xfs_notice(mp, "SB sanity check failed");
 		return -EFSCORRUPTED;
 	}
-- 
2.17.1

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

[Darrick J. Wong]

On Fri, Jul 13, 2018 at 08:10:03AM -0500, Bill O'Donnell wrote:
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 350119eeaecb..cdede769ab88 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -261,7 +261,9 @@ xfs_mount_validate_sb(
>  	    sbp->sb_dblocks == 0					||
>  	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
>  	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp)			||
> -	    sbp->sb_shared_vn != 0)) {
> +	    sbp->sb_shared_vn != 0					||
> +	    sbp->sb_fdblocks > sbp->sb_dblocks				||
> +	    sbp->sb_ifree > sbp->sb_icount)) {

Hmm.  On its face this seems reasonable for the superblock verifier, but
then I started wondering, since these are /summary/ counters.

If the free counts are off by this much, the admin won't be able to
mount the fs, and xfs_repair is the only other tool that can fix the
summary counts.  However, if the log is dirty, the mount won't succeed
to recover the fs, which is too bad since we can reinitialize the
summary counts after log recovery.  xfs_repair -L will be the only way
out, which will wreak havoc on the filesystem from discarding the log
contents.

So, would it be preferable to split this into two parts?  For example,
have this as a corruption check in _sb_write_verify to prevent us from
writing out garbage counters and a clamp in _reinit_percpu_counters so
that we never present ridiculous free counts to users?

(Does any of this make sense with !haslazysbcount filesystems?)

Bonus question: What about checking frextents/rextents?

--D

>  		xfs_notice(mp, "SB sanity check failed");
>  		return -EFSCORRUPTED;
>  	}
> -- 
> 2.17.1
> 
> --
> 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

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

[Bill O'Donnell]

On Fri, Jul 13, 2018 at 09:41:53AM -0700, Darrick J. Wong wrote:
> On Fri, Jul 13, 2018 at 08:10:03AM -0500, Bill O'Donnell wrote:
> > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > Add sanity checks for these parameters.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 350119eeaecb..cdede769ab88 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -261,7 +261,9 @@ xfs_mount_validate_sb(
> >  	    sbp->sb_dblocks == 0					||
> >  	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
> >  	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp)			||
> > -	    sbp->sb_shared_vn != 0)) {
> > +	    sbp->sb_shared_vn != 0					||
> > +	    sbp->sb_fdblocks > sbp->sb_dblocks				||
> > +	    sbp->sb_ifree > sbp->sb_icount)) {
> 
> Hmm.  On its face this seems reasonable for the superblock verifier, but
> then I started wondering, since these are /summary/ counters.

FWIW, I'm proposing a rudimentary bounds check to prevent this sort of
issue from even happening in the first place:
https://www.spinics.net/lists/linux-xfs/msg20592.html

> 
> If the free counts are off by this much, the admin won't be able to
> mount the fs, and xfs_repair is the only other tool that can fix the
> summary counts.  However, if the log is dirty, the mount won't succeed
> to recover the fs, which is too bad since we can reinitialize the
> summary counts after log recovery.  xfs_repair -L will be the only way
> out, which will wreak havoc on the filesystem from discarding the log
> contents.

agreed, but again, I want to prevent the aforementioned use case where
corruption gets introduced.

> 
> So, would it be preferable to split this into two parts?  For example,
> have this as a corruption check in _sb_write_verify to prevent us from
> writing out garbage counters and a clamp in _reinit_percpu_counters so
> that we never present ridiculous free counts to users?
> 
> (Does any of this make sense with !haslazysbcount filesystems?)
> 
> Bonus question: What about checking frextents/rextents?

Hrmm, perhaps. It should definitely be considered.

Thanks-
Bill

> --D
> 
> >  		xfs_notice(mp, "SB sanity check failed");
> >  		return -EFSCORRUPTED;
> >  	}
> > -- 
> > 2.17.1
> > 
> > --
> > 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

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

[Dave Chinner]

On Fri, Jul 13, 2018 at 09:41:53AM -0700, Darrick J. Wong wrote:
> On Fri, Jul 13, 2018 at 08:10:03AM -0500, Bill O'Donnell wrote:
> > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > Add sanity checks for these parameters.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 350119eeaecb..cdede769ab88 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -261,7 +261,9 @@ xfs_mount_validate_sb(
> >  	    sbp->sb_dblocks == 0					||
> >  	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
> >  	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp)			||
> > -	    sbp->sb_shared_vn != 0)) {
> > +	    sbp->sb_shared_vn != 0					||
> > +	    sbp->sb_fdblocks > sbp->sb_dblocks				||
> > +	    sbp->sb_ifree > sbp->sb_icount)) {
> 
> Hmm.  On its face this seems reasonable for the superblock verifier, but
> then I started wondering, since these are /summary/ counters.
> 
> If the free counts are off by this much, the admin won't be able to
> mount the fs, and xfs_repair is the only other tool that can fix the
> summary counts.  However, if the log is dirty, the mount won't succeed
> to recover the fs, which is too bad since we can reinitialize the
> summary counts after log recovery.  xfs_repair -L will be the only way
> out, which will wreak havoc on the filesystem from discarding the log
> contents.

Yup, that's why I said "catch this on /write/", not "always reject
bad counter values".

i.e. we should never be writing a bad value, but we most definitely
need to be able to mount the filesystem to reconstruct them.

> So, would it be preferable to split this into two parts?  For example,
> have this as a corruption check in _sb_write_verify to prevent us from
> writing out garbage counters

yes.

> and a clamp in _reinit_percpu_counters so
> that we never present ridiculous free counts to users?

percpu_counter_{read,sum}_positive() should be used for anything that is
userspace facing. xfs_fs_counts() gets this right, but
xfs_fs_statfs() doesn't - it should use
percpu_counter_sum_positive().

> (Does any of this make sense with !haslazysbcount filesystems?)

Same thing - we can't verify the counters on read until after log
recovery as all the changes are journalled.

> Bonus question: What about checking frextents/rextents?

Same as !lazycount - all changes are journalled.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Sat, Jul 14, 2018 at 09:43:41AM +1000, Dave Chinner wrote:
> On Fri, Jul 13, 2018 at 09:41:53AM -0700, Darrick J. Wong wrote:
> > On Fri, Jul 13, 2018 at 08:10:03AM -0500, Bill O'Donnell wrote:
> > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > > Add sanity checks for these parameters.
> > > 
> > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_sb.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index 350119eeaecb..cdede769ab88 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -261,7 +261,9 @@ xfs_mount_validate_sb(
> > >  	    sbp->sb_dblocks == 0					||
> > >  	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
> > >  	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp)			||
> > > -	    sbp->sb_shared_vn != 0)) {
> > > +	    sbp->sb_shared_vn != 0					||
> > > +	    sbp->sb_fdblocks > sbp->sb_dblocks				||
> > > +	    sbp->sb_ifree > sbp->sb_icount)) {
> > 
> > Hmm.  On its face this seems reasonable for the superblock verifier, but
> > then I started wondering, since these are /summary/ counters.
> > 
> > If the free counts are off by this much, the admin won't be able to
> > mount the fs, and xfs_repair is the only other tool that can fix the
> > summary counts.  However, if the log is dirty, the mount won't succeed
> > to recover the fs, which is too bad since we can reinitialize the
> > summary counts after log recovery.  xfs_repair -L will be the only way
> > out, which will wreak havoc on the filesystem from discarding the log
> > contents.
> 
> Yup, that's why I said "catch this on /write/", not "always reject
> bad counter values".
> 
> i.e. we should never be writing a bad value, but we most definitely
> need to be able to mount the filesystem to reconstruct them.
> 
> > So, would it be preferable to split this into two parts?  For example,
> > have this as a corruption check in _sb_write_verify to prevent us from
> > writing out garbage counters
> 
> yes.
> 
> > and a clamp in _reinit_percpu_counters so
> > that we never present ridiculous free counts to users?
> 
> percpu_counter_{read,sum}_positive() should be used for anything that is
> userspace facing. xfs_fs_counts() gets this right, but
> xfs_fs_statfs() doesn't - it should use
> percpu_counter_sum_positive().

I don't think that will solve this problem -- although sb_fdblocks is
larger than sb_dblocks, sd_fdblocks is not so insanely large that
percpu_counter_{read,sum} return negative values; returning to Eric's
analysis of the original complaint:

> sb_fdblocks 4461713825, counted 166746529
>         - found root inode chunk
> 
> that sb_fdblocks really is ~17T which indicates the problem
> really is on disk.
> 
> 4461713825
> 100001001111100000101100110100001

  ^-- bit 32 of a signed 64-bit quantity

> 166746529
>      1001111100000101100110100001

So we still need a separate sb_fdblocks <= sb_dblocks clamp and/or
forced recalculation somewhere.

I agree that _fs_statfs should only return positive free blocks to avoid
reporting total garbage to userspace, but that's not the problme here.
I'll toss that onto my pile for 4.19 stuff.

> > (Does any of this make sense with !haslazysbcount filesystems?)
> 
> Same thing - we can't verify the counters on read until after log
> recovery as all the changes are journalled.
> 
> > Bonus question: What about checking frextents/rextents?
> 
> Same as !lazycount - all changes are journalled.

Ok.

--D

> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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

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

[Bill O'Donnell]

Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
Add sanity checks for these parameters.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v2: make extra sanity checks exclusive to writes (allow read)

 fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 350119eeaecb..6a98ec68e8ad 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -104,7 +104,8 @@ xfs_mount_validate_sb(
 	xfs_mount_t	*mp,
 	xfs_sb_t	*sbp,
 	bool		check_inprogress,
-	bool		check_version)
+	bool		check_version,
+	bool		write_flag)
 {
 	uint32_t	agcount = 0;
 	uint32_t	rem;
@@ -266,6 +267,15 @@ xfs_mount_validate_sb(
 		return -EFSCORRUPTED;
 	}
 
+	/* Additional sb sanity checks for writes */
+	if (write_flag) {
+		if (sbp->sb_fdblocks > sbp->sb_dblocks ||
+		    sbp->sb_ifree > sbp->sb_icount) {
+			    xfs_notice(mp, "SB sanity check failed");
+			    return -EFSCORRUPTED;
+		}
+	}
+
 	if (sbp->sb_unit) {
 		if (!xfs_sb_version_hasdalign(sbp) ||
 		    sbp->sb_unit > sbp->sb_width ||
@@ -599,7 +609,9 @@ xfs_sb_to_disk(
 static int
 xfs_sb_verify(
 	struct xfs_buf	*bp,
-	bool		check_version)
+	bool		check_version,
+	bool		write_flag)
+
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	struct xfs_sb	sb;
@@ -616,7 +628,7 @@ xfs_sb_verify(
 	 */
 	return xfs_mount_validate_sb(mp, &sb,
 				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
-				     check_version);
+				     check_version, write_flag);
 }
 
 /*
@@ -657,7 +669,7 @@ xfs_sb_read_verify(
 			}
 		}
 	}
-	error = xfs_sb_verify(bp, true);
+	error = xfs_sb_verify(bp, true, false);
 
 out_error:
 	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
@@ -695,7 +707,7 @@ xfs_sb_write_verify(
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
 	int			error;
 
-	error = xfs_sb_verify(bp, false);
+	error = xfs_sb_verify(bp, false, true);
 	if (error) {
 		xfs_verifier_error(bp, error, __this_address);
 		return;
-- 
2.17.1

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

[Carlos Maiolino]

On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v2: make extra sanity checks exclusive to writes (allow read)

It looks mostly ok for me, I'm still wondering if it wouldn't be better to avoid
adding a write_flag argument for xfs_sb_verify, but I think avoiding it would
require some refactoring of the sb verify code, once we convert from BE to CPU
in xfs_sb_verify, and I'm not sure if it is worth.

So,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
>  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 350119eeaecb..6a98ec68e8ad 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
>  	xfs_mount_t	*mp,
>  	xfs_sb_t	*sbp,
>  	bool		check_inprogress,
> -	bool		check_version)
> +	bool		check_version,
> +	bool		write_flag)
>  {
>  	uint32_t	agcount = 0;
>  	uint32_t	rem;
> @@ -266,6 +267,15 @@ xfs_mount_validate_sb(
>  		return -EFSCORRUPTED;
>  	}
>  
> +	/* Additional sb sanity checks for writes */
> +	if (write_flag) {
> +		if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> +		    sbp->sb_ifree > sbp->sb_icount) {
> +			    xfs_notice(mp, "SB sanity check failed");
> +			    return -EFSCORRUPTED;
> +		}
> +	}
> +
>  	if (sbp->sb_unit) {
>  		if (!xfs_sb_version_hasdalign(sbp) ||
>  		    sbp->sb_unit > sbp->sb_width ||
> @@ -599,7 +609,9 @@ xfs_sb_to_disk(
>  static int
>  xfs_sb_verify(
>  	struct xfs_buf	*bp,
> -	bool		check_version)
> +	bool		check_version,
> +	bool		write_flag)
> +
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_sb	sb;
> @@ -616,7 +628,7 @@ xfs_sb_verify(
>  	 */
>  	return xfs_mount_validate_sb(mp, &sb,
>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> -				     check_version);
> +				     check_version, write_flag);
>  }
>  
>  /*
> @@ -657,7 +669,7 @@ xfs_sb_read_verify(
>  			}
>  		}
>  	}
> -	error = xfs_sb_verify(bp, true);
> +	error = xfs_sb_verify(bp, true, false);
>  
>  out_error:
>  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> @@ -695,7 +707,7 @@ xfs_sb_write_verify(
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
>  	int			error;
>  
> -	error = xfs_sb_verify(bp, false);
> +	error = xfs_sb_verify(bp, false, true);
>  	if (error) {
>  		xfs_verifier_error(bp, error, __this_address);
>  		return;
> -- 
> 2.17.1
> 
> --
> 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

-- 
Carlos

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

[Darrick J. Wong]

On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v2: make extra sanity checks exclusive to writes (allow read)
> 
>  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 350119eeaecb..6a98ec68e8ad 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
>  	xfs_mount_t	*mp,
>  	xfs_sb_t	*sbp,
>  	bool		check_inprogress,
> -	bool		check_version)
> +	bool		check_version,
> +	bool		write_flag)

I notice that check_version and write_flag are always xor -- either
we're reading the sb and set check_version, or we're writing the sb and
set write_flag.  Perhaps we can combine these two as write_flag?

if (check_version)
	check version stuff...

becomes:

if (!write_flag)
	check version stuff...

and we only have to pass around one flag.

>  {
>  	uint32_t	agcount = 0;
>  	uint32_t	rem;
> @@ -266,6 +267,15 @@ xfs_mount_validate_sb(
>  		return -EFSCORRUPTED;
>  	}
>  
> +	/* Additional sb sanity checks for writes */
> +	if (write_flag) {
> +		if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> +		    sbp->sb_ifree > sbp->sb_icount) {

Hmm, we still need something that will detect this on read and set a
flag to force recalculation of the summary counters... though since a
patch to implement that flag is sitting in my tree I'll take care of
that part separately.

--D

> +			    xfs_notice(mp, "SB sanity check failed");
> +			    return -EFSCORRUPTED;
> +		}
> +	}
> +
>  	if (sbp->sb_unit) {
>  		if (!xfs_sb_version_hasdalign(sbp) ||
>  		    sbp->sb_unit > sbp->sb_width ||
> @@ -599,7 +609,9 @@ xfs_sb_to_disk(
>  static int
>  xfs_sb_verify(
>  	struct xfs_buf	*bp,
> -	bool		check_version)
> +	bool		check_version,
> +	bool		write_flag)
> +
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_sb	sb;
> @@ -616,7 +628,7 @@ xfs_sb_verify(
>  	 */
>  	return xfs_mount_validate_sb(mp, &sb,
>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> -				     check_version);
> +				     check_version, write_flag);
>  }
>  
>  /*
> @@ -657,7 +669,7 @@ xfs_sb_read_verify(
>  			}
>  		}
>  	}
> -	error = xfs_sb_verify(bp, true);
> +	error = xfs_sb_verify(bp, true, false);
>  
>  out_error:
>  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> @@ -695,7 +707,7 @@ xfs_sb_write_verify(
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
>  	int			error;
>  
> -	error = xfs_sb_verify(bp, false);
> +	error = xfs_sb_verify(bp, false, true);
>  	if (error) {
>  		xfs_verifier_error(bp, error, __this_address);
>  		return;
> -- 
> 2.17.1
> 
> --
> 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

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

[Bill O'Donnell]

On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote:
> On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > Add sanity checks for these parameters.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > v2: make extra sanity checks exclusive to writes (allow read)
> > 
> >  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 350119eeaecb..6a98ec68e8ad 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
> >  	xfs_mount_t	*mp,
> >  	xfs_sb_t	*sbp,
> >  	bool		check_inprogress,
> > -	bool		check_version)
> > +	bool		check_version,
> > +	bool		write_flag)
> 
> I notice that check_version and write_flag are always xor -- either
> we're reading the sb and set check_version, or we're writing the sb and
> set write_flag.  Perhaps we can combine these two as write_flag?
> 
> if (check_version)
> 	check version stuff...
> 
> becomes:
> 
> if (!write_flag)
> 	check version stuff...
> 
> and we only have to pass around one flag.

I suppose that makes sense, but my notion is that 2 unique flags
is preferable for clarity and mutual exclusiveness for anyone doing
subsequent patches.

> 
> >  {
> >  	uint32_t	agcount = 0;
> >  	uint32_t	rem;
> > @@ -266,6 +267,15 @@ xfs_mount_validate_sb(
> >  		return -EFSCORRUPTED;
> >  	}
> >  
> > +	/* Additional sb sanity checks for writes */
> > +	if (write_flag) {
> > +		if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> > +		    sbp->sb_ifree > sbp->sb_icount) {
> 
> Hmm, we still need something that will detect this on read and set a
> flag to force recalculation of the summary counters... though since a
> patch to implement that flag is sitting in my tree I'll take care of
> that part separately.

That sounds good, thanks!
-Bill

> 
> --D
> 
> > +			    xfs_notice(mp, "SB sanity check failed");
> > +			    return -EFSCORRUPTED;
> > +		}
> > +	}
> > +
> >  	if (sbp->sb_unit) {
> >  		if (!xfs_sb_version_hasdalign(sbp) ||
> >  		    sbp->sb_unit > sbp->sb_width ||
> > @@ -599,7 +609,9 @@ xfs_sb_to_disk(
> >  static int
> >  xfs_sb_verify(
> >  	struct xfs_buf	*bp,
> > -	bool		check_version)
> > +	bool		check_version,
> > +	bool		write_flag)
> > +
> >  {
> >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> >  	struct xfs_sb	sb;
> > @@ -616,7 +628,7 @@ xfs_sb_verify(
> >  	 */
> >  	return xfs_mount_validate_sb(mp, &sb,
> >  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> > -				     check_version);
> > +				     check_version, write_flag);
> >  }
> >  
> >  /*
> > @@ -657,7 +669,7 @@ xfs_sb_read_verify(
> >  			}
> >  		}
> >  	}
> > -	error = xfs_sb_verify(bp, true);
> > +	error = xfs_sb_verify(bp, true, false);
> >  
> >  out_error:
> >  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> > @@ -695,7 +707,7 @@ xfs_sb_write_verify(
> >  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> >  	int			error;
> >  
> > -	error = xfs_sb_verify(bp, false);
> > +	error = xfs_sb_verify(bp, false, true);
> >  	if (error) {
> >  		xfs_verifier_error(bp, error, __this_address);
> >  		return;
> > -- 
> > 2.17.1
> > 
> > --
> > 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

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

[Bill O'Donnell]

On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote:
> On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote:
> > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > > Add sanity checks for these parameters.
> > > 
> > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > ---
> > > v2: make extra sanity checks exclusive to writes (allow read)
> > > 
> > >  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
> > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index 350119eeaecb..6a98ec68e8ad 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
> > >  	xfs_mount_t	*mp,
> > >  	xfs_sb_t	*sbp,
> > >  	bool		check_inprogress,
> > > -	bool		check_version)
> > > +	bool		check_version,
> > > +	bool		write_flag)
> > 
> > I notice that check_version and write_flag are always xor -- either
> > we're reading the sb and set check_version, or we're writing the sb and
> > set write_flag.  Perhaps we can combine these two as write_flag?
> > 
> > if (check_version)
> > 	check version stuff...
> > 
> > becomes:
> > 
> > if (!write_flag)
> > 	check version stuff...
> > 
> > and we only have to pass around one flag.
> 
> I suppose that makes sense, but my notion is that 2 unique flags
> is preferable for clarity and mutual exclusiveness for anyone doing
> subsequent patches.

I'm all for simplifying and saving stack space, but is it ok
to turn a single purpose flag into a dual purpose one?

> 
> > 
> > >  {
> > >  	uint32_t	agcount = 0;
> > >  	uint32_t	rem;
> > > @@ -266,6 +267,15 @@ xfs_mount_validate_sb(
> > >  		return -EFSCORRUPTED;
> > >  	}
> > >  
> > > +	/* Additional sb sanity checks for writes */
> > > +	if (write_flag) {
> > > +		if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> > > +		    sbp->sb_ifree > sbp->sb_icount) {
> > 
> > Hmm, we still need something that will detect this on read and set a
> > flag to force recalculation of the summary counters... though since a
> > patch to implement that flag is sitting in my tree I'll take care of
> > that part separately.
> 
> That sounds good, thanks!
> -Bill
> 
> > 
> > --D
> > 
> > > +			    xfs_notice(mp, "SB sanity check failed");
> > > +			    return -EFSCORRUPTED;
> > > +		}
> > > +	}
> > > +
> > >  	if (sbp->sb_unit) {
> > >  		if (!xfs_sb_version_hasdalign(sbp) ||
> > >  		    sbp->sb_unit > sbp->sb_width ||
> > > @@ -599,7 +609,9 @@ xfs_sb_to_disk(
> > >  static int
> > >  xfs_sb_verify(
> > >  	struct xfs_buf	*bp,
> > > -	bool		check_version)
> > > +	bool		check_version,
> > > +	bool		write_flag)
> > > +
> > >  {
> > >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> > >  	struct xfs_sb	sb;
> > > @@ -616,7 +628,7 @@ xfs_sb_verify(
> > >  	 */
> > >  	return xfs_mount_validate_sb(mp, &sb,
> > >  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> > > -				     check_version);
> > > +				     check_version, write_flag);
> > >  }
> > >  
> > >  /*
> > > @@ -657,7 +669,7 @@ xfs_sb_read_verify(
> > >  			}
> > >  		}
> > >  	}
> > > -	error = xfs_sb_verify(bp, true);
> > > +	error = xfs_sb_verify(bp, true, false);
> > >  
> > >  out_error:
> > >  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> > > @@ -695,7 +707,7 @@ xfs_sb_write_verify(
> > >  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> > >  	int			error;
> > >  
> > > -	error = xfs_sb_verify(bp, false);
> > > +	error = xfs_sb_verify(bp, false, true);
> > >  	if (error) {
> > >  		xfs_verifier_error(bp, error, __this_address);
> > >  		return;
> > > -- 
> > > 2.17.1
> > > 
> > > --
> > > 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

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

[Darrick J. Wong]

On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote:
> On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote:
> > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote:
> > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > > > Add sanity checks for these parameters.
> > > > 
> > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > > ---
> > > > v2: make extra sanity checks exclusive to writes (allow read)
> > > > 
> > > >  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
> > > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > > index 350119eeaecb..6a98ec68e8ad 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
> > > >  	xfs_mount_t	*mp,
> > > >  	xfs_sb_t	*sbp,
> > > >  	bool		check_inprogress,
> > > > -	bool		check_version)
> > > > +	bool		check_version,
> > > > +	bool		write_flag)
> > > 
> > > I notice that check_version and write_flag are always xor -- either
> > > we're reading the sb and set check_version, or we're writing the sb and
> > > set write_flag.  Perhaps we can combine these two as write_flag?
> > > 
> > > if (check_version)
> > > 	check version stuff...
> > > 
> > > becomes:
> > > 
> > > if (!write_flag)
> > > 	check version stuff...
> > > 
> > > and we only have to pass around one flag.
> > 
> > I suppose that makes sense, but my notion is that 2 unique flags
> > is preferable for clarity and mutual exclusiveness for anyone doing
> > subsequent patches.
> 
> I'm all for simplifying and saving stack space, but is it ok
> to turn a single purpose flag into a dual purpose one?

That depends on the flag involved -- if they're mutually exclusive, then
I think it's ok to do that, so long as there's a comment nearby to
document the argument semantics.

In the case of this particular flag (check_version) it is set by the
read verifier so that we reject versions that we don't recognize; it is
not set by the write verifier because we don't change the v5 feature
masks at runtime* and we never write anything if the fs won't mount.

For write_flag, the read verifier never sets it because we have to be
able to mount the fs in case the log contains an sb with an updated set
of summary counters or for lazysbcount filesystems we'll recalculate the
counter after recovery; and we set write_flag at write time, obviously.

So having come this far, you could meld them into a single parameter so
long as you note that write_flag == true means that we're writing the fs
and write_flag == false means we want to check the v5 feature flags at
mount time to reject features bits that we don't recognize.

* Oh, but what about that pesky asterisk?  My sense of paranoia wonders
why we don't check the v5 feature flags on write too, just in case
memory gets corrupted.  I think the reason is that we don't allow
feature flag changes at runtime, we'll check the changes at ioctl time
if we ever do support runtime feature flag updates, and we implicitly
trust memory not to corrupt memory on us (ha ha ha).

At this point my tldr opinion is "seems fine to me, let's see if any of
the lurking vacationers have anything to say?  We're still ~3 weeks to
the next merge window.

--D

> > 
> > > 
> > > >  {
> > > >  	uint32_t	agcount = 0;
> > > >  	uint32_t	rem;
> > > > @@ -266,6 +267,15 @@ xfs_mount_validate_sb(
> > > >  		return -EFSCORRUPTED;
> > > >  	}
> > > >  
> > > > +	/* Additional sb sanity checks for writes */
> > > > +	if (write_flag) {
> > > > +		if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> > > > +		    sbp->sb_ifree > sbp->sb_icount) {
> > > 
> > > Hmm, we still need something that will detect this on read and set a
> > > flag to force recalculation of the summary counters... though since a
> > > patch to implement that flag is sitting in my tree I'll take care of
> > > that part separately.
> > 
> > That sounds good, thanks!
> > -Bill
> > 
> > > 
> > > --D
> > > 
> > > > +			    xfs_notice(mp, "SB sanity check failed");
> > > > +			    return -EFSCORRUPTED;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	if (sbp->sb_unit) {
> > > >  		if (!xfs_sb_version_hasdalign(sbp) ||
> > > >  		    sbp->sb_unit > sbp->sb_width ||
> > > > @@ -599,7 +609,9 @@ xfs_sb_to_disk(
> > > >  static int
> > > >  xfs_sb_verify(
> > > >  	struct xfs_buf	*bp,
> > > > -	bool		check_version)
> > > > +	bool		check_version,
> > > > +	bool		write_flag)
> > > > +
> > > >  {
> > > >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> > > >  	struct xfs_sb	sb;
> > > > @@ -616,7 +628,7 @@ xfs_sb_verify(
> > > >  	 */
> > > >  	return xfs_mount_validate_sb(mp, &sb,
> > > >  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> > > > -				     check_version);
> > > > +				     check_version, write_flag);
> > > >  }
> > > >  
> > > >  /*
> > > > @@ -657,7 +669,7 @@ xfs_sb_read_verify(
> > > >  			}
> > > >  		}
> > > >  	}
> > > > -	error = xfs_sb_verify(bp, true);
> > > > +	error = xfs_sb_verify(bp, true, false);
> > > >  
> > > >  out_error:
> > > >  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> > > > @@ -695,7 +707,7 @@ xfs_sb_write_verify(
> > > >  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> > > >  	int			error;
> > > >  
> > > > -	error = xfs_sb_verify(bp, false);
> > > > +	error = xfs_sb_verify(bp, false, true);
> > > >  	if (error) {
> > > >  		xfs_verifier_error(bp, error, __this_address);
> > > >  		return;
> > > > -- 
> > > > 2.17.1
> > > > 
> > > > --
> > > > 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

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

[Dave Chinner]

On Tue, Jul 17, 2018 at 01:33:01PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote:
> > On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote:
> > > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote:
> > > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> > > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > > > > Add sanity checks for these parameters.
> > > > > 
> > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > > > ---
> > > > > v2: make extra sanity checks exclusive to writes (allow read)
> > > > > 
> > > > >  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
> > > > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > > > index 350119eeaecb..6a98ec68e8ad 100644
> > > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
> > > > >  	xfs_mount_t	*mp,
> > > > >  	xfs_sb_t	*sbp,
> > > > >  	bool		check_inprogress,
> > > > > -	bool		check_version)
> > > > > +	bool		check_version,
> > > > > +	bool		write_flag)
> > > > 
> > > > I notice that check_version and write_flag are always xor -- either
> > > > we're reading the sb and set check_version, or we're writing the sb and
> > > > set write_flag.  Perhaps we can combine these two as write_flag?
> > > > 
> > > > if (check_version)
> > > > 	check version stuff...
> > > > 
> > > > becomes:
> > > > 
> > > > if (!write_flag)
> > > > 	check version stuff...
> > > > 
> > > > and we only have to pass around one flag.
> > > 
> > > I suppose that makes sense, but my notion is that 2 unique flags
> > > is preferable for clarity and mutual exclusiveness for anyone doing
> > > subsequent patches.
> > 
> > I'm all for simplifying and saving stack space, but is it ok
> > to turn a single purpose flag into a dual purpose one?
> 
> That depends on the flag involved -- if they're mutually exclusive, then
> I think it's ok to do that, so long as there's a comment nearby to
> document the argument semantics.
> 
> In the case of this particular flag (check_version) it is set by the
> read verifier so that we reject versions that we don't recognize; it is
> not set by the write verifier because we don't change the v5 feature
> masks at runtime* and we never write anything if the fs won't mount.
> 
> For write_flag, the read verifier never sets it because we have to be
> able to mount the fs in case the log contains an sb with an updated set
> of summary counters or for lazysbcount filesystems we'll recalculate the
> counter after recovery; and we set write_flag at write time, obviously.
> 
> So having come this far, you could meld them into a single parameter so
> long as you note that write_flag == true means that we're writing the fs
> and write_flag == false means we want to check the v5 feature flags at
> mount time to reject features bits that we don't recognize.
> 
> * Oh, but what about that pesky asterisk?  My sense of paranoia wonders
> why we don't check the v5 feature flags on write too, just in case
> memory gets corrupted.  I think the reason is that we don't allow
> feature flag changes at runtime, we'll check the changes at ioctl time
> if we ever do support runtime feature flag updates, and we implicitly
> trust memory not to corrupt memory on us (ha ha ha).
> 
> At this point my tldr opinion is "seems fine to me, let's see if any of
> the lurking vacationers have anything to say?  We're still ~3 weeks to
> the next merge window.

My initial reaction was "urk!". I think we should consider putting
this check in xfs_sb_verify_write(), not xfs_mount_validate_sb().
We've kinda taken all the mount time checks (which have to be
liberal because we can a) be handed non-XFS filesystems, and b)
handed filesystems that need recovery) and applied them at write
time, too, then special cased the read side primary superblock stuff
with a flag.

The thing is, write time checks (obviously) need to be stricter than
the mount time checks, and we have to check different things. Hence
I think we need to do slightly more work here to clean this up. i.e.
stop calling xfs_sb_verify() in xfs_sb_write_verify() and open code
it instead, then add all these write-only verifier cases into
xfs_sb_write_verify().

Similarly, open code xfs_sb_verify() and move all the read-side
checks into xfs_sb_read_verify().

Then rename xfs_mount_validate_sb() to xfs_sb_verify_common(), as it
only contains the checks that both the read and write side do, and
it doesn't need any extra parameters at all...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Bill O'Donnell]

On Wed, Jul 18, 2018 at 09:26:16AM +1000, Dave Chinner wrote:
> On Tue, Jul 17, 2018 at 01:33:01PM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 17, 2018 at 02:12:53PM -0500, Bill O'Donnell wrote:
> > > On Tue, Jul 17, 2018 at 12:17:14PM -0500, Bill O'Donnell wrote:
> > > > On Tue, Jul 17, 2018 at 10:06:54AM -0700, Darrick J. Wong wrote:
> > > > > On Mon, Jul 16, 2018 at 02:26:55PM -0500, Bill O'Donnell wrote:
> > > > > > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > > > > > Add sanity checks for these parameters.
> > > > > > 
> > > > > > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > > > > > ---
> > > > > > v2: make extra sanity checks exclusive to writes (allow read)
> > > > > > 
> > > > > >  fs/xfs/libxfs/xfs_sb.c | 22 +++++++++++++++++-----
> > > > > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > > > > index 350119eeaecb..6a98ec68e8ad 100644
> > > > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > > > @@ -104,7 +104,8 @@ xfs_mount_validate_sb(
> > > > > >  	xfs_mount_t	*mp,
> > > > > >  	xfs_sb_t	*sbp,
> > > > > >  	bool		check_inprogress,
> > > > > > -	bool		check_version)
> > > > > > +	bool		check_version,
> > > > > > +	bool		write_flag)
> > > > > 
> > > > > I notice that check_version and write_flag are always xor -- either
> > > > > we're reading the sb and set check_version, or we're writing the sb and
> > > > > set write_flag.  Perhaps we can combine these two as write_flag?
> > > > > 
> > > > > if (check_version)
> > > > > 	check version stuff...
> > > > > 
> > > > > becomes:
> > > > > 
> > > > > if (!write_flag)
> > > > > 	check version stuff...
> > > > > 
> > > > > and we only have to pass around one flag.
> > > > 
> > > > I suppose that makes sense, but my notion is that 2 unique flags
> > > > is preferable for clarity and mutual exclusiveness for anyone doing
> > > > subsequent patches.
> > > 
> > > I'm all for simplifying and saving stack space, but is it ok
> > > to turn a single purpose flag into a dual purpose one?
> > 
> > That depends on the flag involved -- if they're mutually exclusive, then
> > I think it's ok to do that, so long as there's a comment nearby to
> > document the argument semantics.
> > 
> > In the case of this particular flag (check_version) it is set by the
> > read verifier so that we reject versions that we don't recognize; it is
> > not set by the write verifier because we don't change the v5 feature
> > masks at runtime* and we never write anything if the fs won't mount.
> > 
> > For write_flag, the read verifier never sets it because we have to be
> > able to mount the fs in case the log contains an sb with an updated set
> > of summary counters or for lazysbcount filesystems we'll recalculate the
> > counter after recovery; and we set write_flag at write time, obviously.
> > 
> > So having come this far, you could meld them into a single parameter so
> > long as you note that write_flag == true means that we're writing the fs
> > and write_flag == false means we want to check the v5 feature flags at
> > mount time to reject features bits that we don't recognize.
> > 
> > * Oh, but what about that pesky asterisk?  My sense of paranoia wonders
> > why we don't check the v5 feature flags on write too, just in case
> > memory gets corrupted.  I think the reason is that we don't allow
> > feature flag changes at runtime, we'll check the changes at ioctl time
> > if we ever do support runtime feature flag updates, and we implicitly
> > trust memory not to corrupt memory on us (ha ha ha).
> > 
> > At this point my tldr opinion is "seems fine to me, let's see if any of
> > the lurking vacationers have anything to say?  We're still ~3 weeks to
> > the next merge window.
> 
> My initial reaction was "urk!". I think we should consider putting
> this check in xfs_sb_verify_write(), not xfs_mount_validate_sb().
> We've kinda taken all the mount time checks (which have to be
> liberal because we can a) be handed non-XFS filesystems, and b)
> handed filesystems that need recovery) and applied them at write
> time, too, then special cased the read side primary superblock stuff
> with a flag.

> 
> The thing is, write time checks (obviously) need to be stricter than
> the mount time checks, and we have to check different things. Hence
> I think we need to do slightly more work here to clean this up. i.e.
> stop calling xfs_sb_verify() in xfs_sb_write_verify() and open code
> it instead, then add all these write-only verifier cases into
> xfs_sb_write_verify().
> 
> Similarly, open code xfs_sb_verify() and move all the read-side
> checks into xfs_sb_read_verify().
> 
> Then rename xfs_mount_validate_sb() to xfs_sb_verify_common(), as it
> only contains the checks that both the read and write side do, and
> it doesn't need any extra parameters at all...

Ok, IIUC, you suggest a partial gutting of xfs_mount_validate_sb(),
and open coding all the write and read checks in xfs_sb_write_verify() 
and xfs_sb_read_verify(), respectively (keeping common checks in 
xfs_mount_validate_sb().

What's not intuitively obvious to me:
which sanity checks in the current xfs_mount_validate_sb() are common
to read and write (and which are exlusive). 

Thanks-
Bill

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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

[Bill O'Donnell]

Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
Add sanity checks for these parameters.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
v3: eliminate need for additional write_flag, doing those
    unique checks in xfs_sb_write_verify()
v2: make extra sanity checks exclusive to writes


 fs/xfs/libxfs/xfs_sb.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b3ad15956366..f583fb8a10e1 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -599,22 +599,16 @@ xfs_sb_to_disk(
 static int
 xfs_sb_verify(
 	struct xfs_buf	*bp,
+	struct xfs_sb	*sb,
 	bool		check_version)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	struct xfs_sb	sb;
-
-	/*
-	 * Use call variant which doesn't convert quota flags from disk 
-	 * format, because xfs_mount_validate_sb checks the on-disk flags.
-	 */
-	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
 
 	/*
 	 * Only check the in progress field for the primary superblock as
 	 * mkfs.xfs doesn't clear it from secondary superblocks.
 	 */
-	return xfs_mount_validate_sb(mp, &sb,
+	return xfs_mount_validate_sb(mp, sb,
 				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
 				     check_version);
 }
@@ -637,6 +631,7 @@ xfs_sb_read_verify(
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
+	struct xfs_sb	sb;
 	int		error;
 
 	/*
@@ -657,7 +652,13 @@ xfs_sb_read_verify(
 			}
 		}
 	}
-	error = xfs_sb_verify(bp, true);
+
+	/*
+	 * Use call variant which doesn't convert quota flags from disk
+	 * format, because xfs_mount_validate_sb checks the on-disk flags.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+	error = xfs_sb_verify(bp, &sb, true);
 
 out_error:
 	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
@@ -693,9 +694,26 @@ xfs_sb_write_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
+	struct xfs_sb		sb;
 	int			error;
 
-	error = xfs_sb_verify(bp, false);
+	/*
+	 * Use call variant which doesn't convert quota flags from disk
+	 * format, because xfs_mount_validate_sb checks the on-disk flags.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+
+	error = xfs_sb_verify(bp, &sb, false);
+
+	/* 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;
+		}
+	}
+
 	if (error) {
 		xfs_verifier_error(bp, error, __this_address);
 		return;
-- 
2.17.1

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

[Darrick J. Wong]

On Wed, Jul 25, 2018 at 04:33:36PM -0500, Bill O'Donnell wrote:
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> v3: eliminate need for additional write_flag, doing those
>     unique checks in xfs_sb_write_verify()
> v2: make extra sanity checks exclusive to writes
> 
> 
>  fs/xfs/libxfs/xfs_sb.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b3ad15956366..f583fb8a10e1 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -599,22 +599,16 @@ xfs_sb_to_disk(
>  static int
>  xfs_sb_verify(
>  	struct xfs_buf	*bp,
> +	struct xfs_sb	*sb,
>  	bool		check_version)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
> -	struct xfs_sb	sb;
> -
> -	/*
> -	 * Use call variant which doesn't convert quota flags from disk 
> -	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> -	 */
> -	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
>  
>  	/*
>  	 * Only check the in progress field for the primary superblock as
>  	 * mkfs.xfs doesn't clear it from secondary superblocks.
>  	 */
> -	return xfs_mount_validate_sb(mp, &sb,
> +	return xfs_mount_validate_sb(mp, sb,
>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
>  				     check_version);
>  }
> @@ -637,6 +631,7 @@ xfs_sb_read_verify(
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
> +	struct xfs_sb	sb;
>  	int		error;
>  
>  	/*
> @@ -657,7 +652,13 @@ xfs_sb_read_verify(
>  			}
>  		}
>  	}
> -	error = xfs_sb_verify(bp, true);
> +
> +	/*
> +	 * Use call variant which doesn't convert quota flags from disk
> +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> +	 */
> +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> +	error = xfs_sb_verify(bp, &sb, true);
>  
>  out_error:
>  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> @@ -693,9 +694,26 @@ xfs_sb_write_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> +	struct xfs_sb		sb;
>  	int			error;
>  
> -	error = xfs_sb_verify(bp, false);
> +	/*
> +	 * Use call variant which doesn't convert quota flags from disk
> +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> +	 */
> +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> +
> +	error = xfs_sb_verify(bp, &sb, false);
> +
> +	/* 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;
> +		}
> +	}

On the off chance that some day we add more write-time checks, could you
please structure this the usual way?

	error = xfs_sb_verify(bp, &sb, false);
	if (error)
		goto err;

	if (sb.sb_fdblocks > sb.sb_dblocks || sb.sb_ifree > sb.sb_icount) {
		xfs_notice(mp, "SB summary counter sanity check failed");
		error = -EFSCORRUPTED;
		goto err;
	}

err:
	if (error) {
		xfs_verifier_error(bp, error, __this_address);
		return;
	}
}

Other than that, this looks ok to me.

--D

> +
>  	if (error) {
>  		xfs_verifier_error(bp, error, __this_address);
>  		return;
> -- 
> 2.17.1
> 
> --
> 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

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

[Bill O'Donnell]

On Wed, Jul 25, 2018 at 02:47:47PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 25, 2018 at 04:33:36PM -0500, Bill O'Donnell wrote:
> > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > Add sanity checks for these parameters.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > v3: eliminate need for additional write_flag, doing those
> >     unique checks in xfs_sb_write_verify()
> > v2: make extra sanity checks exclusive to writes
> > 
> > 
> >  fs/xfs/libxfs/xfs_sb.c | 38 ++++++++++++++++++++++++++++----------
> >  1 file changed, 28 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index b3ad15956366..f583fb8a10e1 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -599,22 +599,16 @@ xfs_sb_to_disk(
> >  static int
> >  xfs_sb_verify(
> >  	struct xfs_buf	*bp,
> > +	struct xfs_sb	*sb,
> >  	bool		check_version)
> >  {
> >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> > -	struct xfs_sb	sb;
> > -
> > -	/*
> > -	 * Use call variant which doesn't convert quota flags from disk 
> > -	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> > -	 */
> > -	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> >  
> >  	/*
> >  	 * Only check the in progress field for the primary superblock as
> >  	 * mkfs.xfs doesn't clear it from secondary superblocks.
> >  	 */
> > -	return xfs_mount_validate_sb(mp, &sb,
> > +	return xfs_mount_validate_sb(mp, sb,
> >  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> >  				     check_version);
> >  }
> > @@ -637,6 +631,7 @@ xfs_sb_read_verify(
> >  {
> >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> >  	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
> > +	struct xfs_sb	sb;
> >  	int		error;
> >  
> >  	/*
> > @@ -657,7 +652,13 @@ xfs_sb_read_verify(
> >  			}
> >  		}
> >  	}
> > -	error = xfs_sb_verify(bp, true);
> > +
> > +	/*
> > +	 * Use call variant which doesn't convert quota flags from disk
> > +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> > +	 */
> > +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> > +	error = xfs_sb_verify(bp, &sb, true);
> >  
> >  out_error:
> >  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> > @@ -693,9 +694,26 @@ xfs_sb_write_verify(
> >  {
> >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> >  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> > +	struct xfs_sb		sb;
> >  	int			error;
> >  
> > -	error = xfs_sb_verify(bp, false);
> > +	/*
> > +	 * Use call variant which doesn't convert quota flags from disk
> > +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> > +	 */
> > +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> > +
> > +	error = xfs_sb_verify(bp, &sb, false);
> > +
> > +	/* 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;
> > +		}
> > +	}
> 
> On the off chance that some day we add more write-time checks, could you
> please structure this the usual way?

ah, good idea... will do!
Thanks-
Bill


> 
> 	error = xfs_sb_verify(bp, &sb, false);
> 	if (error)
> 		goto err;
> 
> 	if (sb.sb_fdblocks > sb.sb_dblocks || sb.sb_ifree > sb.sb_icount) {
> 		xfs_notice(mp, "SB summary counter sanity check failed");
> 		error = -EFSCORRUPTED;
> 		goto err;
> 	}
> 
> err:
> 	if (error) {
> 		xfs_verifier_error(bp, error, __this_address);
> 		return;
> 	}
> }
> 
> Other than that, this looks ok to me.
> 
> --D
> 
> > +
> >  	if (error) {
> >  		xfs_verifier_error(bp, error, __this_address);
> >  		return;
> > -- 
> > 2.17.1
> > 
> > --
> > 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

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

[Eric Sandeen]

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) ||
+		    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

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

[Darrick J. Wong]

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

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

[Bill O'Donnell]

Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
Add sanity checks for these parameters.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---

v4: adjust control flow to standard error checking. Add
    more descriptive comment. Add validation for sb_icount.
v3: eliminate need for additional write_flag, doing those
    unique checks in xfs_sb_write_verify()
v2: make extra sanity checks exclusive to writes

 fs/xfs/libxfs/xfs_sb.c | 46 +++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b3ad15956366..8d8e579ca426 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -599,22 +599,16 @@ xfs_sb_to_disk(
 static int
 xfs_sb_verify(
 	struct xfs_buf	*bp,
+	struct xfs_sb	*sb,
 	bool		check_version)
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
-	struct xfs_sb	sb;
-
-	/*
-	 * Use call variant which doesn't convert quota flags from disk 
-	 * format, because xfs_mount_validate_sb checks the on-disk flags.
-	 */
-	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
 
 	/*
 	 * Only check the in progress field for the primary superblock as
 	 * mkfs.xfs doesn't clear it from secondary superblocks.
 	 */
-	return xfs_mount_validate_sb(mp, &sb,
+	return xfs_mount_validate_sb(mp, sb,
 				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
 				     check_version);
 }
@@ -637,6 +631,7 @@ xfs_sb_read_verify(
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
+	struct xfs_sb	sb;
 	int		error;
 
 	/*
@@ -657,7 +652,13 @@ xfs_sb_read_verify(
 			}
 		}
 	}
-	error = xfs_sb_verify(bp, true);
+
+	/*
+	 * Use call variant which doesn't convert quota flags from disk
+	 * format, because xfs_mount_validate_sb checks the on-disk flags.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+	error = xfs_sb_verify(bp, &sb, true);
 
 out_error:
 	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
@@ -693,9 +694,34 @@ xfs_sb_write_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
+	struct xfs_sb		sb;
 	int			error;
 
-	error = xfs_sb_verify(bp, false);
+	/*
+	 * Use call variant which doesn't convert quota flags from disk
+	 * format, because xfs_mount_validate_sb checks the on-disk flags.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+
+	error = xfs_sb_verify(bp, &sb, false);
+	if (error)
+		goto err;
+
+	/*
+	 * Carry out additional sb sanity checks exclusively for writes.
+	 * We don't do these checks for reads, since faulty parameters could
+	 * already be on disk, and we shouldn't preclude reads for those
+	 * cases.
+	 */
+	if (sb.sb_fdblocks > sb.sb_dblocks ||
+	    sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks ||
+	    sb.sb_ifree > sb.sb_icount) {
+		xfs_notice(mp, "SB sanity check failed");
+		error = -EFSCORRUPTED;
+		goto err;
+	}
+
+err:
 	if (error) {
 		xfs_verifier_error(bp, error, __this_address);
 		return;
-- 
2.17.1

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

[Darrick J. Wong]

On Thu, Jul 26, 2018 at 11:40:46AM -0500, Bill O'Donnell wrote:
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
> 
> v4: adjust control flow to standard error checking. Add
>     more descriptive comment. Add validation for sb_icount.
> v3: eliminate need for additional write_flag, doing those
>     unique checks in xfs_sb_write_verify()
> v2: make extra sanity checks exclusive to writes
> 
>  fs/xfs/libxfs/xfs_sb.c | 46 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b3ad15956366..8d8e579ca426 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -599,22 +599,16 @@ xfs_sb_to_disk(
>  static int
>  xfs_sb_verify(
>  	struct xfs_buf	*bp,
> +	struct xfs_sb	*sb,
>  	bool		check_version)
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
> -	struct xfs_sb	sb;
> -
> -	/*
> -	 * Use call variant which doesn't convert quota flags from disk 
> -	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> -	 */
> -	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
>  
>  	/*
>  	 * Only check the in progress field for the primary superblock as
>  	 * mkfs.xfs doesn't clear it from secondary superblocks.
>  	 */
> -	return xfs_mount_validate_sb(mp, &sb,
> +	return xfs_mount_validate_sb(mp, sb,
>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
>  				     check_version);
>  }
> @@ -637,6 +631,7 @@ xfs_sb_read_verify(
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
> +	struct xfs_sb	sb;
>  	int		error;
>  
>  	/*
> @@ -657,7 +652,13 @@ xfs_sb_read_verify(
>  			}
>  		}
>  	}
> -	error = xfs_sb_verify(bp, true);
> +
> +	/*
> +	 * Use call variant which doesn't convert quota flags from disk
> +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> +	 */
> +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> +	error = xfs_sb_verify(bp, &sb, true);
>  
>  out_error:
>  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> @@ -693,9 +694,34 @@ xfs_sb_write_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> +	struct xfs_sb		sb;
>  	int			error;
>  
> -	error = xfs_sb_verify(bp, false);
> +	/*
> +	 * Use call variant which doesn't convert quota flags from disk
> +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> +	 */
> +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> +
> +	error = xfs_sb_verify(bp, &sb, false);
> +	if (error)
> +		goto err;
> +
> +	/*
> +	 * Carry out additional sb sanity checks exclusively for writes.
> +	 * We don't do these checks for reads, since faulty parameters could
> +	 * already be on disk, and we shouldn't preclude reads for those
> +	 * cases.
> +	 */
> +	if (sb.sb_fdblocks > sb.sb_dblocks ||
> +	    sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks ||

This is a 64-bit division, which won't work on 32-bit builds.

That said, I also wrote a xfs_verify_icount function for the fs summary
scrub patch, so perhaps we should collaborate to land both of these?
Send a v5 without this test and I'll immediately send a patch adding
both the _verify_icount and putting it to use here.

Actually, I could just modify your patch, add mine, and send both of
them.  That'd be easier...

--D

> +	    sb.sb_ifree > sb.sb_icount) {
> +		xfs_notice(mp, "SB sanity check failed");
> +		error = -EFSCORRUPTED;
> +		goto err;
> +	}
> +
> +err:
>  	if (error) {
>  		xfs_verifier_error(bp, error, __this_address);
>  		return;
> -- 
> 2.17.1
> 
> --
> 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

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

[Bill O'Donnell]

On Thu, Jul 26, 2018 at 10:07:34AM -0700, Darrick J. Wong wrote:
> On Thu, Jul 26, 2018 at 11:40:46AM -0500, Bill O'Donnell wrote:
> > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > Add sanity checks for these parameters.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> > 
> > v4: adjust control flow to standard error checking. Add
> >     more descriptive comment. Add validation for sb_icount.
> > v3: eliminate need for additional write_flag, doing those
> >     unique checks in xfs_sb_write_verify()
> > v2: make extra sanity checks exclusive to writes
> > 
> >  fs/xfs/libxfs/xfs_sb.c | 46 +++++++++++++++++++++++++++++++++---------
> >  1 file changed, 36 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index b3ad15956366..8d8e579ca426 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -599,22 +599,16 @@ xfs_sb_to_disk(
> >  static int
> >  xfs_sb_verify(
> >  	struct xfs_buf	*bp,
> > +	struct xfs_sb	*sb,
> >  	bool		check_version)
> >  {
> >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> > -	struct xfs_sb	sb;
> > -
> > -	/*
> > -	 * Use call variant which doesn't convert quota flags from disk 
> > -	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> > -	 */
> > -	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> >  
> >  	/*
> >  	 * Only check the in progress field for the primary superblock as
> >  	 * mkfs.xfs doesn't clear it from secondary superblocks.
> >  	 */
> > -	return xfs_mount_validate_sb(mp, &sb,
> > +	return xfs_mount_validate_sb(mp, sb,
> >  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
> >  				     check_version);
> >  }
> > @@ -637,6 +631,7 @@ xfs_sb_read_verify(
> >  {
> >  	struct xfs_mount *mp = bp->b_target->bt_mount;
> >  	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
> > +	struct xfs_sb	sb;
> >  	int		error;
> >  
> >  	/*
> > @@ -657,7 +652,13 @@ xfs_sb_read_verify(
> >  			}
> >  		}
> >  	}
> > -	error = xfs_sb_verify(bp, true);
> > +
> > +	/*
> > +	 * Use call variant which doesn't convert quota flags from disk
> > +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> > +	 */
> > +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> > +	error = xfs_sb_verify(bp, &sb, true);
> >  
> >  out_error:
> >  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> > @@ -693,9 +694,34 @@ xfs_sb_write_verify(
> >  {
> >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> >  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> > +	struct xfs_sb		sb;
> >  	int			error;
> >  
> > -	error = xfs_sb_verify(bp, false);
> > +	/*
> > +	 * Use call variant which doesn't convert quota flags from disk
> > +	 * format, because xfs_mount_validate_sb checks the on-disk flags.
> > +	 */
> > +	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
> > +
> > +	error = xfs_sb_verify(bp, &sb, false);
> > +	if (error)
> > +		goto err;
> > +
> > +	/*
> > +	 * Carry out additional sb sanity checks exclusively for writes.
> > +	 * We don't do these checks for reads, since faulty parameters could
> > +	 * already be on disk, and we shouldn't preclude reads for those
> > +	 * cases.
> > +	 */
> > +	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > +	    sb.sb_icount / sb.sb_inopblock > sb.sb_dblocks ||
> 
> This is a 64-bit division, which won't work on 32-bit builds.
> 
> That said, I also wrote a xfs_verify_icount function for the fs summary
> scrub patch, so perhaps we should collaborate to land both of these?
> Send a v5 without this test and I'll immediately send a patch adding
> both the _verify_icount and putting it to use here.
> 
> Actually, I could just modify your patch, add mine, and send both of
> them.  That'd be easier...

That sounds fine. Thanks!
Bill

> 
> --D
> 
> > +	    sb.sb_ifree > sb.sb_icount) {
> > +		xfs_notice(mp, "SB sanity check failed");
> > +		error = -EFSCORRUPTED;
> > +		goto err;
> > +	}
> > +
> > +err:
> >  	if (error) {
> >  		xfs_verifier_error(bp, error, __this_address);
> >  		return;
> > -- 
> > 2.17.1
> > 
> > --
> > 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