[PATCH v2 0/3] ntfs: three fixes for crafted images, found by fuzzing

[PATCH v2 0/3] ntfs: three fixes for crafted images, found by fuzzing

[Wahid Khan]

I fuzz the new fs/ntfs driver with mutated images: an image from mkntfs
populated with ntfs-3g, a few bytes changed, then mount, walk the tree
and write, under KASAN. These three problems are still present in
ntfs-next (e62b384ca). Each patch has its trace and reproduction steps.

1. ntfs: do not read an index block outside $INDEX_ALLOCATION
   A directory without a usable $INDEX_ALLOCATION hangs on its first
   lookup. The task waits for an mft record lock that it already holds.
2. ntfs: open the index allocation before reading or writing an index block
   NULL pointer dereference in ntfs_ib_write() on link().
3. ntfs: do not convert a named $DATA stream when making room in a record
   WARN_ON() in ntfs_attr_make_non_resident(); no damaged volume needed.

All three need an image to be mounted, so a crafted image, not a remote
attacker.

Changes in v2, after an automated review of v1 that I checked by hand:
- 2/3: ntfs_ia_open() also succeeds for a directory without
  $INDEX_ALLOCATION and returns an empty resident inode. Reject an index
  inode that is not non-resident, as 1/3 already does.
- 3/3: v1 opened the named stream and converted it. That would lock the
  stream's folio under the mft record lock, the reverse of the read
  path (ntfs_read_iomap_begin_resident()). v2 skips named streams in
  that loop instead.
- 1/3: unchanged.

v1: https://lore.kernel.org/all/20261106091420.2461683-1-wahidzk0091@gmail.com/


Wahid Khan (3):
  ntfs: do not read an index block outside $INDEX_ALLOCATION
  ntfs: open the index allocation before reading or writing an index
    block
  ntfs: do not convert a named $DATA stream when making room in a record

 fs/ntfs/attrib.c |  6 ++++++
 fs/ntfs/dir.c    | 14 ++++++++++++++
 fs/ntfs/index.c  | 22 ++++++++++++++++++++++
 3 files changed, 42 insertions(+)

-- 
2.53.0

[PATCH v2 1/3] ntfs: do not read an index block outside $INDEX_ALLOCATION

[Wahid Khan]

A directory whose $INDEX_ROOT has a child node but whose
$INDEX_ALLOCATION is missing or damaged gets an empty index inode from
ntfs_read_locked_index_inode(): size 0 and not non-resident.
ntfs_lookup_inode_by_name() still follows the child node and reads a
folio from that inode while it holds the mft record lock of the
directory.

Since the resident iomap read started to take base_ni->mrec_lock, that
read waits for a lock the caller already holds:

  INFO: task init:1 is blocked on a mutex likely owned by task init:1.
   __mutex_lock
   __ntfs_read_iomap_begin
   ntfs_read_iomap_next
   iomap_read_folio
   ntfs_read_folio
   ntfs_lookup_inode_by_name
   ntfs_lookup

The task never returns, and neither does anything else that needs the
directory's mft record.

It takes one corrupted byte on an image from mkntfs: change the first
byte of the type of the $INDEX_ALLOCATION attribute header of a
directory with index blocks from 0xa0 to 0x00. The first lookup in that
directory hangs.

Check that the index inode is non-resident and that the vcn lies inside
it before every index block read, and fail with -EIO otherwise. This
also rejects a negative vcn, which was only checked on later descents.

Fixes: d9e00c457d4a ("ntfs: serialize resident iomap reads with mrec_lock")
Cc: stable@vger.kernel.org # v7.3+
Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
---
 fs/ntfs/dir.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
