Re: [PATCH v2 1/6] ntfs: do not map an unmappable runlist fragment as a hole

[liubaolin]

在 2026/9/27 13:08, Matthias Goergens 写道:
> When the extent mft record holding part of a file's runlist cannot be
> read, ntfs_attr_vcn_to_rl() ignores the failed ntfs_map_runlist_nolock()
> retry and returns with *lcn == LCN_RL_NOT_MAPPED.  The iomap read path
> only rejects lcn < LCN_ENOENT, so it maps the range as a hole and read()
> returns zeros with no error.  The first read already does this:
> ntfs_attr_map_whole_runlist() keeps the fragments it could read,
> readahead drops its error, and the next lookup finds the unmapped tail.
>
> On a file whose runlist is split between its base record (vcn 0-214) and
> one extent record (vcn 215-1499), breaking only the extent record's FILE
> magic makes the kernel log "Failed to map extent mft record", yet read()
> returns 1285 clusters of zeros for vcn 215-1499.  A damaged attribute
> list entry, which makes the retry fail with -ENOENT, gives the same
> zeros.
>
> Return -ENOMEM if the retry ran out of memory and -EIO otherwise.  Both
> callers already handle an ERR_PTR; other negative lcns, including
> LCN_ENOENT, are returned as before.  Reads of vcn 215-1499 now fail with
> -EIO.  An intact volume exercised with buffered, mmap and O_DIRECT I/O,
> fallocate, truncate, sparse and compressed files behaves as before.
>
> Fixes: 495e90fa3348 ("ntfs: update attrib operations")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
> ---
>   fs/ntfs/attrib.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 333b3371acb47..a337a3429b401 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -317,7 +317,7 @@ int ntfs_map_runlist(struct ntfs_inode *ni, s64 vcn)
>   struct runlist_element *ntfs_attr_vcn_to_rl(struct ntfs_inode *ni, s64 vcn, s64 *lcn)
>   {
>   	struct runlist_element *rl = ni->runlist.rl;
> -	int err;
> +	int err = 0;
>   	bool is_retry = false;
>   
>   	if (!rl) {
> @@ -335,12 +335,21 @@ struct runlist_element *ntfs_attr_vcn_to_rl(struct ntfs_inode *ni, s64 vcn, s64
>   
>   	if (*lcn <= LCN_RL_NOT_MAPPED && is_retry == false) {
>   		is_retry = true;
> -		if (!ntfs_map_runlist_nolock(ni, vcn, NULL)) {
> +		err = ntfs_map_runlist_nolock(ni, vcn, NULL);
> +		if (!err) {
>   			rl = ni->runlist.rl;
>   			goto remap_rl;
>   		}
>   	}
>   
> +	/*
> +	 * 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.
> +	 */
> +	if (*lcn == LCN_RL_NOT_MAPPED)
> +		return ERR_PTR(err == -ENOMEM ? -ENOMEM : -EIO);
> +

Hi Matthias,
   This check can reject valid lookups beyond allocated_size.
   Although patch 4 addresses this, could you move its 
allocation-boundary check into this patch, before the 
ntfs_map_runlist_nolock() 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;
  }

  This would avoid introducing a regression when patch 1 is applied on 
its own, particularly for stable backports. The remaining changes can 
stay in patch 4.

Thanks,
Baolin.

>   	return rl;
>   }
>