[PATCH] ntfs: serialize the resident read iomap path with mrec_lock

Hyeontae Lee posted 1 patch 2 months ago
fs/ntfs/iomap.c | 4 ++++
1 file changed, 4 insertions(+)
[PATCH] ntfs: serialize the resident read iomap path with mrec_lock
Posted by Hyeontae Lee 2 months ago
ntfs_read_iomap_begin_resident() walks the MFT record through
ntfs_attr_lookup() -> ntfs_attr_find() without taking ni->mrec_lock,
while ntfs_attr_record_resize(), ntfs_make_room_for_attr() and
ntfs_resident_attr_record_add() memmove() the same base_ni->mrec buffer
under that lock. map_mft_record() only takes a reference and does not
serialize, so the reader can observe torn attribute length and offset
fields while a writer is relocating the records.

KCSAN reports the race between the mmap read fault path and both link()
and unlink():

  BUG: KCSAN: data-race in ntfs_attr_find / ntfs_attr_record_resize

  write to 0xffff888100af1018 of 4 bytes by task 96 on cpu 1:
   ntfs_attr_record_resize+0xd2/0x130
   ntfs_attr_record_rm+0xad/0x530
   ntfs_delete+0x224/0x640
   ntfs_unlink+0x14d/0x280
   vfs_unlink+0x157/0x520

  read to 0xffff888100af1018 of 4 bytes by task 95 on cpu 0:
   ntfs_attr_find+0x104/0x5b0
   ntfs_attr_lookup+0x39c/0x10c0
   ntfs_read_iomap_begin_resident+0xc6/0x230
   ntfs_read_iomap_begin+0x5d/0xa0
   iomap_iter+0x2e2/0x6e0
   iomap_read_folio+0x147/0x2a0
   ntfs_read_folio+0x108/0x170
   filemap_read_folio+0x35/0x100
   filemap_fault+0x993/0x1000

  value changed: 0x00000250 -> 0x000001f0

The address is mrec + 0x18, i.e. mft_record.bytes_in_use, and the change
is the 96 bytes of one $FILE_NAME attribute being removed.

Take base_ni->mrec_lock around the lookup in
ntfs_read_iomap_begin_resident(). The non-resident path is left alone:
ntfs_lookup() already holds the directory inode's mrec_lock when it reads
an index folio through read_mapping_folio(), and taking the lock in the
shared wrapper deadlocks there with recursive locking on mrec_lock. The
comment above the read_mapping_folio() call in fs/ntfs/dir.c notes the
same hazard.

Tested with a reproducer that faults in a 16-byte resident file while
another thread runs link()/unlink() on it. Before: 40 KCSAN reports in
about one second. After: no reports in 180 seconds over 206,090 read
iterations and 423,540 link/unlink cycles. A PROVE_LOCKING build shows no
lockdep splat with the same reproducer running for 60 seconds.

Fixes: b041ca562526 ("ntfs: update iomap and address space operations")
Link: https://lore.kernel.org/all/20260725042421.109599-1-wonju345@naver.com/
Suggested-by: Hyunchul Lee <hyc.lee@gmail.com>
Signed-off-by: Hyeontae Lee <wonju345@naver.com>
---
 fs/ntfs/iomap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ntfs/iomap.c b/fs/ntfs/iomap.c
index 52eecf5cb2..f9fdceeeb3 100644
--- a/fs/ntfs/iomap.c
+++ b/fs/ntfs/iomap.c
@@ -95,6 +95,8 @@ static int ntfs_read_iomap_begin_resident(struct inode *inode, loff_t offset, lo
 	else
 		base_ni = ni;
 
+	mutex_lock(&base_ni->mrec_lock);
+
 	ctx = ntfs_attr_get_search_ctx(base_ni, NULL);
 	if (!ctx) {
 		err = -ENOMEM;
@@ -138,6 +140,8 @@ static int ntfs_read_iomap_begin_resident(struct inode *inode, loff_t offset, lo
 	if (ctx)
 		ntfs_attr_put_search_ctx(ctx);
 
+	mutex_unlock(&base_ni->mrec_lock);
+
 	return err;
 }
 
-- 
2.43.0
Re: [PATCH] ntfs: serialize the resident read iomap path with mrec_lock
Posted by Namjae Jeon 2 months ago
> diff --git a/fs/ntfs/iomap.c b/fs/ntfs/iomap.c
> index 52eecf5cb2..f9fdceeeb3 100644
> --- a/fs/ntfs/iomap.c
> +++ b/fs/ntfs/iomap.c
> @@ -95,6 +95,8 @@ static int ntfs_read_iomap_begin_resident(struct inode *inode, loff_t offset, lo
>         else
>                 base_ni = ni;
>
> +       mutex_lock(&base_ni->mrec_lock);
> +
>         ctx = ntfs_attr_get_search_ctx(base_ni, NULL);
>         if (!ctx) {
>                 err = -ENOMEM;
> @@ -138,6 +140,8 @@ static int ntfs_read_iomap_begin_resident(struct inode *inode, loff_t offset, lo
>         if (ctx)
>                 ntfs_attr_put_search_ctx(ctx);
>
> +       mutex_unlock(&base_ni->mrec_lock);
We need to hold the lock until iomap_end() because iomap_begin() only
returns an inline_data pointer into the MFT record. The actual data
copy happens later in the iomap core. So can you check if the attached
patch fixes this issue ?
Re: [PATCH] ntfs: serialize the resident read iomap path with mrec_lock
Posted by Hyeontae Lee 2 months ago
Hi Namjae,

You are right - my version drops the lock before the iomap core copies from
iomap->inline_data, so the copy itself was still unprotected.

I tested your patch and it fixes the issue:

KCSAN, two runs of 180 seconds each on the same reproducer:

  run 1: 206,015 read faults, 415,351 link/unlink cycles
  run 2: 205,732 read faults, 421,672 link/unlink cycles

No data-race reports in ntfs_attr_find(), ntfs_attr_value_is_valid() or
ntfs_read_iomap_begin_resident(), and no "is corrupt" messages. Before the
fix the same reproducer produced 40 reports in about one second.

PROVE_LOCKING, 60 seconds with the same reproducer: 1,199,159 read faults,
1,396,557 link/unlink cycles, no lockdep splat. Mount, read, write and
unmount all behave normally.

Tested-by: Hyeontae Lee <wonju345@naver.com>

Thanks,
Hyeontae