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

[liubaolin]

在 2026/8/27 10:43, Hyunchul Lee 写道:
> Hi Hongling,
>
> 2026년 8월 26일 (수) 오후 2:42, Hongling Zeng <zenghongling@xxxxxxxxxx>님이 작성:
> > 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>
> > ---
> >   fs/ntfs/file.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> > index 88747217ba61..6958a1469fb0 100644
> > --- a/fs/ntfs/file.c
> > +++ b/fs/ntfs/file.c
> > @@ -1153,11 +1153,9 @@ 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);
> > +       map_locked = true;
>
> map_locked can be removed.
> Otherwise it looks good to me.
>
> Reviewed-by: Hyunchul Lee <hyc.lee@xxxxxxxxx>
>
>
> >          switch (mode & FALLOC_FL_MODE_MASK) {
> >          case FALLOC_FL_ALLOCATE_RANGE:
> > --
> > 2.25.1
>
>
> --
> Thanks,
> Hyunchul

Hi Hyunchul and Hongling,

Thanks for reviewing. 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.

Thanks,
Baolin.