[PATCH] xfs: fix NULL ptr in xfs_attr_leaf_get

[PATCH] xfs: fix NULL ptr in xfs_attr_leaf_get

[Mark Tinguely]

The error path of xfs_attr_leaf_hasname() can leave a NULL
xfs_buf pointer. xfs_has_attr() checks for the NULL pointer but
the other callers do not.

We tripped over the NULL pointer in xfs_attr_leaf_get() but fix
the other callers too.

Fixes v5.8-rc4-95-g07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
No reproducer.

Cc: stable@vger.kernel.org # v5.19+ with another port for v5.9 - v5.18
Signed-off-by: Mark Tinguely <mark.tinguely@oracle.com>
---
  fs/xfs/libxfs/xfs_attr.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 8c04acd30d48..25e2ecf20d14 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1266,7 +1266,8 @@ xfs_attr_leaf_removename(
  
  	error = xfs_attr_leaf_hasname(args, &bp);
  	if (error == -ENOATTR) {
-		xfs_trans_brelse(args->trans, bp);
+		if (bp)
+			xfs_trans_brelse(args->trans, bp);
  		if (args->op_flags & XFS_DA_OP_RECOVERY)
  			return 0;
  		return error;
@@ -1305,7 +1306,8 @@ xfs_attr_leaf_get(xfs_da_args_t *args)
  	error = xfs_attr_leaf_hasname(args, &bp);
  
  	if (error == -ENOATTR)  {
-		xfs_trans_brelse(args->trans, bp);
+		if (bp)
+			xfs_trans_brelse(args->trans, bp);
  		return error;
  	} else if (error != -EEXIST)
  		return error;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] xfs: fix NULL ptr in xfs_attr_leaf_get

[Christoph Hellwig]

On Tue, Dec 30, 2025 at 01:02:41PM -0600, Mark Tinguely wrote:
> 
> The error path of xfs_attr_leaf_hasname() can leave a NULL
> xfs_buf pointer. xfs_has_attr() checks for the NULL pointer but
> the other callers do not.
> 
> We tripped over the NULL pointer in xfs_attr_leaf_get() but fix
> the other callers too.
> 
> Fixes v5.8-rc4-95-g07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> No reproducer.

Eww, what a mess.  I think we're better off to always leave releasing
bp to the caller.  Something like the patch below.  Only compile tested
for now, but I'll kick off an xfstests run.

Or maybe we might just kill off xfs_attr_leaf_hasname entirely and open
code it in the three callers, which might end up being more readable?

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 8c04acd30d48..c5259641dd97 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -50,7 +50,6 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
  */
 STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
 STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
-STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
 
 /*
  * Internal routines when attribute list is more than one block.
@@ -951,6 +950,22 @@ xfs_attr_set_iter(
 	return error;
 }
 
+/*
+ * Return EEXIST if attr is found, or ENOATTR if not.
+ * Caller must relese @bp on error if non-NULL.
+ */
+static int
+xfs_attr_leaf_hasname(
+	struct xfs_da_args	*args,
+	struct xfs_buf		**bp)
+{
+	int                     error;
+
+	error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, bp);
+	if (error)
+		return error;
+	return xfs_attr3_leaf_lookup_int(*bp, args);
+}
 
 /*
  * Return EEXIST if attr is found, or ENOATTR if not
@@ -980,10 +995,8 @@ xfs_attr_lookup(
 
 	if (xfs_attr_is_leaf(dp)) {
 		error = xfs_attr_leaf_hasname(args, &bp);
-
 		if (bp)
 			xfs_trans_brelse(args->trans, bp);
-
 		return error;
 	}
 
@@ -1222,27 +1235,6 @@ xfs_attr_shortform_addname(
  * External routines when attribute list is one block
  *========================================================================*/
 
