Re: [PATCH] ntfs: hold the runlist lock while expanding a non-resident attribute

[Hyunchul Lee <hyc.lee@gmail.com>]

On Sun, Sep 27, 2026 at 02:31:49PM +0800, Matthias Goergens wrote:
> ntfs_non_resident_attr_expand() changes ni->runlist.rl while it
> allocates clusters: ntfs_attr_map_whole_runlist(), the compressed
> branch's ntfs_rl_realloc() and ntfs_runlists_merge() can each kvfree()
> the old array.  Unless the caller already holds ni->runlist.lock, none
> of this happens under it; only the rollback takes it.  Callers without
> it include the write path, fallocate, ntfs_inode_attr_pwrite() and
> ntfs_resident_attr_resize().
> 
> Those callers hold mrec_lock, and the write path also inode_lock, but
> the iomap read side takes neither: FIEMAP, page faults, readahead and
> splice read reach ntfs_read_iomap_begin_non_resident(), which takes only
> ni->runlist.lock.  A FIEMAP running while write(2) extends the same file
> can walk the array that ntfs_runlists_merge() has just freed:
> 
>   BUG: KASAN: slab-use-after-free in ntfs_attr_vcn_to_rl+0x19b/0x200
> 
> A reader that finds a vcn unmapped in the stale array also maps it from
> disk into the runlist the writer is changing, which shows up as "Run
> lists overlap. Cannot merge!".
> 
> Take ni->runlist.lock for writing around the part of the expansion that
> changes the runlist and allocated_size, up to and including the mapping
> pairs update, as ntfs_non_resident_attr_shrink() does since commit
> 91709ba5d6d7 ("ntfs: protect runlist updates with the runlist lock"),
> and hold it throughout the rollback, where ntfs_cluster_free() requires
> it.  The lock nests inside mrec_lock, as on the truncate-up path, which
> already calls this function with the runlist lock held.
> 
> For an $ATTRIBUTE_LIST whose lock the caller does not hold, drop the
> lock again before the mapping pairs update: that update can resize the
> same list through ntfs_attrlist_update_locked(), which returns -ENOSPC
> when told that the list's lock is held.  Holding it there made punching
> holes into a large fragmented file fail with -ENOSPC.
> 
> Fixes: 495e90fa3348 ("ntfs: update attrib operations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
> ---
> A reproducer that extends two files with interleaved clusters while
> FIEMAP loops over them hits the KASAN report above within two seconds in
> each of three runs; with this patch, three 60-second runs are clean.  I
> can send it privately if you want it.
> 
> Also tested in a VM with KASAN, lockdep and the hung task detector:
> writes past EOF, fallocate, truncate-up, sparse and compressed files,
> growing xattrs, directories and a non-resident attribute list, with
> FIEMAP after each step.  Output matches the unpatched kernel, and
> lockdep reports nothing.
> 
>  fs/ntfs/attrib.c | 72 ++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 333b3371acb47..a07acef3a03e8 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -4453,6 +4453,7 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  	struct ntfs_inode *base_ni;
>  	struct super_block *sb = ni->vol->sb;
>  	size_t new_rl_count;
> +	bool runlist_locked = locked_ni == ni;
>  
>  	ntfs_debug("Inode 0x%llx, attr 0x%x, new size %lld old size %lld\n",
>  			(unsigned long long)ni->mft_no, ni->type,
> @@ -4494,10 +4495,17 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  	 * clusters if there is a change.
>  	 */
>  	if (ntfs_bytes_to_cluster(vol, ni->allocated_size) < first_free_vcn) {
> +		/*
> +		 * The runlist array is replaced below.  Readers such as the
> +		 * iomap read path hold only the runlist lock, not mrec_lock.
> +		 */
> +		if (!runlist_locked)
> +			down_write(&ni->runlist.lock);

Could you add a locked_assert_held_write() check when runlist_locked is
true? This would catch callers which does not hold the runlist.lock. 

Otherwise this patch looks good to me.

Reviewed-by: Hyunchul Lee <hyc.lee@gmail.com>

> +
>  		err = ntfs_attr_map_whole_runlist(ni);
>  		if (err) {
>  			ntfs_error(sb, "ntfs_attr_map_whole_runlist failed");
> -			return err;
> +			goto unlock_runlist;
>  		}
>  
>  		/*
> @@ -4522,7 +4530,7 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  						last + more_entries + 1);
>  				if (IS_ERR(rl)) {
>  					err = -ENOMEM;
> -					goto put_err_out;
> +					goto unlock_runlist;
>  				}
>  
>  				alloc_size = ni->allocated_size;
> @@ -4547,7 +4555,7 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  				rl = kmalloc(sizeof(struct runlist_element) * 2, GFP_NOFS);
>  				if (!rl) {
>  					err = -ENOMEM;
> -					goto put_err_out;
> +					goto unlock_runlist;
>  				}
>  
>  				rl[0].vcn = ntfs_bytes_to_cluster(vol, ni->allocated_size);
> @@ -4594,7 +4602,8 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  				ntfs_debug("Cluster allocation failed (%lld)",
>  						(long long)first_free_vcn -
>  						ntfs_bytes_to_cluster(vol, ni->allocated_size));
> -				return PTR_ERR(rl);
> +				err = PTR_ERR(rl);
> +				goto unlock_runlist;
>  			}
>  			/*
>  			 * A contiguous ATTRIBUTE_LIST allocation keeps its mapping
> @@ -4618,8 +4627,10 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  								ni->allocated_size),
>  						lcn_seek_from, DATA_ZONE, false,
>  						false, false);
> -				if (IS_ERR(rl))
> -					return PTR_ERR(rl);
> +				if (IS_ERR(rl)) {
> +					err = PTR_ERR(rl);
> +					goto unlock_runlist;
> +				}
>  			}
>  		}
>  
> @@ -4631,7 +4642,8 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  				ntfs_error(sb, "Run list merge failed");
>  				ntfs_cluster_free_from_rl(vol, rl);
>  				kvfree(rl);
> -				return -EIO;
> +				err = -EIO;
> +				goto unlock_runlist;
>  			}
>  			ni->runlist.rl = rln;
>  			ni->runlist.count = new_rl_count;
> @@ -4639,11 +4651,28 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  
>  		/* Prepare to mapping pairs update. */
>  		ni->allocated_size = ntfs_cluster_to_bytes(vol, first_free_vcn);
> -		err = ntfs_attr_update_mapping_pairs_locked(
> -				ni, 0, locked_ni);
> -		if (err) {
> -			ntfs_debug("Mapping pairs update failed");
> -			goto rollback;
> +		if (ni->type == AT_ATTRIBUTE_LIST && !runlist_locked) {
> +			/*
> +			 * Making room for the list's mapping pairs can resize
> +			 * this attribute list again through
> +			 * ntfs_attrlist_update_locked(), which takes its
> +			 * runlist lock.
> +			 */
> +			up_write(&ni->runlist.lock);
> +			err = ntfs_attr_update_mapping_pairs_locked(ni, 0,
> +								    locked_ni);
> +			if (err) {
> +				ntfs_debug("Mapping pairs update failed");
> +				goto rollback;
> +			}
> +		} else {
> +			err = ntfs_attr_update_mapping_pairs_locked(ni, 0, ni);
> +			if (err) {
> +				ntfs_debug("Mapping pairs update failed");
> +				goto rollback_locked;
> +			}
> +			if (!runlist_locked)
> +				up_write(&ni->runlist.lock);
>  		}
>  	}
>  
> @@ -4677,6 +4706,9 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  	ntfs_attr_put_search_ctx(ctx);
>  	return 0;
>  rollback:
> +	if (!runlist_locked)
> +		down_write(&ni->runlist.lock);
> +rollback_locked:
>  	/* Free allocated clusters. */
>  	err2 = ntfs_cluster_free(ni, ntfs_bytes_to_cluster(vol, org_alloc_size),
>  				-1, ctx);
> @@ -4684,12 +4716,8 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  		ntfs_debug("Leaking clusters");
>  
>  	/* Now, truncate the runlist itself. */
> -	if (ni != locked_ni)
> -		down_write(&ni->runlist.lock);
>  	err2 = ntfs_rl_truncate_nolock(vol, &ni->runlist,
>  			ntfs_bytes_to_cluster(vol, org_alloc_size));
> -	if (ni != locked_ni)
> -		up_write(&ni->runlist.lock);
>  	if (err2) {
>  		/*
>  		 * Failed to truncate the runlist, so just throw it away, it
> @@ -4702,12 +4730,8 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  		/* Prepare to mapping pairs update. */
>  		ni->allocated_size = org_alloc_size;
>  		/* Restore mapping pairs. */
> -		if (ni != locked_ni)
> -			down_read(&ni->runlist.lock);
> -		if (__ntfs_attr_update_mapping_pairs(ni, 0, locked_ni, true))
> +		if (__ntfs_attr_update_mapping_pairs(ni, 0, ni, true))
>  			ntfs_error(sb, "Failed to restore old mapping pairs");
> -		if (ni != locked_ni)
> -			up_read(&ni->runlist.lock);
>  
>  		if (NInoSparse(ni) || NInoCompressed(ni)) {
>  			ni->itype.compressed.size =  org_compressed_size;
> @@ -4715,6 +4739,8 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  		} else
>  			VFS_I(base_ni)->i_blocks = ni->allocated_size >> 9;
>  	}
> +	if (!runlist_locked)
> +		up_write(&ni->runlist.lock);
>  	if (ctx)
>  		ntfs_attr_put_search_ctx(ctx);
>  	return err;
> @@ -4722,6 +4748,10 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  	if (ctx)
>  		ntfs_attr_put_search_ctx(ctx);
>  	return err;
> +unlock_runlist:
> +	if (!runlist_locked)
> +		up_write(&ni->runlist.lock);
> +	return err;
>  }
>  
>  /*
> 
> base-commit: 259abb551e2944998cad4214c201954ab1ac5c8d
> -- 
> 2.55.0
> 

-- 
Thanks,
Hyunchul