Re: [PATCH v2 4/6] ntfs: do not map a vcn as a hole when its runlist lookup failed

[Hyunchul Lee]

On Sun, Sep 27, 2026 at 01:08:23PM +0800, Matthias Goergens wrote:
> ntfs_attr_vcn_to_rl() retries ntfs_map_runlist_nolock() for any lcn up
> to LCN_RL_NOT_MAPPED, which includes LCN_ENOENT, but turns a failed
> retry into an error only for LCN_RL_NOT_MAPPED.  For LCN_ENOENT the
> error is dropped and the read path maps the range as a hole.
> 
> An LCN_ENOENT below allocated_size comes from a base extent with a
> highest_vcn of 0, which ntfs_mapping_pairs_decompress() takes to map the
> whole attribute, so the runlist ends after its last mapping pair.  If
> the pairs end early, the retry finds the same extent and fails with
> -ENOENT.  On a crafted volume with 4 KiB clusters, a 64-cluster file
> whose mapping pairs stop after 16 clusters reads 48 clusters of zeros,
> with no error.
> 
> The same layout gets a crafted $MFT past the check from "ntfs: fail the
> mount when $MFT needs its own extent records".  With 512-byte clusters
> and $MFT's mapping pairs ending at vcn 4, an unpatched kernel hangs on
> the folio lock reading records 0-3.  With the check alone, the -EIO is
> dropped, records 2 and 3 read as zeros and the mount carries on until
> check_mft_mirror() finds the zeroed record 2.
> 
> Fail the lookup whenever the retry leaves @vcn unmapped, -ENOENT
> included.  At or beyond allocated_size nothing is mapped, so do not
> retry there: the runlist ends with LCN_ENOENT, or with LCN_RL_NOT_MAPPED
> when only the last extent is mapped, as after a write into it, and with
> clusters smaller than a page every read of a file's last folio looks up
> such vcns.
> 
> A failed expansion in ntfs_non_resident_attr_expand() or
> ntfs_attrlist_repack() truncates the runlist under the runlist lock but
> restores allocated_size only after dropping it.  A lookup in between
> would now fail, so restore allocated_size under the lock in both.
> 
> The crafted file now fails from vcn 16 on with -EIO, and the crafted
> volume fails to mount with the check's message.
> 
> Fixes: 495e90fa3348 ("ntfs: update attrib operations")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> ---
>  fs/ntfs/attrib.c   | 38 +++++++++++++++++++++++++++++++-------
>  fs/ntfs/attrlist.c |  8 ++++++--
>  2 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index eab4d8d32132f..30d3d2eb5ef3c 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -344,6 +344,23 @@ struct runlist_element *ntfs_attr_vcn_to_rl(struct ntfs_inode *ni, s64 vcn, s64
>  		rl++;
>  	*lcn = ntfs_rl_vcn_to_lcn(rl, vcn);
>  
> +	/*
> +	 * Nothing is mapped at or beyond the allocated size: the runlist ends
> +	 * there with LCN_ENOENT, or with LCN_RL_NOT_MAPPED if only a later
> +	 * extent has been mapped.  Return that end as it is.  Below the
> +	 * allocated size, an unmapped vcn is worth a retry.
> +	 */
> +	if (*lcn <= LCN_RL_NOT_MAPPED && !is_retry) {
> +		unsigned long flags;
> +		s64 allocated_vcn;
> +
> +		read_lock_irqsave(&ni->size_lock, flags);
> +		allocated_vcn = ntfs_bytes_to_cluster(ni->vol, ni->allocated_size);
> +		read_unlock_irqrestore(&ni->size_lock, flags);
> +		if (vcn >= allocated_vcn)
> +			return rl;
> +	}
> +

Could you merge the above if statement with the one below? Both
statments use the same condition.

>  	if (*lcn <= LCN_RL_NOT_MAPPED && is_retry == false) {
>  		is_retry = true;
>  		err = ntfs_map_runlist_nolock(ni, vcn, NULL);
> @@ -354,11 +371,14 @@ struct runlist_element *ntfs_attr_vcn_to_rl(struct ntfs_inode *ni, s64 vcn, s64
>  	}
>  
>  	/*
> -	 * The runlist fragment containing @vcn could not be mapped, e.g.
> -	 * because the extent mft record holding it is corrupt.  Do not hand
> -	 * LCN_RL_NOT_MAPPED back to callers, which would treat it as a hole.
> +	 * Neither the runlist nor the retry mapped @vcn, which lies below the
> +	 * allocated size, e.g. because the extent mft record holding it is
> +	 * corrupt or because the mapping pairs end too soon.
> +	 * ntfs_map_runlist_nolock() reports the latter as -ENOENT, as @vcn
> +	 * lies past the extent it found.  Callers would treat
> +	 * LCN_RL_NOT_MAPPED or LCN_ENOENT here as a hole, so fail instead.
>  	 */
> -	if (*lcn == LCN_RL_NOT_MAPPED)
> +	if (*lcn <= LCN_RL_NOT_MAPPED)
>  		return ERR_PTR(err == -ENOMEM ? -ENOMEM : -EIO);
>  
>  	return rl;
> @@ -4703,11 +4723,17 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  	if (err2)
>  		ntfs_debug("Leaking clusters");
>  
> -	/* Now, truncate the runlist itself. */
> +	/*
> +	 * Now, truncate the runlist itself.  Restore allocated_size before
> +	 * dropping the lock: ntfs_attr_vcn_to_rl() fails a lookup below the
> +	 * allocated size that falls past the end of the runlist.
> +	 */
>  	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 (!err2)
> +		ni->allocated_size = org_alloc_size;

We should protect it with ni->size_lock.

>  	if (ni != locked_ni)
>  		up_write(&ni->runlist.lock);
>  	if (err2) {
> @@ -4719,8 +4745,6 @@ static int ntfs_non_resident_attr_expand(struct ntfs_inode *ni, const s64 newsiz
>  		ni->runlist.rl = NULL;
>  		ntfs_error(sb, "Couldn't truncate runlist. Rollback failed");
>  	} else {
> -		/* Prepare to mapping pairs update. */
> -		ni->allocated_size = org_alloc_size;
>  		/* Restore mapping pairs. */
>  		if (ni != locked_ni)
>  			down_read(&ni->runlist.lock);
> diff --git a/fs/ntfs/attrlist.c b/fs/ntfs/attrlist.c
> index 1bbd2bc62c582..3660e7fd24b13 100644
> --- a/fs/ntfs/attrlist.c
> +++ b/fs/ntfs/attrlist.c
> @@ -168,14 +168,18 @@ static int ntfs_attrlist_repack(struct inode *attr_vi,
>  	return 0;
>  
>  restore_old_runlist:
> +	/*
> +	 * Restore allocated_size before dropping the runlist lock:
> +	 * ntfs_attr_vcn_to_rl() fails a lookup below the allocated size that
> +	 * falls past the end of the runlist.
> +	 */
>  	down_write(&attr_ni->runlist.lock);
>  	attr_ni->runlist.rl = old_rl;
>  	attr_ni->runlist.count = old_rl_count;
> -	up_write(&attr_ni->runlist.lock);
> -
>  	write_lock_irqsave(&attr_ni->size_lock, flags);
>  	attr_ni->allocated_size = old_alloc_size;
>  	write_unlock_irqrestore(&attr_ni->size_lock, flags);
> +	up_write(&attr_ni->runlist.lock);
>  
>  	restore_err = ntfs_attr_update_mapping_pairs_locked(
>  			attr_ni, 0, locked_ni);
> -- 
> 2.55.0
> 

-- 
Thanks,
Hyunchul