fix XFS_IBULK_* vs XFS_IWALK_* confusion

fix XFS_IBULK_* vs XFS_IWALK_* confusion

[Christoph Hellwig]

Hi all,

this fixes a syzcall triggered assert due to the somewhat sloppy split
between the XFS_IBULK and XFS_IWALK flags.  The first is the minimal
fix for the reported problem, and the second one cleans up the
interface to avoid problems like this in the future.

Diffstat:
 xfs_ioctl.c  |    2 +-
 xfs_itable.c |    8 ++------
 xfs_itable.h |   10 ++++------
 3 files changed, 7 insertions(+), 13 deletions(-)

[PATCH 1/2] xfs: fully decouple XFS_IBULK* flags from XFS_IWALK* flags

[Christoph Hellwig]

Fix up xfs_inumbers to now pass in the XFS_IBULK* flags into the flags
argument to xfs_inobt_walk, which expects the XFS_IWALK* flags.

Currently passing the wrong flags works for non-debug builds because
the only XFS_IWALK* flag has the same encoding as the corresponding
XFS_IBULK* flag, but in debug builds it can trigger an assert that no
incorrect flag is passed.  Instead just extra the relevant flag.

Fixes: 5b35d922c52798 ("xfs: Decouple XFS_IBULK flags from XFS_IWALK flags")
Reported-by: cen zhang <zzzccc427@gmail.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_itable.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index c8c9b8d8309f..5116842420b2 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -447,17 +447,21 @@ xfs_inumbers(
 		.breq		= breq,
 	};
 	struct xfs_trans	*tp;
+	unsigned int		iwalk_flags = 0;
 	int			error = 0;
 
 	if (xfs_bulkstat_already_done(breq->mp, breq->startino))
 		return 0;
 
+	if (breq->flags & XFS_IBULK_SAME_AG)
+		iwalk_flags |= XFS_IWALK_SAME_AG;
+
 	/*
 	 * Grab an empty transaction so that we can use its recursive buffer
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
 	tp = xfs_trans_alloc_empty(breq->mp);
-	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->flags,
+	error = xfs_inobt_walk(breq->mp, tp, breq->startino, iwalk_flags,
 			xfs_inumbers_walk, breq->icount, &ic);
 	xfs_trans_cancel(tp);
 
-- 
2.47.2

[PATCH 2/2] xfs: remove XFS_IBULK_SAME_AG

[Christoph Hellwig]

Add a new field to struct xfs_ibulk to directly pass XFS_IWALK* flags,
and thus remove the need to indirect the SAME_AG flag through
XFS_IBULK*.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c  |  2 +-
 fs/xfs/xfs_itable.c | 12 ++----------
 fs/xfs/xfs_itable.h | 10 ++++------
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index fe1f74a3b6a3..e1051a530a50 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -219,7 +219,7 @@ xfs_bulk_ireq_setup(
 		else if (XFS_INO_TO_AGNO(mp, breq->startino) < hdr->agno)
 			return -EINVAL;
 
-		breq->flags |= XFS_IBULK_SAME_AG;
+		breq->iwalk_flags |= XFS_IWALK_SAME_AG;
 
 		/* Asking for an inode past the end of the AG?  We're done! */
 		if (XFS_INO_TO_AGNO(mp, breq->startino) > hdr->agno)
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 5116842420b2..2aa37a4d2706 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -307,7 +307,6 @@ xfs_bulkstat(
 		.breq		= breq,
 	};
 	struct xfs_trans	*tp;
