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

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

Posted by Hongling Zeng 1 month ago

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:
-- 
2.25.1

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

Posted by Hongling Zeng 1 month ago

Hi Hyunchul,

Thanks for the review.

I sent an updated patch too quickly. Simply removing map_locked is not
correct because there is an early compressed-file exit before
invalidate_lock is taken, so making the unlock path unconditional would
unbalance the locking.

Please ignore the previous version. I'll send a v3 with the control-flow
fix shortly.

Thanks,
Hongling
在 2026年08月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:

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

Posted by liubaolin 1 month ago


在 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.

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

Posted by Hongling Zeng 1 month ago

在 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

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

Posted by liubaolin 1 month ago


在 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.

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

Posted by Hyunchul Lee 1 month ago

> > 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.

The approach looks fine to me as well.

>
> Thanks,
> Baolin.
>
>


-- 
Thanks,
Hyunchul