[PATCH 1/5] xfs: initialise error in xfs_defer_finish_one()

[PATCH 1/5] xfs: initialise error in xfs_defer_finish_one()

[Javier Tia]

xfs_defer_finish_one() declares error without an initialiser and only
assigns it inside the loop over dfp->dfp_work.  When that list is empty
the loop body never runs, control falls through to the "Done with the
dfp, free it" path, and the function returns an indeterminate value.

An item-less pending item is not hypothetical.  Of the three
xfs_defer_alloc() callers, xfs_defer_add() always follows with
xfs_defer_add_item(), but the other two do not.
xfs_defer_start_recovery() is harmless because it adds to a
caller-supplied r_dfops list rather than to tp->t_dfops, so its items
never enter this path at all, and they are driven by
xfs_defer_finish_recovery() and ops->recover_work() rather than by
xfs_defer_finish_one().  xfs_defer_add_barrier() is neither:
xfs_defer_create_intents() walks tp->t_dfops without filtering
item-less entries, so a barrier is spliced onto the pending list and is
eligible to be picked by xfs_defer_finish_noroll().
xfs_reap_ag_blocks() adds one every other extent, so online repair
reaches this on any filesystem built with CONFIG_XFS_ONLINE_REPAIR.

The consequence is a filesystem shutdown that depends on stack
contents.  xfs_defer_finish_noroll() treats any non--EAGAIN return as
fatal and calls xfs_force_shutdown(SHUTDOWN_CORRUPT_INCORE), so
whenever the uninitialised value happens to be non-zero a successful
barrier is reported as in-core corruption and the filesystem is taken
down in the middle of a repair.  ops->finish_cleanup() also receives
the same value where an op type provides one, though no op type that
can reach the empty-list path defines one.

Returning zero is the correct result rather than a papered-over error,
and not only because the barrier type deliberately has no work items:
reaching the free path at all means the item loop drained without a
non-zero error, so zero is the truthful value for any op type.

The uninitialised declaration is older than the Fixes: commit below,
but that commit is where the bug became reachable - it added
xfs_defer_add_barrier(), the barrier op type and the only caller of it
in one go, and before it no item-less pending item could exist.

