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

[Namjae Jeon]

On Sun, Nov 8, 2026 at 5:03 PM Wahid Khan <wahidzk0091@xxxxxxxxx> 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
>
> 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@xxxxxxxxxxxxxxx # v7.1+
> Signed-off-by: Wahid Khan <wahidzk0091@xxxxxxxxx>
> ---
> 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)) {
Can you check Sashiko's review comments?

https://sashiko.dev/#/patchset/20261108080323.468892-1-wahidzk0091%40gmail.com

Thanks.