[PATCH 0/2] xfs: a couple of syzbot fixes

[PATCH 0/2] xfs: a couple of syzbot fixes

[Dave Chinner]

Hi folks,

Here are a couple of simple patches that reduce the syzbot noise
we've been getting. The superblock version verifier change is cc'd
for stable so that it gets back to stable kernels and distros
so they behave consistently with upstream w.r.t. future on-disk
format versions.

-Dave.

[PATCH 1/2] xfs: remove WARN when dquot cache insertion fails

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

It just creates unnecessary bot noise these days.

Reported-by: syzbot+6ae213503fb12e87934f@syzkaller.appspotmail.com
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_dquot.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index 8fb90da89787..7f071757f278 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -798,7 +798,6 @@ xfs_qm_dqget_cache_insert(
 	error = radix_tree_insert(tree, id, dqp);
 	if (unlikely(error)) {
 		/* Duplicate found!  Caller must try again. */
-		WARN_ON(error != -EEXIST);
 		mutex_unlock(&qi->qi_tree_lock);
 		trace_xfs_dqget_dup(dqp);
 		return error;
-- 
2.39.2

Re: [PATCH 1/2] xfs: remove WARN when dquot cache insertion fails

[Darrick J. Wong]

On Wed, Apr 12, 2023 at 09:23:41AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It just creates unnecessary bot noise these days.
> 
> Reported-by: syzbot+6ae213503fb12e87934f@syzkaller.appspotmail.com
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_dquot.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index 8fb90da89787..7f071757f278 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -798,7 +798,6 @@ xfs_qm_dqget_cache_insert(
>  	error = radix_tree_insert(tree, id, dqp);
>  	if (unlikely(error)) {
>  		/* Duplicate found!  Caller must try again. */
> -		WARN_ON(error != -EEXIST);
>  		mutex_unlock(&qi->qi_tree_lock);
>  		trace_xfs_dqget_dup(dqp);
>  		return error;
> -- 
> 2.39.2
> 

Re: [PATCH 1/2] xfs: remove WARN when dquot cache insertion fails

[Christoph Hellwig]

On Wed, Apr 12, 2023 at 09:23:41AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It just creates unnecessary bot noise these days.

Looks good:

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

[PATCH 2/2] xfs: don't consider future format versions valid

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

In commit fe08cc504448 we reworked the valid superblock version
checks. If it is a V5 filesystem, it is always valid, then we
checked if the version was less than V4 (reject) and then checked
feature fields in the V4 flags to determine if it was valid.

What we missed was that if the version is not V4 at this point,
we shoudl reject the fs. i.e. the check current treats V6+
filesystems as if it was a v4 filesystem. Fix this.

cc: stable@vger.kernel.org
Fixes: fe08cc504448 ("xfs: open code sb verifier feature checks")
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_sb.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 99cc03a298e2..ba0f17bc1dc0 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -72,7 +72,8 @@ xfs_sb_validate_v5_features(
 }
 
 /*
- * We support all XFS versions newer than a v4 superblock with V2 directories.
+ * We current support XFS v5 formats with known features and v4 superblocks with
+ * at least V2 directories.
  */
 bool
 xfs_sb_good_version(
@@ -86,16 +87,16 @@ xfs_sb_good_version(
 	if (xfs_sb_is_v5(sbp))
 		return xfs_sb_validate_v5_features(sbp);
 
+	/* versions prior to v4 are not supported */
+	if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_4)
+		return false;
+
 	/* We must not have any unknown v4 feature bits set */
 	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
 	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
 	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
 		return false;
 
-	/* versions prior to v4 are not supported */
-	if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4)
-		return false;
-
 	/* V4 filesystems need v2 directories and unwritten extents */
 	if (!(sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT))
 		return false;
-- 
2.39.2

Re: [PATCH 2/2] xfs: don't consider future format versions valid

[Darrick J. Wong]

On Wed, Apr 12, 2023 at 09:23:42AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> In commit fe08cc504448 we reworked the valid superblock version
> checks. If it is a V5 filesystem, it is always valid, then we
> checked if the version was less than V4 (reject) and then checked
> feature fields in the V4 flags to determine if it was valid.
> 
> What we missed was that if the version is not V4 at this point,
> we shoudl reject the fs. i.e. the check current treats V6+
> filesystems as if it was a v4 filesystem. Fix this.
> 
> cc: stable@vger.kernel.org
> Fixes: fe08cc504448 ("xfs: open code sb verifier feature checks")
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Ugh, old code...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_sb.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 99cc03a298e2..ba0f17bc1dc0 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -72,7 +72,8 @@ xfs_sb_validate_v5_features(
>  }
>  
>  /*
> - * We support all XFS versions newer than a v4 superblock with V2 directories.
> + * We current support XFS v5 formats with known features and v4 superblocks with
> + * at least V2 directories.
>   */
>  bool
>  xfs_sb_good_version(
> @@ -86,16 +87,16 @@ xfs_sb_good_version(
>  	if (xfs_sb_is_v5(sbp))
>  		return xfs_sb_validate_v5_features(sbp);
>  
> +	/* versions prior to v4 are not supported */
> +	if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_4)
> +		return false;
> +
>  	/* We must not have any unknown v4 feature bits set */
>  	if ((sbp->sb_versionnum & ~XFS_SB_VERSION_OKBITS) ||
>  	    ((sbp->sb_versionnum & XFS_SB_VERSION_MOREBITSBIT) &&
>  	     (sbp->sb_features2 & ~XFS_SB_VERSION2_OKBITS)))
>  		return false;
>  
> -	/* versions prior to v4 are not supported */
> -	if (XFS_SB_VERSION_NUM(sbp) < XFS_SB_VERSION_4)
> -		return false;
> -
>  	/* V4 filesystems need v2 directories and unwritten extents */
>  	if (!(sbp->sb_versionnum & XFS_SB_VERSION_DIRV2BIT))
>  		return false;
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] xfs: don't consider future format versions valid

[Christoph Hellwig]

> @@ -86,16 +87,16 @@ xfs_sb_good_version(
>  	if (xfs_sb_is_v5(sbp))
>  		return xfs_sb_validate_v5_features(sbp);
>  
> +	/* versions prior to v4 are not supported */
> +	if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_4)
> +		return false;

The comment is a bit confusing now.  But maybe the v4 checks should
move into a xfs_sb_validate_v4_features helper anyway, which
would lead to a quite nice flow here:

	if (xfs_sb_is_v5(sbp))
	 	return xfs_sb_validate_v5_features(sbp);
	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
	 	return xfs_sb_validate_v4_features(sbp);
	return false;

Re: [PATCH 2/2] xfs: don't consider future format versions valid

[Dave Chinner]

On Wed, Apr 12, 2023 at 05:13:41AM -0700, Christoph Hellwig wrote:
> > @@ -86,16 +87,16 @@ xfs_sb_good_version(
> >  	if (xfs_sb_is_v5(sbp))
> >  		return xfs_sb_validate_v5_features(sbp);
> >  
> > +	/* versions prior to v4 are not supported */
> > +	if (XFS_SB_VERSION_NUM(sbp) != XFS_SB_VERSION_4)
> > +		return false;
> 
> The comment is a bit confusing now.  But maybe the v4 checks should
> move into a xfs_sb_validate_v4_features helper anyway, which
> would lead to a quite nice flow here:
> 
> 	if (xfs_sb_is_v5(sbp))
> 	 	return xfs_sb_validate_v5_features(sbp);
> 	if (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_4)
> 	 	return xfs_sb_validate_v4_features(sbp);
> 	return false;

Sure. Care to send a patch that does that cleanup?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com