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

From: liubaolin

Date: Thu Aug 27 2026 - 00:34:12 EST

在 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@xxxxxxxxxxxxxxx
Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
Reviewed-by: Hyunchul Lee <hyc.lee@xxxxxxxxx>
Reviewed-by: Baolin Liu <liubaolin@xxxxxxxxxx>
---
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@xxxxxxxxxx>

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.