Re: [PATCH 0/4] ntfs: serialize attribute-list access

Hi,

I tested this series using my original reproducer/PoC for the reported bug,
and also ran a focused set of generic xfstests write/fsx cases.
Everything looks good to me.

Reported-by: Cen Zhang <[email protected]>
Tested-by: Cen Zhang <[email protected]>

Best regards,
Cen Zhang


Hyunchul Lee <[email protected]> =E4=BA=8E2026=E5=B9=B47=E6=9C=8827=E6=97=
=A5=E5=91=A8=E4=B8=80 08:59=E5=86=99=E9=81=93=EF=BC=9A
>
> ntfs_external_attr_find() walks base_ni->attr_list using raw
> attr_list_entry pointers stored in ctx->al_entry.  Because other paths
> can replace base_ni->attr_list and free the old buffer while a lookup
> still holds one of those cursors, a concurrent attribute-list mutation
> can turn a saved cursor into a stale or freed pointer.
>
> Reported in:
>   https://lore.kernel.org/all/[email protected]=
m/
>
> This series fixes that hazard by replacing the raw ctx->al_entry
> pointer with restartable, generation-checked locators, and by adding
> locks that serialize attribute-list buffer replacement and on-disk
> persistence so concurrent mutations cannot corrupt state or double-free
> memory.
>
> The four patches are:
>
>   1. ntfs: replace raw ctx->al_entry with restartable attr-list locators
>   2. ntfs: protect attribute-list buffer replacement with
>      attr_list_persist_lock
>   3. ntfs: protect attribute-list creation/teardown with
>      attr_list_persist_lock
>   4. ntfs: protect mapping-pairs attr-list updates with
>      attr_list_persist_lock
>
> What changes
> ------------
>
> Instead of a raw struct attr_list_entry *, the search context now
> carries three value-only locators:
>
>   - al_cursor  -- where ntfs_external_attr_find() resumes enumeration;
>                   invalidated on generation mismatch.
>   - al_insert  -- insertion point captured on -ENOENT lookups.
>   - al_exact   -- immutable identity snapshot (type, lowest_vcn,
>                   mft_reference, instance, name length) used by writers
>                   to re-locate the same ALE after a buffer replacement.
>
> The inode gains an rw_semaphore, attr_list_lock, and a 64-bit
> attr_list_gen counter.  The lock protects attr_list, attr_list_size,
> and attr_list_gen; the counter is bumped on every buffer replacement
> so a stale locator is detected before it is dereferenced.
>
> Because ntfs_attrlist_update() can sleep and recurse back into
> attribute lookups, attr_list_lock cannot be held across persistence.
> A separate mutex, attr_list_persist_lock, serializes the whole
> publish-buffer -> persist-to-disk -> rollback-on-failure transaction
> for each mutator.
>
> Lock ordering is:
>
>     mrec_lock -> attr_list_persist_lock -> attr_list_lock
>
> attr_list_lock is never held across anything that can take runlist.lock
> or extent_lock.
>
> Signed-off-by: Hyunchul Lee <[email protected]>
> ---
> Hyunchul Lee (4):
>       ntfs: replace raw ctx->al_entry with restartable attr-list locators
>       ntfs: protect attribute-list buffer replacement with attr_list_pers=
ist_lock
>       ntfs: protect attribute-list creation/teardown with attr_list_persi=
st_lock
>       ntfs: protect mapping-pairs attr-list updates with attr_list_persis=
t_lock
>
>  fs/ntfs/attrib.c   | 411 ++++++++++++++++++++++++++++++++++++++++++++---=
------
>  fs/ntfs/attrib.h   |  46 +++++-
>  fs/ntfs/attrlist.c | 139 ++++++++++++++----
>  fs/ntfs/index.c    |   3 +-
>  fs/ntfs/inode.c    |  69 +++++++--
>  fs/ntfs/inode.h    |   9 ++
>  6 files changed, 561 insertions(+), 116 deletions(-)
> ---
> base-commit: f5098b6bae761e346ebcd9da7f95622c04733cff
> change-id: 20260727-topic-attr-list-lock-v6-f1a909e3c859
>
> Best regards,
> --
> Hyunchul Lee <[email protected]>
>