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

[Brian Foster <bfoster@redhat.com>]

On Fri, Sep 04, 2026 at 08:41:26AM +0800, Kefeng Wang wrote:
> 
> 
> On 9/3/2026 9:52 PM, Brian Foster wrote:
> > 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?
> 
> 
> I personally tend to remove the helper functions, but let's see what others
> think.
> 

I noticed after writing this that there were many more open coded nofs
calls than I originally thought, so this is a fair point. After some
thought I think this is a reasonable approach regardless:

Reviewed-by: Brian Foster <bfoster@redhat.com>

You might want to send as a standalone patch though..

Brian

> > 
> > (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
> > > 
> > > 
> > 
> > 
>