Re: [PATCH] xfs: fix NOFS state corruption in btree split worker

[Brian Foster <bfoster@redhat.com>]

On Thu, Sep 03, 2026 at 09:37:56PM +0800, Kefeng Wang wrote:
> xfs_btree_split_worker() calls xfs_trans_set_context() and
> xfs_trans_clear_context() on the caller's transaction, overwriting
> tp->t_pflags with the worker's NOFS state. When the caller already has
> PF_MEMALLOC_NOFS set (e.g. xfs_end_ioend_write, xfs_dio_write_end_io),
> the corrupted tp->t_pflags causes xfs_trans_free() to erroneously clear
> the caller's NOFS protection.
> 
> Use memalloc_nofs_save/restore with a local variable instead so
> tp->t_pflags is never touched.
> 
> Closes: https://sashiko.dev/#/patchset/20260902131653.1338227-1-wangkefeng.wang@huawei.com
> Fixes: 756b1c343333 ("xfs: use current->journal_info for detecting transaction recursion")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---

I agree that the Sashiko analysis looks correct. The only thing I wonder
is whether it might be a bit cleaner to have the set_context() helper
return the context instead of hardcode the assignment to ->t_pflags so
it can be used in both places. The reasoning is just that the current
arrangement kind of makes it easy to repeat this mistake in the future.

Then again, it's a single line helper so maybe another option could be
to just remove and open code it. I suppose the pro of keeping the helper
is that it's a decent spot to document the concern and why it returns a
value, etc. *shrug* Thoughts?

(Please don't change this patch just on my comments alone. Let's see if
others have input first..).

Brian

>  fs/xfs/libxfs/xfs_btree.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 6738d9d1511b..8ae4b94e6995 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
> @@ -3007,12 +3007,18 @@ xfs_btree_split_worker(
>  {
>  	struct xfs_btree_split_args	*args = container_of(work,
>  						struct xfs_btree_split_args, work);
> -	xfs_trans_set_context(args->cur->bc_tp);
> +	unsigned int			nofs_flags;
> +
> +	/*
> +	 * Don't use xfs_trans_set_context() here: it would overwrite the
> +	 * caller's saved NOFS state in tp->t_pflags.  Use a local scope.
> +	 */
> +	nofs_flags = memalloc_nofs_save();
>  
>  	args->result = __xfs_btree_split(args->cur, args->level, args->ptrp,
>  					 args->key, args->curp, args->stat);
>  
> -	xfs_trans_clear_context(args->cur->bc_tp);
> +	memalloc_nofs_restore(nofs_flags);
>  
>  	/*
>  	 * Do not access args after complete() has run here. We don't own args
> -- 
> 2.55.0
> 
>