Fixes: 3f3cec031099 ("xfs: force small EFIs for reaping btree extents")
Cc: <stable@vger.kernel.org>
Signed-off-by: Javier Tia <floss@jetm.me>
---
 fs/xfs/libxfs/xfs_defer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 89501e8bd2f8..843c33304441 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -583,7 +583,7 @@ xfs_defer_finish_one(
 	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
 	struct xfs_btree_cur		*state = NULL;
 	struct list_head		*li, *n;
-	int				error;
+	int				error = 0;
 
 	trace_xfs_defer_pending_finish(tp->t_mountp, dfp);
 
-- 
Javier Tia

Re: [PATCH 1/5] xfs: initialise error in xfs_defer_finish_one()

[Darrick J. Wong]

On Sat, Aug 08, 2026 at 05:40:18PM -0600, Javier Tia wrote:
> xfs_defer_finish_one() declares error without an initialiser and only
> assigns it inside the loop over dfp->dfp_work.  When that list is empty
> the loop body never runs, control falls through to the "Done with the
> dfp, free it" path, and the function returns an indeterminate value.

You could have stopped here with the commit mess.age

> An item-less pending item is not hypothetical.  Of the three
> xfs_defer_alloc() callers, xfs_defer_add() always follows with
> xfs_defer_add_item(), but the other two do not.
> xfs_defer_start_recovery() is harmless because it adds to a
> caller-supplied r_dfops list rather than to tp->t_dfops, so its items
> never enter this path at all, and they are driven by
> xfs_defer_finish_recovery() and ops->recover_work() rather than by
> xfs_defer_finish_one().  xfs_defer_add_barrier() is neither:
> xfs_defer_create_intents() walks tp->t_dfops without filtering
> item-less entries, so a barrier is spliced onto the pending list and is
> eligible to be picked by xfs_defer_finish_noroll().
> xfs_reap_ag_blocks() adds one every other extent, so online repair
> reaches this on any filesystem built with CONFIG_XFS_ONLINE_REPAIR.
> 
> The consequence is a filesystem shutdown that depends on stack
> contents.  xfs_defer_finish_noroll() treats any non--EAGAIN return as
> fatal and calls xfs_force_shutdown(SHUTDOWN_CORRUPT_INCORE), so
> whenever the uninitialised value happens to be non-zero a successful
> barrier is reported as in-core corruption and the filesystem is taken
> down in the middle of a repair.  ops->finish_cleanup() also receives
> the same value where an op type provides one, though no op type that
> can reach the empty-list path defines one.
> 
> Returning zero is the correct result rather than a papered-over error,
> and not only because the barrier type deliberately has no work items:
> reaching the free path at all means the item loop drained without a
> non-zero error, so zero is the truthful value for any op type.
> 
> The uninitialised declaration is older than the Fixes: commit below,
> but that commit is where the bug became reachable - it added
> xfs_defer_add_barrier(), the barrier op type and the only caller of it
> in one go, and before it no item-less pending item could exist.
> 
> Fixes: 3f3cec031099 ("xfs: force small EFIs for reaping btree extents")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Javier Tia <floss@jetm.me>
> ---
>  fs/xfs/libxfs/xfs_defer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 89501e8bd2f8..843c33304441 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -583,7 +583,7 @@ xfs_defer_finish_one(
>  	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
>  	struct xfs_btree_cur		*state = NULL;
>  	struct list_head		*li, *n;
> -	int				error;
> +	int				error = 0;

We should really just turn on automatic zeroing of automatic variable.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  
>  	trace_xfs_defer_pending_finish(tp->t_mountp, dfp);
>  
> -- 
> Javier Tia
> 

[PATCH v2 1/6] xfs: initialise error in xfs_defer_finish_one()

[Javier Tia]

xfs_defer_finish_one() declares error without an initialiser and only
assigns it inside the loop over dfp->dfp_work.  When that list is empty
the loop body never runs, control falls through to the "Done with the
dfp, free it" path, and the function returns an indeterminate value.

An item-less pending item reaches this through xfs_defer_add_barrier(),
which xfs_reap_ag_blocks() uses on any CONFIG_XFS_ONLINE_REPAIR kernel.
xfs_defer_finish_noroll() treats any non-EAGAIN return as fatal, so a
non-zero stack value turns a successful barrier into a
SHUTDOWN_CORRUPT_INCORE in the middle of a repair.  Zero is the correct
result: reaching the free path means the item loop drained without a
non-zero error.

Fixes: 3f3cec031099 ("xfs: force small EFIs for reaping btree extents")
Cc: <stable@vger.kernel.org>
Signed-off-by: Javier Tia <floss@jetm.me>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_defer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 89501e8bd2f8..843c33304441 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -583,7 +583,7 @@ xfs_defer_finish_one(
 	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
 	struct xfs_btree_cur		*state = NULL;
 	struct list_head		*li, *n;
-	int				error;
+	int				error = 0;
 
 	trace_xfs_defer_pending_finish(tp->t_mountp, dfp);
 
-- 
Javier Tia

[PATCH v2 5/6] xfs: initialise args->total for parent pointer updates

[Javier Tia]

xfs_parent_da_args_init() builds an xfs_da_args from a zeroed
xfs_parent_args (kmem_cache_zalloc), leaving args->total == 0.
xfs_da_grow_inode_int() treats that field as a running block reservation
and subtracts from it; because it is an xfs_extlen_t (uint32_t), the
first attr-fork growth wraps it to ~0U.  That defeats the free-space
check in xfs_alloc_space_available(), and when it coincides with an AG
that has exactly zero available blocks the allocation is clamped to
maxlen 0 and returns -ENOSPC, which xfs_defer_finish_noroll() escalates
to a filesystem shutdown.

Set args->total the way the log recovery path does
(xfs_attri_recover_work(), xfs_attr_item.c:706), in the add and replace
paths that can grow the fork.  Removals and lookups never grow it, so
they leave the field alone, matching that switch.

Fixes: b7c62d90c12c ("xfs: parent pointer attribute creation")
Cc: <stable@vger.kernel.org> # v6.10
Signed-off-by: Javier Tia <floss@jetm.me>
---
 fs/xfs/libxfs/xfs_parent.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_parent.c b/fs/xfs/libxfs/xfs_parent.c
index 3509cc4b2175..312f33086d53 100644
--- a/fs/xfs/libxfs/xfs_parent.c
+++ b/fs/xfs/libxfs/xfs_parent.c
@@ -194,7 +194,7 @@ xfs_parent_addname(
 	const struct xfs_name	*parent_name,
 	struct xfs_inode	*child)
 {
-	int			error;
+	int			error, local;
 
 	error = xfs_parent_iread_extents(tp, child);
 	if (error)
@@ -204,6 +204,10 @@ xfs_parent_addname(
 	xfs_parent_da_args_init(&ppargs->args, tp, &ppargs->rec, child,
 			child->i_ino, parent_name);
 
+	/* Growing the attr fork needs a real reservation in args->total. */
+	ppargs->args.total = xfs_attr_calc_size(&ppargs->args, &local);
+	ASSERT(local);
+
 	return xfs_attr_setname(&ppargs->args, 0);
 }
 
@@ -240,7 +244,7 @@ xfs_parent_replacename(
 	const struct xfs_name	*new_name,
 	struct xfs_inode	*child)
 {
-	int			error;
+	int			error, local;
 
 	error = xfs_parent_iread_extents(tp, child);
 	if (error)
@@ -250,6 +254,10 @@ xfs_parent_replacename(
 	xfs_parent_da_args_init(&ppargs->args, tp, &ppargs->rec, child,
 			child->i_ino, old_name);
 
+	/* Growing the attr fork needs a real reservation in args->total. */
+	ppargs->args.total = xfs_attr_calc_size(&ppargs->args, &local);
+	ASSERT(local);
+
 	xfs_inode_to_parent_rec(&ppargs->new_rec, new_dp);
 
 	ppargs->args.new_name = new_name->name;
-- 
Javier Tia

Re: [PATCH v2 5/6] xfs: initialise args->total for parent pointer updates

[Darrick J. Wong]

On Mon, Aug 10, 2026 at 10:43:18AM -0600, Javier Tia wrote:
> xfs_parent_da_args_init() builds an xfs_da_args from a zeroed
> xfs_parent_args (kmem_cache_zalloc), leaving args->total == 0.
> xfs_da_grow_inode_int() treats that field as a running block reservation
> and subtracts from it; because it is an xfs_extlen_t (uint32_t), the
> first attr-fork growth wraps it to ~0U.  That defeats the free-space
> check in xfs_alloc_space_available(), and when it coincides with an AG
> that has exactly zero available blocks the allocation is clamped to
> maxlen 0 and returns -ENOSPC, which xfs_defer_finish_noroll() escalates
> to a filesystem shutdown.
> 
> Set args->total the way the log recovery path does
> (xfs_attri_recover_work(), xfs_attr_item.c:706), in the add and replace
> paths that can grow the fork.  Removals and lookups never grow it, so
> they leave the field alone, matching that switch.
> 
> Fixes: b7c62d90c12c ("xfs: parent pointer attribute creation")
> Cc: <stable@vger.kernel.org> # v6.10
> Signed-off-by: Javier Tia <floss@jetm.me>

Much improved, thanks for the corrections.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

(As a future note: please send new revisions of patchsets as a new
thread, not a continuation of the previous revision.)

--D

> ---
>  fs/xfs/libxfs/xfs_parent.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_parent.c b/fs/xfs/libxfs/xfs_parent.c
> index 3509cc4b2175..312f33086d53 100644
> --- a/fs/xfs/libxfs/xfs_parent.c
> +++ b/fs/xfs/libxfs/xfs_parent.c
> @@ -194,7 +194,7 @@ xfs_parent_addname(
>  	const struct xfs_name	*parent_name,
>  	struct xfs_inode	*child)
>  {
> -	int			error;
> +	int			error, local;
>  
>  	error = xfs_parent_iread_extents(tp, child);
>  	if (error)
> @@ -204,6 +204,10 @@ xfs_parent_addname(
>  	xfs_parent_da_args_init(&ppargs->args, tp, &ppargs->rec, child,
>  			child->i_ino, parent_name);
>  
> +	/* Growing the attr fork needs a real reservation in args->total. */
> +	ppargs->args.total = xfs_attr_calc_size(&ppargs->args, &local);
> +	ASSERT(local);
> +
>  	return xfs_attr_setname(&ppargs->args, 0);
>  }
>  
> @@ -240,7 +244,7 @@ xfs_parent_replacename(
>  	const struct xfs_name	*new_name,
>  	struct xfs_inode	*child)
>  {
> -	int			error;
> +	int			error, local;
>  
>  	error = xfs_parent_iread_extents(tp, child);
>  	if (error)
> @@ -250,6 +254,10 @@ xfs_parent_replacename(
>  	xfs_parent_da_args_init(&ppargs->args, tp, &ppargs->rec, child,
>  			child->i_ino, old_name);
>  
> +	/* Growing the attr fork needs a real reservation in args->total. */
> +	ppargs->args.total = xfs_attr_calc_size(&ppargs->args, &local);
> +	ASSERT(local);
> +
>  	xfs_inode_to_parent_rec(&ppargs->new_rec, new_dp);
>  
>  	ppargs->args.new_name = new_name->name;
> -- 
> Javier Tia
> 
> 

Re: [PATCH v2 5/6] xfs: initialise args->total for parent pointer updates

[Javier Tia]

Hi Darrick,

Thanks for the reviews on 5/6 and 6/6, and for catching the args->total
placement in v1.

Noted on sending revisions as their own thread - I'll do that from the
next one.

Patch 3 ("report the error that made deferred work shut down the fs") is
the only one in the series without your Reviewed-by. Would you mind
taking a look? It only moves the tracepoint ahead of the shutdown and
logs the errno plus t_blk_res, so nothing invasive, but I'd rather not
land it without a second pair of eyes.

-- 
Javier Tia

Re: [PATCH v2 5/6] xfs: initialise args->total for parent pointer updates

[Darrick J. Wong]

On Mon, Aug 10, 2026 at 12:39:48PM -0600, Javier Tia wrote:
> Hi Darrick,
> 
> Thanks for the reviews on 5/6 and 6/6, and for catching the args->total
> placement in v1.

No problem.

> Noted on sending revisions as their own thread - I'll do that from the
> next one.
> 
> Patch 3 ("report the error that made deferred work shut down the fs") is
> the only one in the series without your Reviewed-by. Would you mind
> taking a look? It only moves the tracepoint ahead of the shutdown and
> logs the errno plus t_blk_res, so nothing invasive, but I'd rather not
> land it without a second pair of eyes.

Done.  I did actually miss that one, so thanks for the hint.

--D