-	unsigned int		iwalk_flags = 0;
 	int			error;
 
 	if (breq->idmap != &nop_mnt_idmap) {
@@ -328,10 +327,7 @@ xfs_bulkstat(
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
 	tp = xfs_trans_alloc_empty(breq->mp);
-	if (breq->flags & XFS_IBULK_SAME_AG)
-		iwalk_flags |= XFS_IWALK_SAME_AG;
-
-	error = xfs_iwalk(breq->mp, tp, breq->startino, iwalk_flags,
+	error = xfs_iwalk(breq->mp, tp, breq->startino, breq->iwalk_flags,
 			xfs_bulkstat_iwalk, breq->icount, &bc);
 	xfs_trans_cancel(tp);
 	kfree(bc.buf);
@@ -447,21 +443,17 @@ xfs_inumbers(
 		.breq		= breq,
 	};
 	struct xfs_trans	*tp;
-	unsigned int		iwalk_flags = 0;
 	int			error = 0;
 
 	if (xfs_bulkstat_already_done(breq->mp, breq->startino))
 		return 0;
 
-	if (breq->flags & XFS_IBULK_SAME_AG)
-		iwalk_flags |= XFS_IWALK_SAME_AG;
-
 	/*
 	 * Grab an empty transaction so that we can use its recursive buffer
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
 	tp = xfs_trans_alloc_empty(breq->mp);
-	error = xfs_inobt_walk(breq->mp, tp, breq->startino, iwalk_flags,
+	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->iwalk_flags,
 			xfs_inumbers_walk, breq->icount, &ic);
 	xfs_trans_cancel(tp);
 
diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
index f10e8f8f2335..2d0612f14d6e 100644
--- a/fs/xfs/xfs_itable.h
+++ b/fs/xfs/xfs_itable.h
@@ -13,17 +13,15 @@ struct xfs_ibulk {
 	xfs_ino_t		startino; /* start with this inode */
 	unsigned int		icount;   /* number of elements in ubuffer */
 	unsigned int		ocount;   /* number of records returned */
-	unsigned int		flags;    /* see XFS_IBULK_FLAG_* */
+	unsigned int		flags;    /* XFS_IBULK_FLAG_* */
+	unsigned int		iwalk_flags; /* XFS_IWALK_FLAG_* */
 };
 
-/* Only iterate within the same AG as startino */
-#define XFS_IBULK_SAME_AG	(1U << 0)
-
 /* Fill out the bs_extents64 field if set. */
-#define XFS_IBULK_NREXT64	(1U << 1)
+#define XFS_IBULK_NREXT64	(1U << 0)
 
 /* Signal that we can return metadata directories. */
-#define XFS_IBULK_METADIR	(1U << 2)
+#define XFS_IBULK_METADIR	(1U << 1)
 
 /*
  * Advance the user buffer pointer by one record of the given size.  If the
-- 
2.47.2

Re: [PATCH 1/2] xfs: fully decouple XFS_IBULK* flags from XFS_IWALK* flags

[Darrick J. Wong]

On Wed, Jul 23, 2025 at 02:19:44PM +0200, Christoph Hellwig wrote:
> Fix up xfs_inumbers to now pass in the XFS_IBULK* flags into the flags
> argument to xfs_inobt_walk, which expects the XFS_IWALK* flags.
> 
> Currently passing the wrong flags works for non-debug builds because
> the only XFS_IWALK* flag has the same encoding as the corresponding
> XFS_IBULK* flag, but in debug builds it can trigger an assert that no
> incorrect flag is passed.  Instead just extra the relevant flag.
> 
> Fixes: 5b35d922c52798 ("xfs: Decouple XFS_IBULK flags from XFS_IWALK flags")
> Reported-by: cen zhang <zzzccc427@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I'd prefer this come with the
Cc: <stable@vger.kernel.org> # v5.19
so that I don't have to manually backport this to 6.12

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

--D

> ---
>  fs/xfs/xfs_itable.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index c8c9b8d8309f..5116842420b2 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -447,17 +447,21 @@ xfs_inumbers(
>  		.breq		= breq,
>  	};
>  	struct xfs_trans	*tp;
> +	unsigned int		iwalk_flags = 0;
>  	int			error = 0;
>  
>  	if (xfs_bulkstat_already_done(breq->mp, breq->startino))
>  		return 0;
>  
> +	if (breq->flags & XFS_IBULK_SAME_AG)
> +		iwalk_flags |= XFS_IWALK_SAME_AG;
> +
>  	/*
>  	 * Grab an empty transaction so that we can use its recursive buffer
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
>  	tp = xfs_trans_alloc_empty(breq->mp);
> -	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->flags,
> +	error = xfs_inobt_walk(breq->mp, tp, breq->startino, iwalk_flags,
>  			xfs_inumbers_walk, breq->icount, &ic);
>  	xfs_trans_cancel(tp);
>  
> -- 
> 2.47.2
> 
> 

Re: [PATCH 2/2] xfs: remove XFS_IBULK_SAME_AG

[Darrick J. Wong]

On Wed, Jul 23, 2025 at 02:19:45PM +0200, Christoph Hellwig wrote:
> Add a new field to struct xfs_ibulk to directly pass XFS_IWALK* flags,
> and thus remove the need to indirect the SAME_AG flag through
> XFS_IBULK*.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yeah, that clears things up :)
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_ioctl.c  |  2 +-
>  fs/xfs/xfs_itable.c | 12 ++----------
>  fs/xfs/xfs_itable.h | 10 ++++------
>  3 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index fe1f74a3b6a3..e1051a530a50 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -219,7 +219,7 @@ xfs_bulk_ireq_setup(
>  		else if (XFS_INO_TO_AGNO(mp, breq->startino) < hdr->agno)
>  			return -EINVAL;
>  
> -		breq->flags |= XFS_IBULK_SAME_AG;
> +		breq->iwalk_flags |= XFS_IWALK_SAME_AG;
>  
>  		/* Asking for an inode past the end of the AG?  We're done! */
>  		if (XFS_INO_TO_AGNO(mp, breq->startino) > hdr->agno)
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 5116842420b2..2aa37a4d2706 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -307,7 +307,6 @@ xfs_bulkstat(
>  		.breq		= breq,
>  	};
>  	struct xfs_trans	*tp;
> -	unsigned int		iwalk_flags = 0;
>  	int			error;
>  
>  	if (breq->idmap != &nop_mnt_idmap) {
> @@ -328,10 +327,7 @@ xfs_bulkstat(
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
>  	tp = xfs_trans_alloc_empty(breq->mp);
> -	if (breq->flags & XFS_IBULK_SAME_AG)
> -		iwalk_flags |= XFS_IWALK_SAME_AG;
> -
> -	error = xfs_iwalk(breq->mp, tp, breq->startino, iwalk_flags,
> +	error = xfs_iwalk(breq->mp, tp, breq->startino, breq->iwalk_flags,
>  			xfs_bulkstat_iwalk, breq->icount, &bc);
>  	xfs_trans_cancel(tp);
>  	kfree(bc.buf);
> @@ -447,21 +443,17 @@ xfs_inumbers(
>  		.breq		= breq,
>  	};
>  	struct xfs_trans	*tp;
> -	unsigned int		iwalk_flags = 0;
>  	int			error = 0;
>  
>  	if (xfs_bulkstat_already_done(breq->mp, breq->startino))
>  		return 0;
>  
> -	if (breq->flags & XFS_IBULK_SAME_AG)
> -		iwalk_flags |= XFS_IWALK_SAME_AG;
> -
>  	/*
>  	 * Grab an empty transaction so that we can use its recursive buffer
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
>  	tp = xfs_trans_alloc_empty(breq->mp);
> -	error = xfs_inobt_walk(breq->mp, tp, breq->startino, iwalk_flags,
> +	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->iwalk_flags,
>  			xfs_inumbers_walk, breq->icount, &ic);
>  	xfs_trans_cancel(tp);
>  
> diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> index f10e8f8f2335..2d0612f14d6e 100644
> --- a/fs/xfs/xfs_itable.h
> +++ b/fs/xfs/xfs_itable.h
> @@ -13,17 +13,15 @@ struct xfs_ibulk {
>  	xfs_ino_t		startino; /* start with this inode */
>  	unsigned int		icount;   /* number of elements in ubuffer */
>  	unsigned int		ocount;   /* number of records returned */
> -	unsigned int		flags;    /* see XFS_IBULK_FLAG_* */
> +	unsigned int		flags;    /* XFS_IBULK_FLAG_* */
> +	unsigned int		iwalk_flags; /* XFS_IWALK_FLAG_* */
>  };
>  
> -/* Only iterate within the same AG as startino */
> -#define XFS_IBULK_SAME_AG	(1U << 0)
> -
>  /* Fill out the bs_extents64 field if set. */
> -#define XFS_IBULK_NREXT64	(1U << 1)
> +#define XFS_IBULK_NREXT64	(1U << 0)
>  
>  /* Signal that we can return metadata directories. */
> -#define XFS_IBULK_METADIR	(1U << 2)
> +#define XFS_IBULK_METADIR	(1U << 1)
>  
>  /*
>   * Advance the user buffer pointer by one record of the given size.  If the
> -- 
> 2.47.2
> 
> 

Re: [PATCH 1/2] xfs: fully decouple XFS_IBULK* flags from XFS_IWALK* flags

[Carlos Maiolino]

On Wed, Jul 23, 2025 at 09:20:47AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 23, 2025 at 02:19:44PM +0200, Christoph Hellwig wrote:
> > Fix up xfs_inumbers to now pass in the XFS_IBULK* flags into the flags
> > argument to xfs_inobt_walk, which expects the XFS_IWALK* flags.
> >
> > Currently passing the wrong flags works for non-debug builds because
> > the only XFS_IWALK* flag has the same encoding as the corresponding
> > XFS_IBULK* flag, but in debug builds it can trigger an assert that no
> > incorrect flag is passed.  Instead just extra the relevant flag.
> >
> > Fixes: 5b35d922c52798 ("xfs: Decouple XFS_IBULK flags from XFS_IWALK flags")
> > Reported-by: cen zhang <zzzccc427@gmail.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> I'd prefer this come with the
> Cc: <stable@vger.kernel.org> # v5.19
> so that I don't have to manually backport this to 6.12

Done.


> 
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 
> --D
> 
> > ---
> >  fs/xfs/xfs_itable.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index c8c9b8d8309f..5116842420b2 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -447,17 +447,21 @@ xfs_inumbers(
> >  		.breq		= breq,
> >  	};
> >  	struct xfs_trans	*tp;
> > +	unsigned int		iwalk_flags = 0;
> >  	int			error = 0;
> >
> >  	if (xfs_bulkstat_already_done(breq->mp, breq->startino))
> >  		return 0;
> >
> > +	if (breq->flags & XFS_IBULK_SAME_AG)
> > +		iwalk_flags |= XFS_IWALK_SAME_AG;
> > +
> >  	/*
> >  	 * Grab an empty transaction so that we can use its recursive buffer
> >  	 * locking abilities to detect cycles in the inobt without deadlocking.
> >  	 */
> >  	tp = xfs_trans_alloc_empty(breq->mp);
> > -	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->flags,
> > +	error = xfs_inobt_walk(breq->mp, tp, breq->startino, iwalk_flags,
> >  			xfs_inumbers_walk, breq->icount, &ic);
> >  	xfs_trans_cancel(tp);
> >
> > --
> > 2.47.2
> >
> >
> 

Re: fix XFS_IBULK_* vs XFS_IWALK_* confusion

[Carlos Maiolino]

On Wed, 23 Jul 2025 14:19:43 +0200, Christoph Hellwig wrote:
> this fixes a syzcall triggered assert due to the somewhat sloppy split
> between the XFS_IBULK and XFS_IWALK flags.  The first is the minimal
> fix for the reported problem, and the second one cleans up the
> interface to avoid problems like this in the future.
> 
> Diffstat:
>  xfs_ioctl.c  |    2 +-
>  xfs_itable.c |    8 ++------
>  xfs_itable.h |   10 ++++------
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> [...]

Applied to for-next, thanks!

[1/2] xfs: fully decouple XFS_IBULK* flags from XFS_IWALK* flags
      commit: d2845519b0723c5d5a0266cbf410495f9b8fd65c
[2/2] xfs: remove XFS_IBULK_SAME_AG
      commit: 82efde9cf2e4ce25eac96a20e36eae7c338df1e0

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>