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

[liubaolin <liubaolin12138@163.com>]

在 2026/8/27 12:04, Hongling Zeng 写道:
> 
> 在 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

Hi Hyunchul,
    I think this approach is good.Looking forward to your feedback.

Thanks,
Baolin.