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

[Christoph Hellwig <hch@infradead.org>]

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;
 }