Re: [PATCH v2] ntfs: fix race between fallocate and mmap reads

[Hongling Zeng <zhongling0719@126.com>]

在 2026年08月27日 11:37, liubaolin 写道:
>
>
> 在 2026/8/27 11:16, Hongling Zeng 写道:
>> The fallocate implementation only takes invalidate_lock for punch hole,
>> collapse range, and insert range operations. For standard allocation 
>> modes
>> (mode == 0, FALLOC_FL_KEEP_SIZE), the lock is not held.
>>
>> During ntfs_attr_fallocate(), new clusters are mapped to the runlist via
>> ntfs_attr_map_cluster() before being zeroed by ntfs_dio_zero_range(). 
>> This
>> creates a window where concurrent mmap page faults can read 
>> uninitialized
>> disk data.
>>
>> Since mmap uses filemap_fault() which takes invalidate_lock in shared 
>> mode,
>> it can fault in pages during this window and expose old disk contents to
>> userspace. This is an information leak and data integrity issue.
>>
>> Fix by taking invalidate_lock for all fallocate operations, not just for
>> punch/collapse/insert modes. This prevents concurrent page faults from
>> accessing unzeroed clusters during the allocation window.
>>
>> Fixes: 495e90fa3348 ("ntfs: update attrib operations")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>> Reviewed-by: Hyunchul Lee <hyc.lee@gmail.com>
>> Reviewed-by: Baolin Liu <liubaolin@kylinos.cn>
>> ---
>> Change in v2:
>>   -Remove now-unnecessary map_locked variable.
>> ---
>>   fs/ntfs/file.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
>> index 88747217ba61..779baafa0319 100644
>> --- a/fs/ntfs/file.c
>> +++ b/fs/ntfs/file.c
>> @@ -1153,11 +1153,8 @@ static long ntfs_fallocate(struct file *file, 
>> int mode, loff_t offset, loff_t le
>>       }
>>         inode_dio_wait(vi);
>> -    if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
>> -            FALLOC_FL_INSERT_RANGE)) {
>> -        filemap_invalidate_lock(vi->i_mapping);
>> -        map_locked = true;
>> -    }
>> +    /* Take invalidate_lock for all fallocate operations to prevent 
>> races */
>> +    filemap_invalidate_lock(vi->i_mapping);
>>         switch (mode & FALLOC_FL_MODE_MASK) {
>>       case FALLOC_FL_ALLOCATE_RANGE:
>
> Hi Hyunchul and Hongling,
>
> I think map_locked might still be needed though.
>
> Before filemap_invalidate_lock() is taken, there's this check:
>
>         inode_lock(vi);
>         if (NInoCompressed(ni) || NInoEncrypted(ni) || 
> NInoWofCompressed(ni)) {
>                 err = -EOPNOTSUPP;
>                 goto out;
>         }
>
>         inode_dio_wait(vi);
>         filemap_invalidate_lock(vi->i_mapping);
>
> If that goto is taken, we reach the exit path without having locked.
> Without map_locked, we'd call filemap_invalidate_unlock() on a lock
> we never took.
>
> Would it make sense to either:
> 1. Keep map_locked as a guard for the unlock, or
> 2. Change that goto to inode_unlock() + direct return, so the shared
>      exit path always holds the lock?
>
> I'd appreciate your thoughts on this.
>
> Reviewed-by: Baolin Liu <liubaolin@kylinos.cn>
>
> Thanks,
> Baolin.
Hi  Baolin and Hyunchul
  You're right — that's exactly the unbalanced-unlock path we spotted while
   preparing v3, and option 2 is what v3 implements. The early exit now does
   inode_unlock() + direct return, so every path reaching out: holds
   invalidate_lock, which let us drop map_locked entirely:

    inode_lock(vi);
    if (NInoCompressed(...) ) {
            inode_unlock(vi);
            return -EOPNOTSUPP;
    }
    inode_dio_wait(vi);
    filemap_invalidate_lock(vi->i_mapping);
    ...
    out:
            filemap_invalidate_unlock(vi->i_mapping);
    v3 with this cleanup is on its way / attached.

  Would this v3 approach be OK with you?

    Thanks,
    Hongling