-/*
- * Return EEXIST if attr is found, or ENOATTR if not
- */
-STATIC int
-xfs_attr_leaf_hasname(
-	struct xfs_da_args	*args,
-	struct xfs_buf		**bp)
-{
-	int                     error = 0;
-
-	error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, bp);
-	if (error)
-		return error;
-
-	error = xfs_attr3_leaf_lookup_int(*bp, args);
-	if (error != -ENOATTR && error != -EEXIST)
-		xfs_trans_brelse(args->trans, *bp);
-
-	return error;
-}
-
 /*
  * Remove a name from the leaf attribute list structure
  *
@@ -1253,26 +1245,24 @@ STATIC int
 xfs_attr_leaf_removename(
 	struct xfs_da_args	*args)
 {
-	struct xfs_inode	*dp;
-	struct xfs_buf		*bp;
+	struct xfs_inode	*dp = args->dp;
 	int			error, forkoff;
+	struct xfs_buf		*bp;
 
 	trace_xfs_attr_leaf_removename(args);
 
-	/*
-	 * Remove the attribute.
-	 */
-	dp = args->dp;
-
 	error = xfs_attr_leaf_hasname(args, &bp);
-	if (error == -ENOATTR) {
-		xfs_trans_brelse(args->trans, bp);
-		if (args->op_flags & XFS_DA_OP_RECOVERY)
+	if (error != -EEXIST) {
+		if (bp)
+			xfs_trans_brelse(args->trans, bp);
+		if (error == -ENOATTR && (args->op_flags & XFS_DA_OP_RECOVERY))
 			return 0;
 		return error;
-	} else if (error != -EEXIST)
-		return error;
+	}
 
+	/*
+	 * Remove the attribute.
+	 */
 	xfs_attr3_leaf_remove(bp, args);
 
 	/*
@@ -1281,8 +1271,8 @@ xfs_attr_leaf_removename(
 	forkoff = xfs_attr_shortform_allfit(bp, dp);
 	if (forkoff)
 		return xfs_attr3_leaf_to_shortform(bp, args, forkoff);
-		/* bp is gone due to xfs_da_shrink_inode */
 
+	/* bp is gone due to xfs_da_shrink_inode */
 	return 0;
 }
 