index fce10baa2..9fb5bb3d3 100644
--- a/fs/ntfs/dir.c
+++ b/fs/ntfs/dir.c
@@ -308,6 +308,20 @@ u64 ntfs_lookup_inode_by_name(struct ntfs_inode *dir_ni, const __le16 *uname,
 
 	ia_mapping = ia_vi->i_mapping;
 descend_into_child_node:
+	/*
+	 * The child node has to be inside $INDEX_ALLOCATION. A directory that
+	 * lacks the attribute gets an empty index inode that is not
+	 * non-resident, and reading a folio from it locks the mft record that
+	 * is already locked here.
+	 */
+	if (!NInoNonResident(NTFS_I(ia_vi)) || vcn < 0 ||
+	    (u64)vcn >= (u64)i_size_read(ia_vi) >> dir_ni->itype.index.vcn_size_bits) {
+		ntfs_error(sb,
+			"Child node vcn %lld is outside $INDEX_ALLOCATION in directory inode 0x%llx.",
+			vcn, dir_ni->mft_no);
+		err = -EIO;
+		goto err_out;
+	}
 	/*
 	 * Convert vcn to index into the index allocation attribute in units
 	 * of PAGE_SIZE and map the page cache page, reading it from
-- 
2.53.0

[PATCH v2 2/3] ntfs: open the index allocation before reading or writing an index block

[Wahid Khan]

ntfs_ib_write() and ntfs_ib_read() use icx->ia_ni without checking it.
ntfs_ir_reparent() only opens the index allocation through
ntfs_ia_add() when the root is a small index. For a root that is already
marked as a large index it calls ntfs_ib_write() directly.

The lookup does not open the index allocation either when it ends in the
root. If the last entry of the root has no child node, ia_ni is still
NULL when the root has to be moved into an index block, and the write
dereferences it:

  Oops: general protection fault, KASAN: null-ptr-deref in range
        [0x0000000000000128-0x000000000000012f]
  RIP: 0010:ntfs_inode_attr_pwrite+0x52/0x1600
   ntfs_ir_make_space
   ntfs_ie_add
   ntfs_index_add_filename
   __ntfs_link
   ntfs_link
   vfs_link

A single byte of an image from mkntfs is enough. In a directory that has
one empty index block, clear INDEX_ENTRY_NODE (flags 3 -> 2) of the last
entry of the $INDEX_ROOT and link() a file into the directory.

Commit 9365c4a1357e ("ntfs: fix NULL dereference on index allocation open failure")
covers the case where ntfs_ia_open() fails in the lookup. Here it was
never tried. Open the index allocation on demand in both
helpers and fail with -EIO if that does not work.

ntfs_ia_open() also succeeds for a directory that has no
$INDEX_ALLOCATION attribute. The inode it returns is then an empty resident
stub that does not describe an index allocation, so reject that case too.

Fixes: 0a8ac0c1fa0b ("ntfs: update directory operations")
Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
---
 fs/ntfs/index.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/fs/ntfs/index.c b/fs/ntfs/index.c
index 322cb371d..74716f916 100644
--- a/fs/ntfs/index.c
+++ b/fs/ntfs/index.c
@@ -99,12 +99,30 @@ static s64 ntfs_ib_pos_to_vcn(struct ntfs_index_context *icx, s64 pos)
 	return pos >> icx->vcn_size_bits;
 }
 
+/* Open the index allocation inode if no earlier step of the operation did. */
+static int ntfs_icx_ia_ensure(struct ntfs_index_context *icx)
+{
+	if (!icx->ia_ni) {
+		icx->ia_ni = ntfs_ia_open(icx, icx->idx_ni);
+		if (!icx->ia_ni)
+			return -EIO;
+	}
+	/* Without $INDEX_ALLOCATION the index inode is an empty resident stub. */
+	if (!NInoNonResident(icx->ia_ni))
+		return -EIO;
+	return 0;
+}
+
 static int ntfs_ib_write(struct ntfs_index_context *icx, struct index_block *ib)
 {
 	s64 ret, vcn = le64_to_cpu(ib->index_block_vcn);
 
 	ntfs_debug("vcn: %lld\n", vcn);
 
+	ret = ntfs_icx_ia_ensure(icx);
+	if (ret)
+		return ret;
+
 	ret = pre_write_mst_fixup((struct ntfs_record *)ib, icx->block_size);
 	if (ret)
 		return -EIO;
@@ -805,6 +823,10 @@ static int ntfs_ib_read(struct ntfs_index_context *icx, s64 vcn, struct index_bl
 
 	ntfs_debug("vcn: %lld\n", vcn);
 
+	ret = ntfs_icx_ia_ensure(icx);
+	if (ret)
+		return ret;
+
 	pos = ntfs_ib_vcn_to_pos(icx, vcn);
 
 	ret = ntfs_inode_attr_pread(VFS_I(icx->ia_ni), pos, icx->block_size, (u8 *)dst);
-- 
2.53.0

[PATCH v2 3/3] ntfs: do not convert a named $DATA stream when making room in a record

[Wahid Khan]

When an attribute has to grow and the mft record is full,
ntfs_resident_attr_resize() walks the other resident attributes and
converts them to non-resident ones. For $DATA it always opens the
unnamed data inode of the base inode, even if the attribute found is a
named stream:

  tvi = ntfs_iget(sb, base_ni->mft_no);
  ntfs_attr_make_non_resident(NTFS_I(tvi), value_len);

value_len is the length of the stream, so it does not match the
attribute that ntfs_attr_make_non_resident() looks up for the unnamed
inode, and the WARN_ON() on entry fires:

  WARNING: fs/ntfs/attrib.c:2115 at ntfs_attr_make_non_resident
   ntfs_resident_attr_resize
   ntfs_attr_expand_locked
   ntfs_expand_for_write
   ntfs_file_write_iter
   vfs_write

With debug output the values were attr_size 13 (unnamed $DATA) and
data_size 550 (named stream) for the same mft record.

It does not need a damaged volume. A file with an alternate data stream
whose record has no room left is enough. The fuzzer found it through a
single changed byte in a file name of an image from mkntfs, which made
a write grow a resident attribute in such a record.

Opening the stream by name instead would convert it with only the mft
record lock held. Its folio would be locked under that lock, while the
read path takes the folio lock first and the mft record lock inside
ntfs_read_iomap_begin_resident(). Streams are not converted from this
loop, which only tries to make room, so skip them.

Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
---
 fs/ntfs/attrib.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index ce7c1b524..800e53df5 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -4932,6 +4932,12 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
 			continue;
 		if (a->type == AT_DATA && !value_len)
 			continue;
+		/*
+		 * Named streams have inodes of their own and are not converted
+		 * from here.
+		 */
+		if (a->type == AT_DATA && a->name_length)
+			continue;
 
 		if (a->type == AT_DATA)
 			tvi = ntfs_iget(sb, base_ni->mft_no);
-- 
2.53.0

Re: [PATCH v2 3/3] ntfs: do not convert a named $DATA stream when making room in a record

[Wahid Khan]

Hi Namjae,

The Sashiko review of 3/3 also suggests skipping every AT_DATA
attribute in this loop, not only the named ones. I'd keep the unnamed
case as it is, for three reasons:

- Converting the unnamed $DATA from this loop is what the loop already
  did before the patch. The patch only stops it from pairing the length
  of a named stream with the unnamed inode.

- The lock order the review points at (mrec_lock held while
  ntfs_attr_make_non_resident() locks folio 0) is the same one used by
  the direct conversion near the top of ntfs_resident_attr_resize(),
  before the loop. So it is not specific to the loop.

- When an EA or a stream grows in a full record, converting the unnamed
  $DATA is what frees the space. Skipping all of $DATA would turn those
  writes into failures.

If you'd rather drop the unnamed case from the loop too, I can send
that as a separate patch so the two changes can be judged on their own.

Thanks,
Wahid

Re: [PATCH v2 0/3] ntfs: three fixes for crafted images, found by fuzzing

[Namjae Jeon]

On Fri, Nov 6, 2026 at 9:46 PM Wahid Khan <wahidzk0091@gmail.com> wrote:
>
> I fuzz the new fs/ntfs driver with mutated images: an image from mkntfs
> populated with ntfs-3g, a few bytes changed, then mount, walk the tree
> and write, under KASAN. These three problems are still present in
> ntfs-next (e62b384ca). Each patch has its trace and reproduction steps.
>
> 1. ntfs: do not read an index block outside $INDEX_ALLOCATION
>    A directory without a usable $INDEX_ALLOCATION hangs on its first
>    lookup. The task waits for an mft record lock that it already holds.
> 2. ntfs: open the index allocation before reading or writing an index block
>    NULL pointer dereference in ntfs_ib_write() on link().
> 3. ntfs: do not convert a named $DATA stream when making room in a record
>    WARN_ON() in ntfs_attr_make_non_resident(); no damaged volume needed.
>
> All three need an image to be mounted, so a crafted image, not a remote
> attacker.
>
> Changes in v2, after an automated review of v1 that I checked by hand:
> - 2/3: ntfs_ia_open() also succeeds for a directory without
>   $INDEX_ALLOCATION and returns an empty resident inode. Reject an index
>   inode that is not non-resident, as 1/3 already does.
> - 3/3: v1 opened the named stream and converted it. That would lock the
>   stream's folio under the mft record lock, the reverse of the read
>   path (ntfs_read_iomap_begin_resident()). v2 skips named streams in
>   that loop instead.
> - 1/3: unchanged.
>
> v1: https://lore.kernel.org/all/20261106091420.2461683-1-wahidzk0091@gmail.com/
>
>
> Wahid Khan (3):
>   ntfs: do not read an index block outside $INDEX_ALLOCATION
>   ntfs: open the index allocation before reading or writing an index
>     block
>   ntfs: do not convert a named $DATA stream when making room in a record
Applied it to #ntfs-next.
Thanks!

Re: [PATCH v2 3/3] ntfs: do not convert a named $DATA stream when making room in a record

[Namjae Jeon]

On Fri, Nov 6, 2026 at 10:31 PM Wahid Khan <wahidzk0091@gmail.com> wrote:
>
> Hi Namjae,
Hi Wahid,
>
> The Sashiko review of 3/3 also suggests skipping every AT_DATA
> attribute in this loop, not only the named ones. I'd keep the unnamed
> case as it is, for three reasons:
>
> - Converting the unnamed $DATA from this loop is what the loop already
>   did before the patch. The patch only stops it from pairing the length
>   of a named stream with the unnamed inode.
>
> - The lock order the review points at (mrec_lock held while
>   ntfs_attr_make_non_resident() locks folio 0) is the same one used by
>   the direct conversion near the top of ntfs_resident_attr_resize(),
>   before the loop. So it is not specific to the loop.
>
> - When an EA or a stream grows in a full record, converting the unnamed
>   $DATA is what frees the space. Skipping all of $DATA would turn those
>   writes into failures.
>
> If you'd rather drop the unnamed case from the loop too, I can send
> that as a separate patch so the two changes can be judged on their own.
I think making the lock ordering consistent and using ntfs_attr_iget()
for named $DATA would be the right approach. Please check it if you
have the time.
Thanks for the patches!

[PATCH v3] ntfs: open a named $DATA stream by name when making room in a record

[Wahid Khan]

When an attribute has to grow and the mft record is full,
ntfs_resident_attr_resize() walks the other resident attributes and
converts them to non-resident ones. For $DATA it always opens the
unnamed data inode of the base inode, even if the attribute found is a
named stream:

  tvi = ntfs_iget(sb, base_ni->mft_no);
  ntfs_attr_make_non_resident(NTFS_I(tvi), value_len);

value_len is the length of the stream, so it does not match the
attribute that ntfs_attr_make_non_resident() looks up for the unnamed
inode, and the WARN_ON() on entry fires:

  WARNING: fs/ntfs/attrib.c:2115 at ntfs_attr_make_non_resident
   ntfs_resident_attr_resize
   ntfs_attr_expand_locked
   ntfs_expand_for_write
   ntfs_file_write_iter
   vfs_write

With debug output the values were attr_size 13 (unnamed $DATA) and
data_size 550 (named stream) for the same mft record.

It does not need a damaged volume. A file with an alternate data stream
whose record has no room left is enough. The fuzzer found it through a
single changed byte in a file name of an image from mkntfs, which made
a write grow a resident attribute in such a record.

Use ntfs_attr_iget() with the name of the attribute for a named $DATA
stream, as is already done for the other attribute types in this loop,
and keep ntfs_iget() for the unnamed one.

Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
---
v3: open a named $DATA stream with ntfs_attr_iget(), as suggested, instead
    of skipping it (v2). Same change as v1 of this patch.

 fs/ntfs/attrib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index ce7c1b524180..c92d1b06afc3 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -4933,7 +4933,7 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
 		if (a->type == AT_DATA && !value_len)
 			continue;
 
-		if (a->type == AT_DATA)
+		if (a->type == AT_DATA && !a->name_length)
 			tvi = ntfs_iget(sb, base_ni->mft_no);
 		else
 			tvi = ntfs_attr_iget(VFS_I(base_ni), a->type,
-- 
2.56.0

Re: [PATCH v2 3/3] ntfs: do not convert a named $DATA stream when making room in a record

[Wahid Khan]

Hi Namjae,

I sent v3 of this patch as a reply to this mail. It opens a named $DATA
stream with ntfs_attr_iget() like the other attribute types in the loop,
and keeps ntfs_iget() for the unnamed one. With it the image that
triggered the WARN mounts and walks cleanly on a KASAN and lockdep
kernel. Without it the WARN fires at ntfs_attr_make_non_resident().

About the lock ordering: I checked the code. The comment above the
__filemap_get_folio() call in ntfs_attr_make_non_resident() says the
folio lock nests outside all ntfs locks, but its callers hold mrec_lock
when they get there (ntfs_expand_for_write() takes it in file.c and goes
through ntfs_attr_expand() into ntfs_resident_attr_resize()). The read
side takes the folio first and mrec_lock inside
ntfs_read_iomap_begin_resident(). So the two orders do exist. This is
true for the direct conversion at the top of ntfs_resident_attr_resize()
as well, not only for the loop.

I could not turn it into a hang. A reader and a writer racing on resident
files, also with a delay forced between taking mrec_lock and the folio,
completed every time. So I do not know how reachable it is, and I did
not want to change the locking in this patch on a guess.

If you want the order fixed, I think it needs the folio to be taken
before mrec_lock in the callers (or a non-blocking folio get on the
make-room path, which only tries to free space). Tell me which you
prefer and I will send it as a separate patch.

Thanks,
Wahid

Re: [PATCH v3] ntfs: open a named $DATA stream by name when making room in a record

[Namjae Jeon]

On Sun, Nov 8, 2026 at 4:28 AM Wahid Khan <wahidzk0091@gmail.com> wrote:
>
> When an attribute has to grow and the mft record is full,
> ntfs_resident_attr_resize() walks the other resident attributes and
> converts them to non-resident ones. For $DATA it always opens the
> unnamed data inode of the base inode, even if the attribute found is a
> named stream:
>
>   tvi = ntfs_iget(sb, base_ni->mft_no);
>   ntfs_attr_make_non_resident(NTFS_I(tvi), value_len);
>
> value_len is the length of the stream, so it does not match the
> attribute that ntfs_attr_make_non_resident() looks up for the unnamed
> inode, and the WARN_ON() on entry fires:
>
>   WARNING: fs/ntfs/attrib.c:2115 at ntfs_attr_make_non_resident
>    ntfs_resident_attr_resize
>    ntfs_attr_expand_locked
>    ntfs_expand_for_write
>    ntfs_file_write_iter
>    vfs_write
>
> With debug output the values were attr_size 13 (unnamed $DATA) and
> data_size 550 (named stream) for the same mft record.
>
> It does not need a damaged volume. A file with an alternate data stream
> whose record has no room left is enough. The fuzzer found it through a
> single changed byte in a file name of an image from mkntfs, which made
> a write grow a resident attribute in such a record.
>
> Use ntfs_attr_iget() with the name of the attribute for a named $DATA
> stream, as is already done for the other attribute types in this loop,
> and keep ntfs_iget() for the unnamed one.
>
> Fixes: 495e90fa3348 ("ntfs: update attrib operations")
> Cc: stable@vger.kernel.org # v7.1+
> Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
> ---
> v3: open a named $DATA stream with ntfs_attr_iget(), as suggested, instead
>     of skipping it (v2). Same change as v1 of this patch.
>
>  fs/ntfs/attrib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index ce7c1b524180..c92d1b06afc3 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -4933,7 +4933,7 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
>                 if (a->type == AT_DATA && !value_len)
>                         continue;
>
> -               if (a->type == AT_DATA)
> +               if (a->type == AT_DATA && !a->name_length)
>                         tvi = ntfs_iget(sb, base_ni->mft_no);
>                 else
>                         tvi = ntfs_attr_iget(VFS_I(base_ni), a->type,
This can still deadlock with a concurrent read of the named stream ?

Thread A: grow unnamed $DATA          Thread B: read named $DATA
--------------------------------
-----------------------------
hold unnamed inode i_rwsem               hold named inode i_rwsem
hold base mrec_lock                             hold named stream folio 0

ntfs_attr_iget(..., stream_name, ...)
ntfs_attr_make_non_resident()
wait for named stream folio 0                wait for base mrec_lock

Thanks.

[PATCH v4] ntfs: convert a named $DATA stream safely when making room in a record

[Wahid Khan]

When an attribute has to grow and the mft record is full,
ntfs_resident_attr_resize() walks the other resident attributes and
converts them to non-resident ones. For $DATA it always opens the
unnamed data inode of the base inode, even if the attribute found is a
named stream:

  tvi = ntfs_iget(sb, base_ni->mft_no);
  ntfs_attr_make_non_resident(NTFS_I(tvi), value_len);

value_len is the length of the stream, so it does not match the
attribute that ntfs_attr_make_non_resident() looks up for the unnamed
inode, and the WARN_ON() on entry fires:

  WARNING: fs/ntfs/attrib.c:2115 at ntfs_attr_make_non_resident
   ntfs_resident_attr_resize
   ntfs_attr_expand_locked
   ntfs_expand_for_write
   ntfs_file_write_iter
   vfs_write

It does not need a damaged volume. A file with an alternate data stream
whose record has no room left is enough.

Open a named stream by name with ntfs_attr_iget(), as is done for the
other attribute types in this loop, and keep ntfs_iget() for the
unnamed one. The loop runs with the mft record lock held and takes no
lock of the stream inode, so converting a stream from here needs more
care than the plain lookup gives:

- A reader of the stream holds the folio lock and then takes the mft
  record lock in ntfs_read_iomap_begin_resident(), the conversion takes
  them the other way round and waits for folio 0. Take the folio with
  FGP_NOWAIT in this loop and skip the attribute if it is busy. The loop
  only tries to free space.

- A stream that is in the inode cache can be open, being written, being
  removed or already unlinked. A write that passed its residency check
  before the conversion carries on with a non-resident attribute, and
  a stream in the middle of ntfs_stream_finish_remove() is converted
  under its feet. Convert only a stream that is not cached, and none of
  a file that is being deleted or has no links left.

- The lookup of a stream ignores case, so a stream whose name differs
  only in case from an earlier one resolves to that one and the length
  does not match again. Compare the names exactly.

Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org # v7.1+
Signed-off-by: Wahid Khan <wahidzk0091@gmail.com>
---
v4: v3 (convert a named stream via ntfs_attr_iget()) can deadlock against
    a reader of the stream and has other races with an open or removed
    stream. Each is reproduced with a forced make-room pass and a debug
    delay, fails on v3 and passes here: reader/converter deadlock
    (hung task), write continuing on a converted attribute (iomap_iter
    WARNINGs, recursive locking), unlinked file with an open stream (mft
    record reported corrupt), stream converted during its own removal,
    and a stream pair that differs only in case (WARN in
    ntfs_attr_make_non_resident).
v3: use ntfs_attr_iget() for a named stream instead of skipping it (v2).

 fs/ntfs/attrib.c | 57 +++++++++++++++++++++++++++++++++++++++++-------
 fs/ntfs/inode.c  | 19 ++++++++++++++++
 fs/ntfs/inode.h  |  2 ++
 3 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index ce7c1b524180..c824bd3ea9fa 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -1975,7 +1975,21 @@ int ntfs_resident_attr_value_resize(struct mft_record *m, struct attr_record *a,
  * inode i_size is that this is not necessarily uptodate.  This happens when
  * ntfs_attr_make_non_resident() is called in the ->truncate call path(s).
  */
+static int __ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size,
+		bool nowait);
+
 int ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size)
+{
+	return __ntfs_attr_make_non_resident(ni, data_size, false);
+}
+
+/*
+ * With @nowait the folio lock is only tried: a caller that holds mrec_lock
+ * must not wait for a folio lock, the read path takes them the other way
+ * round. Fails with -EAGAIN if the folio is locked.
+ */
+static int __ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size,
+		bool nowait)
 {
 	s64 new_size;
 	struct inode *vi = VFS_I(ni);
@@ -2056,10 +2070,11 @@ int ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size)
 		 * outside all ntfs locks, we need to get the folio now.
 		 */
 		folio = __filemap_get_folio(vi->i_mapping, 0,
-					    FGP_CREAT | FGP_LOCK,
+					    FGP_CREAT | FGP_LOCK |
+					    (nowait ? FGP_NOWAIT : 0),
 					    mapping_gfp_mask(vi->i_mapping));
 		if (IS_ERR(folio)) {
-			err = -ENOMEM;
+			err = nowait ? -EAGAIN : -ENOMEM;
 			goto err_out;
 		}
 
@@ -4933,18 +4948,44 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
 		if (a->type == AT_DATA && !value_len)
 			continue;
 
-		if (a->type == AT_DATA)
+		if (a->type == AT_DATA && !a->name_length) {
 			tvi = ntfs_iget(sb, base_ni->mft_no);
-		else
-			tvi = ntfs_attr_iget(VFS_I(base_ni), a->type,
-				(__le16 *)((u8 *)a + le16_to_cpu(a->name_offset)),
-				a->name_length);
+		} else {
+			__le16 *name = (__le16 *)((u8 *)a +
+					le16_to_cpu(a->name_offset));
+
+			/*
+			 * A named $DATA stream is converted only if nobody has
+			 * it open: an open one can be unlinked, being removed
+			 * or in use. Its file must not be going away either.
+			 */
+			if (a->type == AT_DATA) {
+				if (NInoBeingDeleted(base_ni) ||
+				    !VFS_I(base_ni)->i_nlink)
+					continue;
+				tvi = ntfs_attr_ilookup(VFS_I(base_ni), a->type,
+						name, a->name_length);
+				if (tvi) {
+					iput(tvi);
+					continue;
+				}
+			}
+			tvi = ntfs_attr_iget(VFS_I(base_ni), a->type, name,
+					a->name_length);
+			/* the lookup ignores case for streams, ours does not */
+			if (!IS_ERR(tvi) && a->type == AT_DATA &&
+			    memcmp(NTFS_I(tvi)->name, name,
+				   a->name_length * sizeof(__le16))) {
+				iput(tvi);
+				continue;
+			}
+		}
 		if (IS_ERR(tvi)) {
 			ntfs_error(sb, "Couldn't open attribute");
 			continue;
 		}
 
-		if (ntfs_attr_make_non_resident(NTFS_I(tvi), value_len)) {
+		if (__ntfs_attr_make_non_resident(NTFS_I(tvi), value_len, true)) {
 			iput(tvi);
 			continue;
 		}
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 62dd5bd5bb81..ec9c7b219192 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -287,6 +287,25 @@ struct inode *ntfs_attr_iget(struct inode *base_vi, __le32 type,
 	return vi;
 }
 
+/*
+ * ntfs_attr_ilookup - find an attribute inode that is already in the cache
+ *
+ * Like ntfs_attr_iget() but never reads anything. Returns the inode with an
+ * increased reference count, or NULL if it is not cached.
+ */
+struct inode *ntfs_attr_ilookup(struct inode *base_vi, __le32 type,
+		__le16 *name, u32 name_len)
+{
+	struct ntfs_attr na = {
+		.mft_no = base_vi->i_ino,
+		.type = type,
+		.name = name,
+		.name_len = name_len,
+	};
+
+	return ilookup5(base_vi->i_sb, na.mft_no, ntfs_test_inode, &na);
+}
+
 /*
  * ntfs_index_iget - obtain a struct inode corresponding to an index
  * @base_vi:	vfs base inode containing the index related attributes
diff --git a/fs/ntfs/inode.h b/fs/ntfs/inode.h
index a18cbcf1c631..7ea52c59c04b 100644
--- a/fs/ntfs/inode.h
+++ b/fs/ntfs/inode.h
@@ -326,6 +326,8 @@ struct ntfs_attr {
 int ntfs_test_inode(struct inode *vi, void *data);
 int ntfs_test_inode_rcu(struct inode *vi, void *data);
 struct inode *ntfs_iget(struct super_block *sb, u64 mft_no);
+struct inode *ntfs_attr_ilookup(struct inode *base_vi, __le32 type,
+		__le16 *name, u32 name_len);
 struct inode *ntfs_attr_iget(struct inode *base_vi, __le32 type,
 		__le16 *name, u32 name_len);
 struct inode *ntfs_index_iget(struct inode *base_vi, __le16 *name,
-- 
2.56.0

Re: [PATCH v2 3/3] ntfs: do not convert a named $DATA stream when making room in a record

[Wahid Khan]

Hi,

You were right, v3 deadlocks. I reproduced your diagram: with a debug
delay before the folio lock in ntfs_attr_make_non_resident(), a reader
of the stream and the make-room loop hang the kernel (hung task, lockdep
shows the writer holding the unnamed inode's i_rwsem and mrec_lock and
waiting for folio 0, the reader holding the stream's folio 0 and waiting
for mrec_lock). The same direct conversion of the inode being written
is safe because the reader of that inode waits on its i_rwsem first.

An automated review of v3 also listed other problems with converting a
stream from this loop. I could reproduce most of them, with a forced
make-room pass and debug delays:

- a write to the stream that passed its residency check before the
  conversion carries on with a non-resident attribute (iomap_iter
  WARNINGs, "possible recursive locking", hang)
- an unlinked file with an open stream: "mft 0x1b, type 0x80 is
  corrupt. Run chkdsk." and the next write to the stream fails
- a stream that is being removed is converted under
  ntfs_stream_finish_remove() (unlink returned 1)
- two streams whose names differ only in case: WARN in
  ntfs_attr_make_non_resident(), the lookup ignores case

v4 keeps ntfs_attr_iget() for the named stream and adds what is needed:
FGP_NOWAIT for the folio in this loop (skip when busy), convert a
stream only if it is not in the inode cache and its file is not being
deleted or unlinked, and compare the stream name exactly. All of the
cases above fail on v3 and pass with v4. The WARN from the original
image is fixed.

I could not show data loss on unlink, only that the unsafe state is
reached. The debug delays are mine, so these windows are narrow without
them. If you think this is more than the loop should
carry, the one-line v2 (skip named streams) also fixes the WARN.

Thanks,
Wahid