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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

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.

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