[PATCH] xfs: remove xfs_attr_leaf_hasname

[PATCH] xfs: remove xfs_attr_leaf_hasname

[Christoph Hellwig]

The calling convention of xfs_attr_leaf_hasname() is problematic, because
it returns a NULL buffer when xfs_attr3_leaf_read fails, a valid buffer
when xfs_attr3_leaf_lookup_int returns -ENOATTR or -EEXIST, and a
non-NULL buffer pointer for an already released buffer when
xfs_attr3_leaf_lookup_int fails with other error values.

Fix this by simply open coding xfs_attr_leaf_hasname in the callers, so
that the buffer release code is done by each caller of
xfs_attr3_leaf_read.

X-Cc: stable@vger.kernel.org # v5.19+
Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
Reported-by: Mark Tinguely <mark.tinguely@oracle.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr.c | 75 +++++++++++++---------------------------
 1 file changed, 24 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 8c04acd30d48..b88e65c7e45d 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.
@@ -979,11 +978,12 @@ xfs_attr_lookup(
 		return error;
 
 	if (xfs_attr_is_leaf(dp)) {
-		error = xfs_attr_leaf_hasname(args, &bp);
-
-		if (bp)
-			xfs_trans_brelse(args->trans, bp);
-
+		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);
+		xfs_trans_brelse(args->trans, bp);
 		return error;
 	}
 
@@ -1222,27 +1222,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,25 +1232,22 @@ 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) {
+	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 != -EEXIST) {
 		xfs_trans_brelse(args->trans, bp);
-		if (args->op_flags & XFS_DA_OP_RECOVERY)
+		if (error == -ENOATTR && (args->op_flags & XFS_DA_OP_RECOVERY))
 			return 0;
 		return error;
-	} else if (error != -EEXIST)
-		return error;
+	}
 
 	xfs_attr3_leaf_remove(bp, args);
 
@@ -1295,23 +1271,20 @@ 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)  {
-		xfs_trans_brelse(args->trans, bp);
-		return error;
-	} else if (error != -EEXIST)
+	error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, &bp);
+	if (error)
 		return error;
-
-
-	error = xfs_attr3_leaf_getvalue(bp, args);
+	error = xfs_attr3_leaf_lookup_int(bp, args);
+	if (error == -EEXIST)
+		error = xfs_attr3_leaf_getvalue(bp, args);
 	xfs_trans_brelse(args->trans, bp);
 	return error;
 }
-- 
2.47.3

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Darrick J. Wong]

On Fri, Jan 09, 2026 at 04:17:40PM +0100, Christoph Hellwig wrote:
> The calling convention of xfs_attr_leaf_hasname() is problematic, because
> it returns a NULL buffer when xfs_attr3_leaf_read fails, a valid buffer
> when xfs_attr3_leaf_lookup_int returns -ENOATTR or -EEXIST, and a
> non-NULL buffer pointer for an already released buffer when
> xfs_attr3_leaf_lookup_int fails with other error values.
> 
> Fix this by simply open coding xfs_attr_leaf_hasname in the callers, so
> that the buffer release code is done by each caller of
> xfs_attr3_leaf_read.
> 
> X-Cc: stable@vger.kernel.org # v5.19+
> Fixes: 07120f1abdff ("xfs: Add xfs_has_attr and subroutines")
> Reported-by: Mark Tinguely <mark.tinguely@oracle.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Blerrgh, xfs_attr3_leaf_lookup_int encoding results with error numbers
makes my brain hurt every time I look at the xattr code... but this
patch is (afaict) a correct way to avoid the **bp state confusion
presented by xfs_attr_leaf_hasname.

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

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c | 75 +++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 51 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 8c04acd30d48..b88e65c7e45d 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.
> @@ -979,11 +978,12 @@ xfs_attr_lookup(
>  		return error;
>  
>  	if (xfs_attr_is_leaf(dp)) {
> -		error = xfs_attr_leaf_hasname(args, &bp);
> -
> -		if (bp)
> -			xfs_trans_brelse(args->trans, bp);
> -
> +		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);
> +		xfs_trans_brelse(args->trans, bp);
>  		return error;
>  	}
>  
> @@ -1222,27 +1222,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,25 +1232,22 @@ 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) {
> +	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 != -EEXIST) {
>  		xfs_trans_brelse(args->trans, bp);
> -		if (args->op_flags & XFS_DA_OP_RECOVERY)
> +		if (error == -ENOATTR && (args->op_flags & XFS_DA_OP_RECOVERY))
>  			return 0;
>  		return error;
> -	} else if (error != -EEXIST)
> -		return error;
> +	}
>  
>  	xfs_attr3_leaf_remove(bp, args);
>  
> @@ -1295,23 +1271,20 @@ 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)  {
> -		xfs_trans_brelse(args->trans, bp);
> -		return error;
> -	} else if (error != -EEXIST)
> +	error = xfs_attr3_leaf_read(args->trans, args->dp, args->owner, 0, &bp);
> +	if (error)
>  		return error;
> -
> -
> -	error = xfs_attr3_leaf_getvalue(bp, args);
> +	error = xfs_attr3_leaf_lookup_int(bp, args);
> +	if (error == -EEXIST)
> +		error = xfs_attr3_leaf_getvalue(bp, args);
>  	xfs_trans_brelse(args->trans, bp);
>  	return error;
>  }
> -- 
> 2.47.3
> 
> 

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Christoph Hellwig]