@@ -1295,24 +1285,19 @@ xfs_attr_leaf_removename(
  * Returns 0 on successful retrieval, otherwise an error.
  */
 STATIC int
-xfs_attr_leaf_get(xfs_da_args_t *args)
+xfs_attr_leaf_get(
+	struct xfs_da_args	*args)
 {
-	struct xfs_buf *bp;
-	int error;
+	struct xfs_buf		*bp;
+	int			error;
 
 	trace_xfs_attr_leaf_get(args);
 
 	error = xfs_attr_leaf_hasname(args, &bp);
-
-	if (error == -ENOATTR)  {
+	if (error == -EEXIST)
+		error = xfs_attr3_leaf_getvalue(bp, args);
+	if (bp)
 		xfs_trans_brelse(args->trans, bp);
-		return error;
-	} else if (error != -EEXIST)
-		return error;
-
-
-	error = xfs_attr3_leaf_getvalue(bp, args);
-	xfs_trans_brelse(args->trans, bp);
 	return error;
 }
 

Re: [PATCH] xfs: fix NULL ptr in xfs_attr_leaf_get

[Darrick J. Wong]

On Tue, Jan 06, 2026 at 12:09:25AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 30, 2025 at 01:02:41PM -0600, Mark Tinguely wrote:
> > 
> > The error path of xfs_attr_leaf_hasname() can leave a NULL
> > xfs_buf pointer. xfs_has_attr() checks for the NULL pointer but
> > the other callers do not.
> > 
> > We tripped over the NULL pointer in xfs_attr_leaf_get() but fix
> > the other callers too.
> > 
> > Fixes v5.8-rc4-95-g07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> > No reproducer.
> 
> Eww, what a mess.  I think we're better off to always leave releasing
> bp to the caller.  Something like the patch below.  Only compile tested
> for now, but I'll kick off an xfstests run.
> 
> Or maybe we might just kill off xfs_attr_leaf_hasname entirely and open
> code it in the three callers, which might end up being more readable?

...unless this is yet another case of the block layer returning ENODATA,
which is then mistaken for returning ENOATTR-but-here's-your-buffer by
the xfs_attr code?

--D

> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 8c04acd30d48..c5259641dd97 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -50,7 +50,6 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
>   */
>  STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
>  STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
> -STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
>  
>  /*
>   * Internal routines when attribute list is more than one block.
> @@ -951,6 +950,22 @@ xfs_attr_set_iter(
>  	return error;
>  }
>  
> +/*
> + * Return EEXIST if attr is found, or ENOATTR if not.
> + * Caller must relese @bp on error if non-NULL.
> + */
> +static int
> +xfs_attr_leaf_hasname(
> +	struct xfs_da_args	*args,
> +	struct xfs_buf		**bp)
> +{
> +	int                     error;
> +
> +	error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, bp);
> +	if (error)
> +		return error;
> +	return xfs_attr3_leaf_lookup_int(*bp, args);
> +}
>  
>  /*
>   * Return EEXIST if attr is found, or ENOATTR if not
> @@ -980,10 +995,8 @@ xfs_attr_lookup(
>  
>  	if (xfs_attr_is_leaf(dp)) {
>  		error = xfs_attr_leaf_hasname(args, &bp);
> -
>  		if (bp)
>  			xfs_trans_brelse(args->trans, bp);
> -
>  		return error;
>  	}
>  
> @@ -1222,27 +1235,6 @@ xfs_attr_shortform_addname(
>   * External routines when attribute list is one block
>   *========================================================================*/
>  
> -/*
> - * Return EEXIST if attr is found, or ENOATTR if not
> - */
> -STATIC int
> -xfs_attr_leaf_hasname(
> -	struct xfs_da_args	*args,
> -	struct xfs_buf		**bp)
> -{
> -	int                     error = 0;
> -
> -	error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, bp);
> -	if (error)
> -		return error;
> -
> -	error = xfs_attr3_leaf_lookup_int(*bp, args);
> -	if (error != -ENOATTR && error != -EEXIST)
> -		xfs_trans_brelse(args->trans, *bp);
> -
> -	return error;
> -}
> -
>  /*
>   * Remove a name from the leaf attribute list structure
>   *
> @@ -1253,26 +1245,24 @@ STATIC int
>  xfs_attr_leaf_removename(
>  	struct xfs_da_args	*args)
>  {
> -	struct xfs_inode	*dp;
> -	struct xfs_buf		*bp;
> +	struct xfs_inode	*dp = args->dp;
>  	int			error, forkoff;
> +	struct xfs_buf		*bp;
>  
>  	trace_xfs_attr_leaf_removename(args);
>  
> -	/*
> -	 * Remove the attribute.
> -	 */
> -	dp = args->dp;
> -
>  	error = xfs_attr_leaf_hasname(args, &bp);
> -	if (error == -ENOATTR) {
> -		xfs_trans_brelse(args->trans, bp);
> -		if (args->op_flags & XFS_DA_OP_RECOVERY)
> +	if (error != -EEXIST) {
> +		if (bp)
> +			xfs_trans_brelse(args->trans, bp);
> +		if (error == -ENOATTR && (args->op_flags & XFS_DA_OP_RECOVERY))
>  			return 0;
>  		return error;
> -	} else if (error != -EEXIST)
> -		return error;
> +	}
>  
> +	/*
> +	 * Remove the attribute.
> +	 */
>  	xfs_attr3_leaf_remove(bp, args);
>  
>  	/*
> @@ -1281,8 +1271,8 @@ xfs_attr_leaf_removename(
>  	forkoff = xfs_attr_shortform_allfit(bp, dp);
>  	if (forkoff)
>  		return xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> -		/* bp is gone due to xfs_da_shrink_inode */
>  
> +	/* bp is gone due to xfs_da_shrink_inode */
>  	return 0;
>  }
>  
> @@ -1295,24 +1285,19 @@ xfs_attr_leaf_removename(
>   * Returns 0 on successful retrieval, otherwise an error.
>   */
>  STATIC int
> -xfs_attr_leaf_get(xfs_da_args_t *args)
> +xfs_attr_leaf_get(
> +	struct xfs_da_args	*args)
>  {
> -	struct xfs_buf *bp;
> -	int error;
> +	struct xfs_buf		*bp;
> +	int			error;
>  
>  	trace_xfs_attr_leaf_get(args);
>  
>  	error = xfs_attr_leaf_hasname(args, &bp);
> -
> -	if (error == -ENOATTR)  {
> +	if (error == -EEXIST)
> +		error = xfs_attr3_leaf_getvalue(bp, args);
> +	if (bp)
>  		xfs_trans_brelse(args->trans, bp);
> -		return error;
> -	} else if (error != -EEXIST)
> -		return error;
> -
> -
> -	error = xfs_attr3_leaf_getvalue(bp, args);
> -	xfs_trans_brelse(args->trans, bp);
>  	return error;
>  }
>  
> 

Re: [PATCH] xfs: fix NULL ptr in xfs_attr_leaf_get

[Christoph Hellwig]

On Tue, Jan 06, 2026 at 07:40:26AM -0800, Darrick J. Wong wrote:
> > > Fixes v5.8-rc4-95-g07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> > > No reproducer.
> > 
> > Eww, what a mess.  I think we're better off to always leave releasing
> > bp to the caller.  Something like the patch below.  Only compile tested
> > for now, but I'll kick off an xfstests run.
> > 
> > Or maybe we might just kill off xfs_attr_leaf_hasname entirely and open
> > code it in the three callers, which might end up being more readable?
> 
> ...unless this is yet another case of the block layer returning ENODATA,
> which is then mistaken for returning ENOATTR-but-here's-your-buffer by
> the xfs_attr code?

Not really and unless.  This patch (and the full removal that I've
prepared in the meantime) still fix the missing buffer release in that
case and make the code easier to follow.  But yes, they would not fix
the underlying issue, which is why I still think we should not blindly
propagate block layer errors into b_error.

Re: [PATCH] xfs: fix NULL ptr in xfs_attr_leaf_get

[Christoph Hellwig]

On Tue, Jan 06, 2026 at 10:01:01PM -0800, Christoph Hellwig wrote:
> On Tue, Jan 06, 2026 at 07:40:26AM -0800, Darrick J. Wong wrote:
> > > > Fixes v5.8-rc4-95-g07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> > > > No reproducer.
> > > 
> > > Eww, what a mess.  I think we're better off to always leave releasing
> > > bp to the caller.  Something like the patch below.  Only compile tested
> > > for now, but I'll kick off an xfstests run.
> > > 
> > > Or maybe we might just kill off xfs_attr_leaf_hasname entirely and open
> > > code it in the three callers, which might end up being more readable?
> > 
> > ...unless this is yet another case of the block layer returning ENODATA,
> > which is then mistaken for returning ENOATTR-but-here's-your-buffer by
> > the xfs_attr code?
> 
> Not really and unless.  This patch (and the full removal that I've
> prepared in the meantime) still fix the missing buffer release in that
> case and make the code easier to follow.  But yes, they would not fix
> the underlying issue, which is why I still think we should not blindly
> propagate block layer errors into b_error.

Having looked at this a bit more, there are clear issued with the calling
convention of xfs_attr_leaf_hasname, which are fixed by removing it as 
done in the patch I just sent.

But this is very clearly another case where ENODATA/ENOATTR can leak from
the buffer cache.  So just as I said back then I think we need to stop
leaking the block status code through the buffer cache.