Re: [PATCH] ntfs: serialize read iomap lookup with mrec_lock

[Hyunchul Lee]

Hi Cen,

2026년 6월 26일 (금) 오전 10:47, Cen Zhang <zzzccc427@xxxxxxxxx>님이 작성:
>
> Take the base inode's mrec_lock in the shared read/seek iomap begin
> wrapper so both resident and non-resident lookups run under the same
> serialization already used by the write-side metadata path.
>
> The buggy scenario involves two paths, with each column showing the order
> within that path:
>
> read/seek iomap lookup path:
> 1. Enter __ntfs_read_iomap_begin().
> 2. Enter ntfs_attr_lookup() / ntfs_external_attr_find() on
>    base_ni->attr_list.
> 3. Keep raw attr_list_entry cursors while mapping the extent.
>
> attr-list mutation path:
> 1. Enter ntfs_attrlist_entry_add() or ntfs_attrlist_entry_rm().
> 2. Publish the replacement base_ni->attr_list.
> 3. kvfree() the old buffer.
>
> Validation reproduced this kernel report:
> BUG: KASAN: slab-use-after-free in ntfs_attr_lookup+0x24b2/0x2bf0
>
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x66/0xa0
>  print_report+0xce/0x630
>  ? ntfs_attr_lookup+0x24b2/0x2bf0
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? __virt_addr_valid+0x20d/0x410
>  ? ntfs_attr_lookup+0x24b2/0x2bf0
>  kasan_report+0xe0/0x110
>  ? ntfs_attr_lookup+0x24b2/0x2bf0
>  ntfs_attr_lookup+0x24b2/0x2bf0
>  ? ntfs_attr_lookup+0x9/0x2bf0
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? 0xffffffffc00000da
>  ? __pfx_ntfs_attr_lookup+0x10/0x10
>  ntfs_attr_map_whole_runlist+0x488/0x8b0
>  ? __pfx_ntfs_attr_map_whole_runlist+0x10/0x10
>  ? __pfx_do_raw_spin_trylock+0x10/0x10
>  ntfs_attr_vcn_to_rl+0x13a/0x1d0
>  ? filemap_fault+0xf03/0x2080
>  ntfs_read_iomap_begin_non_resident+0x142/0x920
>  ? __pfx_ntfs_read_iomap_begin_non_resident+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? __pfx_ntfs_read_iomap_begin+0x10/0x10
>  iomap_iter+0x6de/0x11e0
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? prepare_alloc_pages+0x1c4/0x4d0
>  iomap_readahead+0x239/0x910
>  ? folios_put_refs+0x1ed/0x4f0
>  ? __pfx_iomap_readahead+0x10/0x10
>  ? __pfx_folios_put_refs+0x10/0x10
>  ? lock_release+0x1e0/0x280
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? xas_store+0x871/0x1650
>  ? xas_find_conflict+0x64b/0xb90
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_release+0x1e0/0x280
>  ntfs_readahead+0x193/0x1d0
>  ? __pfx_ntfs_readahead+0x10/0x10
>  ? __filemap_add_folio+0x869/0xa60
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_acquire+0x2b8/0x2f0
>  ? __pfx___filemap_add_folio+0x10/0x10
>  read_pages+0x18f/0x890
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? folio_alloc_noprof+0x69/0xb0
>  ? __pfx_read_pages+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  page_cache_ra_unbounded+0x389/0x6c0
>  do_page_cache_ra+0x109/0x170
>  filemap_fault+0xf03/0x2080
>  ? __pfx_filemap_fault+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_acquire+0x2b8/0x2f0
>  ? lockdep_init_map_type+0x5c/0x230
>  __do_fault+0xf0/0x260
>  __handle_mm_fault+0xdcf/0x1ca0
>  ? __pfx___handle_mm_fault+0x10/0x10
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? srso_alias_return_thunk+0x5/0xfbef5
>  ? lock_release+0x1e0/0x280
>  handle_mm_fault+0x19c/0x470
>  do_user_addr_fault+0x23b/0x9c0
>  exc_page_fault+0x5c/0xc0
>  asm_exc_page_fault+0x26/0x30
>
> Allocated by task 511:
>  kasan_save_stack+0x33/0x60
>  kasan_save_track+0x14/0x30
>  __kasan_kmalloc+0xaa/0xb0
>  __kvmalloc_node_noprof+0x353/0x920
>  ntfs_attrlist_entry_rm+0x28d/0x510
>  ntfs_attr_record_rm+0x4dd/0xda0
>  ntfs_delete+0x50a/0xf90
>  ntfs_unlink+0x212/0x4d0
>  vfs_unlink+0x259/0xb00
>  filename_unlinkat+0x2db/0x5f0
>  __x64_sys_unlink+0x46/0x70
>  do_syscall_64+0x115/0x6a0
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Freed by task 511:
>  kasan_save_stack+0x33/0x60
>  kasan_save_track+0x14/0x30
>  kasan_save_free_info+0x3b/0x60
>  __kasan_slab_free+0x5f/0x80
>  kfree+0x307/0x580
>  ntfs_attrlist_entry_add+0xc1b/0x1110
>  ntfs_resident_attr_record_add+0x6a8/0xa10
>  ntfs_attr_add+0x682/0xcd0
>  __ntfs_link+0x815/0xde0
>  ntfs_link+0x223/0x3d0
>  vfs_link+0x465/0xcf0
>  filename_linkat+0x316/0x540
>  __x64_sys_link+0x81/0xb0
>  do_syscall_64+0x115/0x6a0
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>
> Fixes: b041ca562526 ("ntfs: update iomap and address space operations")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
> ---
>  fs/ntfs/iomap.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ntfs/iomap.c b/fs/ntfs/iomap.c
> index 52eecf5cb256..5f15a0432f0c 100644
> --- a/fs/ntfs/iomap.c
> +++ b/fs/ntfs/iomap.c
> @@ -263,11 +263,26 @@ static int __ntfs_read_iomap_begin(struct inode *inode, loff_t offset, loff_t le
>                 unsigned int flags, struct iomap *iomap, struct iomap *srcmap,
>                 bool need_unwritten)
>  {
> -       if (NInoNonResident(NTFS_I(inode)))
> -               return ntfs_read_iomap_begin_non_resident(inode, offset, length,
> -                               flags, iomap, need_unwritten);
> -       return ntfs_read_iomap_begin_resident(inode, offset, length,
> -                                            flags, iomap);
> +       struct ntfs_inode *ni = NTFS_I(inode);
> +       struct ntfs_inode *base_ni = NInoAttr(ni) ? ni->ext.base_ntfs_ino : ni;
> +       bool lock_mrec = NInoAttrList(base_ni);
> +       int err;
> +
> +       if (lock_mrec)
> +               mutex_lock(&base_ni->mrec_lock);

It looks like there is a race condition here.
The concurrent writer may create attr_list immediately after this check.
In that case, the reader can call ntfs_external_attr_find() from
ntfs_attr_lookup() and access the newly created attr_list without the lock.

There also appears to be a few places such as ntfs_attr_map_whole_runlist()
to call ntfs_attr_lookup() without the lock.

Considering performance, how about introducing the rw_semaphore to
protect attr_list?

> +
> +       if (NInoNonResident(ni))
> +               err = ntfs_read_iomap_begin_non_resident(inode, offset, length,
> +                                                        flags, iomap,
> +                                                        need_unwritten);
> +       else
> +               err = ntfs_read_iomap_begin_resident(inode, offset, length,
> +                                                    flags, iomap);
> +
> +       if (lock_mrec)
> +               mutex_unlock(&base_ni->mrec_lock);
> +
> +       return err;
>  }
>
>  static int ntfs_read_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> --
> 2.43.0
>


-- 
Thanks,
Hyunchul