On Fri, Jan 09, 2026 at 08:29:11AM -0800, Darrick J. Wong wrote:
> Blerrgh, xfs_attr3_leaf_lookup_int encoding results with error numbers
> makes my brain hurt every time I look at the xattr code...

Same.  I've been really tempted to pass a separate bool multiple times.
Maybe we should finally go for it?  That would also remove most (but
all of the issues) due to the block layer -ENODATA leak.

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Darrick J. Wong]

On Fri, Jan 09, 2026 at 05:37:06PM +0100, Christoph Hellwig wrote:
> On Fri, Jan 09, 2026 at 08:29:11AM -0800, Darrick J. Wong wrote:
> > Blerrgh, xfs_attr3_leaf_lookup_int encoding results with error numbers
> > makes my brain hurt every time I look at the xattr code...
> 
> Same.  I've been really tempted to pass a separate bool multiple times.
> Maybe we should finally go for it?  That would also remove most (but
> all of the issues) due to the block layer -ENODATA leak.

I support that.

--D

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Carlos Maiolino]

On Fri, Jan 09, 2026 at 09:41:27AM -0800, Darrick J. Wong wrote:
> On Fri, Jan 09, 2026 at 05:37:06PM +0100, Christoph Hellwig wrote:
> > On Fri, Jan 09, 2026 at 08:29:11AM -0800, Darrick J. Wong wrote:
> > > Blerrgh, xfs_attr3_leaf_lookup_int encoding results with error numbers
> > > makes my brain hurt every time I look at the xattr code...
> > 
> > Same.  I've been really tempted to pass a separate bool multiple times.
> > Maybe we should finally go for it?  That would also remove most (but
> > all of the issues) due to the block layer -ENODATA leak.
> 
> I support that.

+1.

Should I pick this patch and you send and incremental one or should I
just wait for a new one?

^ hch

> 
> --D
> 

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Christoph Hellwig]

On Tue, Jan 13, 2026 at 10:38:25AM +0100, Carlos Maiolino wrote:
> On Fri, Jan 09, 2026 at 09:41:27AM -0800, Darrick J. Wong wrote:
> > On Fri, Jan 09, 2026 at 05:37:06PM +0100, Christoph Hellwig wrote:
> > > On Fri, Jan 09, 2026 at 08:29:11AM -0800, Darrick J. Wong wrote:
> > > > Blerrgh, xfs_attr3_leaf_lookup_int encoding results with error numbers
> > > > makes my brain hurt every time I look at the xattr code...
> > > 
> > > Same.  I've been really tempted to pass a separate bool multiple times.
> > > Maybe we should finally go for it?  That would also remove most (but
> > > all of the issues) due to the block layer -ENODATA leak.
> > 
> > I support that.
> 
> +1.
> 
> Should I pick this patch and you send and incremental one or should I
> just wait for a new one?

Please pick it up.  Fixing up the attr/dir calling conventions will
be a big project that'll take a while and produce a lot of patches.

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Niklas Cassel]

On Fri, Jan 09, 2026 at 04:17:40PM +0100, Christoph Hellwig wrote:
> The calling convention of xfs_attr_leaf_hasname() is problematic, because
> it returns a NULL buffer when xfs_attr3_leaf_read fails, a valid buffer
> when xfs_attr3_leaf_lookup_int returns -ENOATTR or -EEXIST, and a
> non-NULL buffer pointer for an already released buffer when
> xfs_attr3_leaf_lookup_int fails with other error values.
> 
> Fix this by simply open coding xfs_attr_leaf_hasname in the callers, so
> that the buffer release code is done by each caller of
> xfs_attr3_leaf_read.
> 
> X-Cc: stable@vger.kernel.org # v5.19+

This looks like a typo. (Thus stable is not on cc:)

Probably not enough reson to resend,
(since the stable scripts backport anything with a Fixes-tag anyway),
but perhaps s/X-Cc/Cc/ when applying.


Kind regards,
Niklas

Re: [PATCH] xfs: remove xfs_attr_leaf_hasname

[Carlos Maiolino]

On Fri, 09 Jan 2026 16:17:40 +0100, Christoph Hellwig wrote:
> The calling convention of xfs_attr_leaf_hasname() is problematic, because
> it returns a NULL buffer when xfs_attr3_leaf_read fails, a valid buffer
> when xfs_attr3_leaf_lookup_int returns -ENOATTR or -EEXIST, and a
> non-NULL buffer pointer for an already released buffer when
> xfs_attr3_leaf_lookup_int fails with other error values.
> 
> Fix this by simply open coding xfs_attr_leaf_hasname in the callers, so
> that the buffer release code is done by each caller of
> xfs_attr3_leaf_read.
> 
> [...]

Applied to for-next, thanks!

[1/1] xfs: remove xfs_attr_leaf_hasname
      commit: 3a65ea768b8094e4699e72f9ab420eb9e0f3f568

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