* [PATCH v7 0/4] ntfs: serialize attribute-list access
@ 2026-08-12 1:18 Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 1/4] ntfs: replace raw ctx->al_entry with restartable attr-list locators Hyunchul Lee
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Hyunchul Lee @ 2026-08-12 1:18 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, ntfs, Cen Zhang, Copilot, stable
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/20260702161710.2606853-1-zzzccc427@gmail.com/
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 attr_list_lock, an rwsem that protects the live buffer,
its size and generation, and in-place ALE changes. Readers hold it
only while dereferencing the buffer; writers hold it while replacing the
buffer or changing an ALE, then increment the generation. Pointers into
the buffer never survive an unlock, and the rwsem is dropped before
persistence or an operation that can take runlist.lock or extent_lock.
attr_list_persist_lock is a non-recursive mutex that serializes a
mutation through persistence and rollback, including list creation and
teardown. ntfs_attrlist_update_locked() writes the live buffer without
attr_list_lock, so the caller holds this mutex from mutation through the
write. It also stabilizes the buffer while attaching extents. When both
locks are needed, take attr_list_persist_lock first. Creation owns the
mutex itself, so callers must create first and then restart; re-entrant
teardown uses persist_locked instead of recursively acquiring it.
---
* Changes in v7:
* Patch 3: serialize non-$ATTRIBUTE_LIST removal with concurrent
attribute-list creation.
* Patch 3: initialize successful removal results and release
attr_list_persist_lock on every early creation error.
* Changes in v6:
* Patch 2: initialize new_al before an early error path can reach its
cleanup, fixing an x86_64 allmodconfig Clang build warning.
* Changes in v5:
* Patch 3: keep each in-place ALE update and its persist in one
transaction. This fixes the Patch 2 review: a record move could
update an ALE while persistence copied the same buffer.
* Patch 3: serialize attribute-list creation, teardown, and extent
attachment.
* Patch 4: acquire the mutex unconditionally for mapping-pairs updates to
close the attribute-list creation TOCTOU; drop it before creating a
missing list and restart afterward to avoid self-deadlock.
* Changes in v4:
* Patch 1: fix attr-list lock/unlock and uninitialized locator on
the AT_END path
* Patch 2: hold attr_list_persist_lock around all
ntfs_attrlist_update() callers and recheck the attribute-list
state after locking
* Patch 4: avoid self-deadlock rebuilding $ATTRIBUTE_LIST's own
mapping pairs
* Changes in v3:
* Patch 1: fix $ATTRIBUTE_LIST extent double-delivery during
mapping-pairs rebuild (xfstests generic/040)
* Patch 3: fix self-deadlock on attr_list_persist_lock during
attr-list rebuild (xfstests generic/040)
* Changes in v2:
* add Fixes tags
* add Reported-by and Tested-by tags
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
Hyunchul Lee (4):
ntfs: replace raw ctx->al_entry with restartable attr-list locators
ntfs: protect attribute-list buffer replacement with attr_list_persist_lock
ntfs: protect attribute-list creation/teardown with attr_list_persist_lock
ntfs: protect mapping-pairs attr-list updates with attr_list_persist_lock
fs/ntfs/attrib.c | 568 ++++++++++++++++++++++++++++++++++++++++++++---------
fs/ntfs/attrib.h | 48 ++++-
fs/ntfs/attrlist.c | 181 +++++++++++++----
fs/ntfs/attrlist.h | 3 +-
fs/ntfs/index.c | 26 ++-
fs/ntfs/inode.c | 116 ++++++++---
fs/ntfs/inode.h | 10 +
fs/ntfs/namei.c | 2 +-
fs/ntfs/super.c | 2 +-
9 files changed, 795 insertions(+), 161 deletions(-)
---
base-commit: 5c244a0e9ca95468f4c2087b2c9269825429a13f
change-id: 20260727-topic-attr-list-lock-v6-f1a909e3c859
Best regards,
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v7 1/4] ntfs: replace raw ctx->al_entry with restartable attr-list locators
2026-08-12 1:18 [PATCH v7 0/4] ntfs: serialize attribute-list access Hyunchul Lee
@ 2026-08-12 1:18 ` Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 2/4] ntfs: protect attribute-list buffer replacement with attr_list_persist_lock Hyunchul Lee
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Hyunchul Lee @ 2026-08-12 1:18 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, ntfs, Cen Zhang, Copilot, stable
ntfs_attr_search_ctx kept a raw al_entry pointer into the in-memory
$ATTRIBUTE_LIST buffer. Any insertion or removal reallocates the
whole buffer, so that pointer becomes stale while the context is still
alive across extent mapping, record moves, and mapping-pair rebuilds.
Replace the raw pointer with three generation-checked locators:
- al_cursor: resume position for ntfs_external_attr_find()
- al_insert: insertion point captured on -ENOENT
- al_exact: identity snapshot used by writers to re-find an entry
Add attr_list_lock (rw_semaphore) and attr_list_gen to protect the
in-memory list and detect stale locators. ntfs_external_attr_find()
takes the lock for read. Writers re-locate under the lock for write
via ntfs_attrlist_find_exact_locked() before mutating.
Reported-by: Cen Zhang <zzzccc427@gmail.com>
Link: https://lore.kernel.org/all/20260702161710.2606853-1-zzzccc427@gmail.com/
Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org
Tested-by: Cen Zhang <zzzccc427@gmail.com>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
fs/ntfs/attrib.c | 377 +++++++++++++++++++++++++++++++++++++++++++----------
fs/ntfs/attrib.h | 44 ++++++-
fs/ntfs/attrlist.c | 23 ++--
fs/ntfs/index.c | 3 +-
fs/ntfs/inode.c | 34 +++--
fs/ntfs/inode.h | 6 +
6 files changed, 391 insertions(+), 96 deletions(-)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index c62c8ca8b987..0f48dd923b72 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -30,6 +30,97 @@
__le16 AT_UNNAMED[] = { cpu_to_le16('\0') };
+void ntfs_attrlist_reset_locators(struct ntfs_attr_search_ctx *ctx)
+{
+ ctx->al_cursor = (struct ntfs_attrlist_cursor){ 0 };
+ ctx->al_insert = (struct ntfs_attrlist_anchor){ 0 };
+ ctx->al_exact = (struct ntfs_attrlist_exact){ 0 };
+}
+
+void ntfs_attrlist_capture_exact(struct ntfs_attr_search_ctx *ctx,
+ struct ntfs_inode *base_ni,
+ struct attr_list_entry *ale, u8 *al_start)
+{
+ ctx->al_exact.off = (u8 *)ale - al_start;
+ ctx->al_exact.gen = base_ni->attr_list_gen;
+ ctx->al_exact.valid = true;
+ ntfs_attrlist_exact_key_from_ale(&ctx->al_exact.key, ale);
+}
+
+static void ntfs_attrlist_capture_insert(struct ntfs_attr_search_ctx *ctx,
+ struct ntfs_inode *base_ni,
+ struct attr_list_entry *ale,
+ u8 *al_start, u8 *al_end)
+{
+ ctx->al_insert.off = (u8 *)ale - al_start;
+ ctx->al_insert.gen = base_ni->attr_list_gen;
+ ctx->al_insert.valid = true;
+ ctx->al_insert.at_end = ((u8 *)ale == al_end);
+}
+
+void ntfs_attrlist_exact_key_from_ale(struct ntfs_attrlist_exact_key *key,
+ const struct attr_list_entry *ale)
+{
+ key->type = ale->type;
+ key->lowest_vcn = ale->lowest_vcn;
+ key->mft_reference = ale->mft_reference;
+ key->instance = ale->instance;
+ key->name_len = ale->name_length;
+}
+
+bool ntfs_attrlist_exact_key_eq(const struct attr_list_entry *ale,
+ const struct ntfs_attrlist_exact_key *key)
+{
+ if (ale->type != key->type)
+ return false;
+ if (ale->lowest_vcn != key->lowest_vcn)
+ return false;
+ if (ale->mft_reference != key->mft_reference)
+ return false;
+ if (ale->instance != key->instance)
+ return false;
+
+ return ale->name_length == key->name_len;
+}
+
+static struct attr_list_entry *ntfs_attrlist_find_exact_locked(struct ntfs_inode *base_ni,
+ struct ntfs_attrlist_exact *exact)
+{
+ struct attr_list_entry *ale;
+ u8 *al_end;
+
+ if (!exact->valid || !base_ni->attr_list)
+ return NULL;
+
+ al_end = base_ni->attr_list + base_ni->attr_list_size;
+ if (exact->gen == base_ni->attr_list_gen &&
+ exact->off < base_ni->attr_list_size) {
+ ale = (struct attr_list_entry *)(base_ni->attr_list + exact->off);
+
+ if (ntfs_attr_list_entry_is_valid(ale, al_end) &&
+ ntfs_attrlist_exact_key_eq(ale, &exact->key))
+ return ale;
+ }
+
+ for (ale = (struct attr_list_entry *)base_ni->attr_list;
+ ntfs_attr_list_entry_is_valid(ale, al_end);
+ ale = (struct attr_list_entry *)((u8 *)ale +
+ le16_to_cpu(ale->length))) {
+ if (ntfs_attrlist_exact_key_eq(ale, &exact->key))
+ return ale;
+ }
+
+ return NULL;
+}
+
+static struct ntfs_inode *ntfs_attr_ctx_base_ni(struct ntfs_attr_search_ctx *ctx)
+{
+ if (ctx->ntfs_ino->nr_extents == -1)
+ return ctx->base_ntfs_ino;
+
+ return ctx->ntfs_ino;
+}
+
/*
* Maximum size allowed for reading attributes by ntfs_attr_readall().
* Extended attribute, reparse point are not expected to be larger than this size.
@@ -1156,6 +1247,7 @@ static int ntfs_external_attr_find(const __le32 type,
u32 al_name_len;
u32 attr_len, mft_free_len;
bool is_first_search = false;
+ bool attr_list_locked = false;
int err = 0;
static const char *es = " Unmount and run chkdsk.";
@@ -1171,10 +1263,16 @@ static int ntfs_external_attr_find(const __le32 type,
if (type == AT_END)
goto not_found;
vol = base_ni->vol;
+ down_read(&base_ni->attr_list_lock);
+ attr_list_locked = true;
al_start = base_ni->attr_list;
al_end = al_start + base_ni->attr_list_size;
- if (!ctx->al_entry) {
- ctx->al_entry = (struct attr_list_entry *)al_start;
+ if (!ctx->al_cursor.valid ||
+ ctx->al_cursor.gen != base_ni->attr_list_gen ||
+ ctx->al_cursor.off >= base_ni->attr_list_size) {
+ ctx->al_cursor.off = 0;
+ ctx->al_cursor.gen = base_ni->attr_list_gen;
+ ctx->al_cursor.valid = true;
is_first_search = true;
}
/*
@@ -1182,7 +1280,7 @@ static int ntfs_external_attr_find(const __le32 type,
* or the entry following that, if @ctx->is_first is 'true'.
*/
if (ctx->is_first) {
- al_entry = ctx->al_entry;
+ al_entry = (struct attr_list_entry *)(al_start + ctx->al_cursor.off);
ctx->is_first = false;
/*
* If an enumeration and the first attribute is higher than
@@ -1195,14 +1293,14 @@ static int ntfs_external_attr_find(const __le32 type,
goto find_attr_list_attr;
} else {
/* Check for small entry */
- if (((al_end - (u8 *)ctx->al_entry) <
- (long)offsetof(struct attr_list_entry, name)) ||
- (le16_to_cpu(ctx->al_entry->length) & 7) ||
- (le16_to_cpu(ctx->al_entry->length) < offsetof(struct attr_list_entry, name)))
+ al_entry = (struct attr_list_entry *)(al_start + ctx->al_cursor.off);
+ if (((al_end - (u8 *)al_entry) < (long)offsetof(struct attr_list_entry, name)) ||
+ (le16_to_cpu(al_entry->length) & 7) ||
+ (le16_to_cpu(al_entry->length) < offsetof(struct attr_list_entry, name)))
goto corrupt;
- al_entry = (struct attr_list_entry *)((u8 *)ctx->al_entry +
- le16_to_cpu(ctx->al_entry->length));
+ al_entry = (struct attr_list_entry *)((u8 *)al_entry +
+ le16_to_cpu(al_entry->length));
if ((u8 *)al_entry == al_end)
goto not_found;
@@ -1218,15 +1316,18 @@ static int ntfs_external_attr_find(const __le32 type,
* attribute list attribute from the base mft record as it is
* not listed in the attribute list itself.
*/
- if ((type == AT_UNUSED) && le32_to_cpu(ctx->al_entry->type) <
- le32_to_cpu(AT_ATTRIBUTE_LIST) &&
- le32_to_cpu(al_entry->type) >
- le32_to_cpu(AT_ATTRIBUTE_LIST)) {
+ if ((type == AT_UNUSED) &&
+ le32_to_cpu(((struct attr_list_entry *)(al_start +
+ ctx->al_cursor.off))->type) <
+ le32_to_cpu(AT_ATTRIBUTE_LIST) &&
+ le32_to_cpu(al_entry->type) > le32_to_cpu(AT_ATTRIBUTE_LIST)) {
find_attr_list_attr:
/* Check for bogus calls. */
- if (name || name_len || val || val_len || lowest_vcn)
- return -EINVAL;
+ if (name || name_len || val || val_len || lowest_vcn) {
+ err = -EINVAL;
+ goto unlock_list_attr;
+ }
/* We want the base record. */
if (ctx->ntfs_ino != base_ni)
@@ -1248,28 +1349,33 @@ static int ntfs_external_attr_find(const __le32 type,
* Setup the search context so the correct
* attribute is returned next time round.
*/
- ctx->al_entry = al_entry;
+ ctx->al_cursor.off = (u8 *)al_entry - al_start;
+ ctx->al_cursor.gen = base_ni->attr_list_gen;
+ ctx->al_cursor.valid = true;
ctx->is_first = true;
- /* Got it. Done. */
if (!err)
- return 0;
+ goto unlock_list_attr;
/* Error! If other than not found return it. */
if (err != -ENOENT)
- return err;
+ goto unlock_list_attr;
/* Not found?!? Absurd! */
ntfs_error(ctx->ntfs_ino->vol->sb, "Attribute list wasn't found");
- return -EIO;
+ err = -EIO;
+ goto unlock_list_attr;
}
}
for (;; al_entry = next_al_entry) {
+scan_ale:
/* Out of bounds check. */
if ((u8 *)al_entry < base_ni->attr_list ||
(u8 *)al_entry > al_end)
break; /* Inode is corrupt. */
- ctx->al_entry = al_entry;
+ ctx->al_cursor.off = (u8 *)al_entry - al_start;
+ ctx->al_cursor.gen = base_ni->attr_list_gen;
+ ctx->al_cursor.valid = true;
/* Catch the end of the attribute list. */
if ((u8 *)al_entry == al_end)
goto not_found;
@@ -1333,6 +1439,9 @@ static int ntfs_external_attr_find(const __le32 type,
if (rc)
continue;
}
+ if (type != AT_UNUSED &&
+ le64_to_cpu(al_entry->lowest_vcn) > (u64)lowest_vcn)
+ goto not_found;
/*
* The names match or @name not present and attribute is
* unnamed. Now check @lowest_vcn. Continue search if the
@@ -1355,8 +1464,19 @@ static int ntfs_external_attr_find(const __le32 type,
continue;
is_enumeration:
- if (MREF_LE(al_entry->mft_reference) == ni->mft_no) {
- if (MSEQNO_LE(al_entry->mft_reference) != ni->seq_no) {
+ /*
+ * The extent mapping below takes extent_lock. Keep only a
+ * stable copy of this ALE while taking it, so the attr-list
+ * read lock never nests outside extent_lock.
+ */
+ ntfs_attrlist_capture_exact(ctx, base_ni, al_entry, al_start);
+ if (attr_list_locked)
+ up_read(&base_ni->attr_list_lock);
+ attr_list_locked = false;
+
+ if (MREF_LE(ctx->al_exact.key.mft_reference) == ni->mft_no) {
+ if (MSEQNO_LE(ctx->al_exact.key.mft_reference) !=
+ ni->seq_no) {
ntfs_error(vol->sb,
"Found stale mft reference in attribute list of base inode 0x%llx.%s",
base_ni->mft_no, es);
@@ -1368,21 +1488,22 @@ static int ntfs_external_attr_find(const __le32 type,
if (ni != base_ni)
unmap_extent_mft_record(ni);
/* Do we want the base record back? */
- if (MREF_LE(al_entry->mft_reference) ==
- base_ni->mft_no) {
+ if (MREF_LE(ctx->al_exact.key.mft_reference) ==
+ base_ni->mft_no) {
ni = ctx->ntfs_ino = base_ni;
ctx->mrec = ctx->base_mrec;
ctx->mapped_mrec = ctx->mapped_base_mrec;
} else {
/* We want an extent record. */
- ctx->mrec = map_extent_mft_record(base_ni,
- le64_to_cpu(
- al_entry->mft_reference), &ni);
+ ctx->mrec =
+ map_extent_mft_record(base_ni, le64_to_cpu(
+ ctx->al_exact.key.mft_reference),
+ &ni);
if (IS_ERR(ctx->mrec)) {
ntfs_error(vol->sb,
- "Failed to map extent mft record 0x%lx of base inode 0x%llx.%s",
- MREF_LE(al_entry->mft_reference),
- base_ni->mft_no, es);
+ "Failed to map extent mft record 0x%lx of base inode 0x%llx.%s",
+ MREF_LE(ctx->al_exact.key.mft_reference),
+ base_ni->mft_no, es);
err = PTR_ERR(ctx->mrec);
if (err == -ENOENT)
err = -EIO;
@@ -1392,7 +1513,6 @@ static int ntfs_external_attr_find(const __le32 type,
}
ctx->ntfs_ino = ni;
ctx->mapped_mrec = true;
-
}
}
a = ctx->attr = (struct attr_record *)((u8 *)ctx->mrec +
@@ -1424,8 +1544,19 @@ static int ntfs_external_attr_find(const __le32 type,
mft_free_len = le32_to_cpu(ctx->mrec->bytes_in_use) -
((u8 *)a - (u8 *)ctx->mrec);
- if (mft_free_len >= sizeof(a->type) && a->type == AT_END)
- continue;
+ if (mft_free_len >= sizeof(a->type) && a->type == AT_END) {
+ down_read(&base_ni->attr_list_lock);
+ attr_list_locked = true;
+ al_start = base_ni->attr_list;
+ al_end = al_start + base_ni->attr_list_size;
+ al_entry = ntfs_attrlist_find_exact_locked(base_ni,
+ &ctx->al_exact);
+ if (!al_entry)
+ goto corrupt;
+ al_entry = (struct attr_list_entry *)((u8 *)al_entry +
+ le16_to_cpu(al_entry->length));
+ goto scan_ale;
+ }
attr_len = le32_to_cpu(a->length);
if (!attr_len ||
@@ -1434,23 +1565,38 @@ static int ntfs_external_attr_find(const __le32 type,
attr_len > mft_free_len)
break;
- if (al_entry->instance != a->instance)
+ if (ctx->al_exact.key.instance != a->instance)
goto do_next_attr;
/*
* If the type and/or the name are mismatched between the
* attribute list entry and the attribute record, there is
* corruption so we break and return error EIO.
*/
- if (al_entry->type != a->type)
+ if (ctx->al_exact.key.type != a->type)
break;
if (a->name_length && ((le16_to_cpu(a->name_offset) +
a->name_length * sizeof(__le16)) > attr_len))
break;
- if (!ntfs_are_names_equal((__le16 *)((u8 *)a +
- le16_to_cpu(a->name_offset)), a->name_length,
- al_name, al_name_len, CASE_SENSITIVE,
- vol->upcase, vol->upcase_len))
+ /*
+ * al_entry's name isn't carried across the attr_list_lock
+ * drop above; re-take the lock just long enough to re-locate
+ * the same attr-list entry (same pattern as the AT_END case
+ * above) and compare @a's on-disk name against it. The
+ * corrupt/unlock_list_attr labels below drop the lock again
+ * via attr_list_locked on the not-found/mismatch paths.
+ */
+ down_read(&base_ni->attr_list_lock);
+ attr_list_locked = true;
+ al_entry = ntfs_attrlist_find_exact_locked(base_ni, &ctx->al_exact);
+ if (!al_entry)
+ goto corrupt;
+ if (!ntfs_are_names_equal((__le16 *)((u8 *)a + le16_to_cpu(a->name_offset)),
+ a->name_length, al_entry->name,
+ al_entry->name_length, CASE_SENSITIVE,
+ vol->upcase, vol->upcase_len))
break;
+ up_read(&base_ni->attr_list_lock);
+ attr_list_locked = false;
ctx->attr = a;
@@ -1480,6 +1626,11 @@ static int ntfs_external_attr_find(const __le32 type,
goto do_next_attr_loop;
}
+unlock_list_attr:
+ if (attr_list_locked)
+ up_read(&base_ni->attr_list_lock);
+ return err;
+
corrupt:
if (ni != base_ni) {
if (ni)
@@ -1491,20 +1642,31 @@ static int ntfs_external_attr_find(const __le32 type,
}
if (!err) {
- u64 mft_no = ctx->al_entry ? MREF_LE(ctx->al_entry->mft_reference) : 0;
- u32 type = ctx->al_entry ? le32_to_cpu(ctx->al_entry->type) : 0;
+ u64 mft_no = ctx->al_exact.valid ?
+ MREF_LE(ctx->al_exact.key.mft_reference) :
+ 0;
+ u32 ale_type = ctx->al_exact.valid ?
+ le32_to_cpu(ctx->al_exact.key.type) :
+ 0;
ntfs_error(vol->sb,
- "Base inode 0x%llx contains corrupt attribute, mft %#llx, type %#x. %s",
- (long long)base_ni->mft_no, (long long)mft_no, type,
- "Unmount and run chkdsk.");
+ "Base inode 0x%llx contains corrupt attribute, mft %#llx, type %#x. %s",
+ (long long)base_ni->mft_no, (long long)mft_no,
+ ale_type, "Unmount and run chkdsk.");
err = -EIO;
}
+ if (attr_list_locked)
+ up_read(&base_ni->attr_list_lock);
if (err != -ENOMEM)
NVolSetErrors(vol);
return err;
not_found:
+ if (attr_list_locked) {
+ ntfs_attrlist_capture_insert(ctx, base_ni, al_entry, al_start,
+ al_end);
+ up_read(&base_ni->attr_list_lock);
+ }
/*
* If we were looking for AT_END, we reset the search context @ctx and
* use ntfs_attr_find() to seek to the end of the base mft record.
@@ -1634,11 +1796,11 @@ static bool ntfs_attr_init_search_ctx(struct ntfs_attr_search_ctx *ctx,
ctx->attr = (struct attr_record *)((u8 *)mrec + le16_to_cpu(mrec->attrs_offset));
ctx->is_first = true;
ctx->ntfs_ino = ni;
- ctx->al_entry = NULL;
ctx->base_ntfs_ino = NULL;
ctx->base_mrec = NULL;
ctx->base_attr = NULL;
ctx->mapped_base_mrec = false;
+ ntfs_attrlist_reset_locators(ctx);
return true;
}
@@ -1662,11 +1824,7 @@ void ntfs_attr_reinit_search_ctx(struct ntfs_attr_search_ctx *ctx)
/* Sanity checks are performed elsewhere. */
ctx->attr = (struct attr_record *)((u8 *)ctx->mrec +
le16_to_cpu(ctx->mrec->attrs_offset));
- /*
- * This needs resetting due to ntfs_external_attr_find() which
- * can leave it set despite having zeroed ctx->base_ntfs_ino.
- */
- ctx->al_entry = NULL;
+ ntfs_attrlist_reset_locators(ctx);
return;
} /* Attribute list. */
if (ctx->ntfs_ino != ctx->base_ntfs_ino && ctx->ntfs_ino)
@@ -2626,13 +2784,12 @@ static int ntfs_non_resident_attr_record_add(struct ntfs_inode *ni, __le32 type,
* update of attribute list.
*/
ntfs_attr_reinit_search_ctx(ctx);
- err = ntfs_attr_lookup(type, name, name_len, CASE_SENSITIVE,
- lowest_vcn, NULL, 0, ctx);
+ err = ntfs_attr_lookup(type, name, name_len, CASE_SENSITIVE, lowest_vcn,
+ NULL, 0, ctx);
if (err) {
pr_err("%s: attribute lookup failed\n", __func__);
ntfs_attr_put_search_ctx(ctx);
return err;
-
}
offset = (u8 *)ctx->attr - (u8 *)ctx->mrec;
ntfs_attr_put_search_ctx(ctx);
@@ -3396,6 +3553,8 @@ int ntfs_attr_record_move_to(struct ntfs_attr_search_ctx *ctx, struct ntfs_inode
int err;
struct mft_record *ni_mrec;
struct super_block *sb;
+ struct ntfs_inode *base_ni;
+ struct attr_list_entry *ale;
if (!ctx || !ctx->attr || !ctx->ntfs_ino || !ni) {
ntfs_debug("Invalid arguments passed.\n");
@@ -3411,7 +3570,7 @@ int ntfs_attr_record_move_to(struct ntfs_attr_search_ctx *ctx, struct ntfs_inode
if (ctx->ntfs_ino == ni)
return 0;
- if (!ctx->al_entry) {
+ if (!ctx->al_exact.valid) {
ntfs_debug("Inode should contain attribute list to use this function.\n");
return -EINVAL;
}
@@ -3465,9 +3624,24 @@ int ntfs_attr_record_move_to(struct ntfs_attr_search_ctx *ctx, struct ntfs_inode
mark_mft_record_dirty(ni);
/* Update attribute list. */
- ctx->al_entry->mft_reference =
+ a = (struct attr_record *)nctx->attr;
+ base_ni = ntfs_attr_ctx_base_ni(ctx);
+
+ down_write(&base_ni->attr_list_lock);
+ ale = ntfs_attrlist_find_exact_locked(base_ni, &ctx->al_exact);
+ if (!ale) {
+ up_write(&base_ni->attr_list_lock);
+ unmap_mft_record(ni);
+ err = -EIO;
+ goto put_err_out;
+ }
+
+ ale->mft_reference =
MK_LE_MREF(ni->mft_no, le16_to_cpu(ni_mrec->sequence_number));
- ctx->al_entry->instance = nctx->attr->instance;
+ ale->instance = nctx->attr->instance;
+ base_ni->attr_list_gen++;
+ ntfs_attrlist_capture_exact(ctx, base_ni, ale, base_ni->attr_list);
+ up_write(&base_ni->attr_list_lock);
unmap_mft_record(ni);
put_err_out:
ntfs_attr_put_search_ctx(nctx);
@@ -3707,6 +3881,23 @@ static int ntfs_attr_update_meta(struct attr_record *a, struct ntfs_inode *ni,
* call to this function. Vice-versa @na->compressed_size will be calculated and
* set to correct value during this function.
*/
+static bool ntfs_attr_ctx_matches_ni(const struct ntfs_attr_search_ctx *ctx,
+ const struct ntfs_inode *ni)
+{
+ const struct attr_record *a = ctx->attr;
+ const __le16 *name;
+
+ if (a->type != ni->type || a->name_length != ni->name_len)
+ return false;
+ if (!a->name_length)
+ return true;
+
+ name = (const __le16 *)((const u8 *)a + le16_to_cpu(a->name_offset));
+ return ntfs_are_names_equal(name, a->name_length, ni->name,
+ ni->name_len, CASE_SENSITIVE,
+ ni->vol->upcase, ni->vol->upcase_len);
+}
+
int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
{
struct ntfs_attr_search_ctx *ctx;
@@ -3715,11 +3906,12 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
struct attr_record *a;
s64 stop_vcn;
int err = 0, mp_size, cur_max_mp_size, exp_max_mp_size;
- bool finished_build;
+ bool finished_build, attrlist_changed = false;
bool first_updated = false;
struct super_block *sb;
struct runlist_element *start_rl;
unsigned int de_cluster_count = 0;
+ bool first_lookup = true;
retry:
if (!ni || !ni->runlist.rl)
@@ -3748,11 +3940,24 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
/* Fill attribute records with new mapping pairs. */
stop_vcn = 0;
finished_build = false;
+ first_lookup = true;
start_rl = ni->runlist.rl;
- while (!(err = ntfs_attr_lookup(ni->type, ni->name, ni->name_len,
- CASE_SENSITIVE, from_vcn, NULL, 0, ctx))) {
+ while (1) {
unsigned int de_cnt = 0;
+ err = ntfs_attr_lookup(AT_UNUSED, NULL, 0, CASE_SENSITIVE, 0,
+ NULL, 0, ctx);
+ if (err)
+ break;
+ if (!ntfs_attr_ctx_matches_ni(ctx, ni))
+ continue;
+ if (first_lookup) {
+ if (le64_to_cpu(ctx->attr->data.non_resident.lowest_vcn) <
+ from_vcn)
+ continue;
+ first_lookup = false;
+ }
+
a = ctx->attr;
m = ctx->mrec;
if (!a->data.non_resident.lowest_vcn)
@@ -3878,15 +4083,40 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
}
}
- /* Update lowest vcn. */
- a->data.non_resident.lowest_vcn = cpu_to_le64(stop_vcn);
- mark_mft_record_dirty(ctx->ntfs_ino);
- if ((ctx->ntfs_ino->nr_extents == -1 || NInoAttrList(ctx->ntfs_ino)) &&
+ /*
+ * al_cursor.valid is true here exactly when this iteration's
+ * ntfs_attr_lookup() went through ntfs_external_attr_find()
+ */
+ if (ctx->al_cursor.valid &&
ctx->attr->type != AT_ATTRIBUTE_LIST) {
- ctx->al_entry->lowest_vcn = cpu_to_le64(stop_vcn);
- err = ntfs_attrlist_update(base_ni);
- if (err)
+ struct attr_list_entry *ale;
+
+ down_write(&base_ni->attr_list_lock);
+ ale = ntfs_attrlist_find_exact_locked(base_ni,
+ &ctx->al_exact);
+ if (!ale) {
+ up_write(&base_ni->attr_list_lock);
+ err = -EIO;
goto put_err_out;
+ }
+ ale->lowest_vcn = cpu_to_le64(stop_vcn);
+ base_ni->attr_list_gen++;
+ ntfs_attrlist_capture_exact(ctx, base_ni, ale,
+ base_ni->attr_list);
+ ctx->al_cursor.off = (u8 *)ale - base_ni->attr_list;
+ ctx->al_cursor.gen = base_ni->attr_list_gen;
+ ctx->al_cursor.valid = true;
+ attrlist_changed = true;
+ up_write(&base_ni->attr_list_lock);
+
+ /* Update lowest vcn in attr record after ALE is fixed. */
+ a->data.non_resident.lowest_vcn = cpu_to_le64(stop_vcn);
+ mark_mft_record_dirty(ctx->ntfs_ino);
+
+ } else {
+ /* Update lowest vcn. */
+ a->data.non_resident.lowest_vcn = cpu_to_le64(stop_vcn);
+ mark_mft_record_dirty(ctx->ntfs_ino);
}
/*
@@ -3937,12 +4167,21 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
}
}
+ if (attrlist_changed) {
+ err = ntfs_attrlist_update(base_ni);
+ if (err)
+ goto put_err_out;
+ }
+
/* Deallocate not used attribute extents and return with success. */
if (finished_build) {
ntfs_attr_reinit_search_ctx(ctx);
ntfs_debug("Deallocate marked extents.\n");
- while (!(err = ntfs_attr_lookup(ni->type, ni->name, ni->name_len,
- CASE_SENSITIVE, 0, NULL, 0, ctx))) {
+ while (!(err = ntfs_attr_lookup(AT_UNUSED, NULL, 0,
+ CASE_SENSITIVE, 0, NULL, 0,
+ ctx))) {
+ if (!ntfs_attr_ctx_matches_ni(ctx, ni))
+ continue;
if (le64_to_cpu(ctx->attr->data.non_resident.highest_vcn) !=
NTFS_VCN_DELETE_MARK)
continue;
diff --git a/fs/ntfs/attrib.h b/fs/ntfs/attrib.h
index e2224fbfaabe..a39719f75d06 100644
--- a/fs/ntfs/attrib.h
+++ b/fs/ntfs/attrib.h
@@ -15,6 +15,34 @@
extern __le16 AT_UNNAMED[];
+struct ntfs_attrlist_cursor {
+ u32 off;
+ u64 gen;
+ bool valid;
+};
+
+struct ntfs_attrlist_anchor {
+ u32 off;
+ u64 gen;
+ bool valid;
+ bool at_end;
+};
+
+struct ntfs_attrlist_exact_key {
+ __le32 type;
+ __le64 lowest_vcn;
+ __le64 mft_reference;
+ __le16 instance;
+ u8 name_len;
+};
+
+struct ntfs_attrlist_exact {
+ u32 off;
+ u64 gen;
+ bool valid;
+ struct ntfs_attrlist_exact_key key;
+};
+
/*
* ntfs_attr_search_ctx - used in attribute search functions
* @mrec: buffer containing mft record to search
@@ -22,10 +50,12 @@ extern __le16 AT_UNNAMED[];
* @attr: attribute record in @mrec where to begin/continue search
* @is_first: if true ntfs_attr_lookup() begins search with @attr, else after
* @ntfs_ino: Inode owning this attribute search
- * @al_entry: Current attribute list entry
* @base_ntfs_ino: Base inode
* @mapped_base_mrec: true if @base_mrec was mapped by the search
* @base_attr: Base attribute record pointer
+ * @al_cursor: Restartable attr-list enumeration cursor
+ * @al_insert: Insert-before anchor used on -ENOENT
+ * @al_exact: Exact ALE identity for writer-side updates
*
* Structure must be initialized to zero before the first call to one of the
* attribute search functions. Initialize @mrec to point to the mft record to
@@ -45,11 +75,13 @@ struct ntfs_attr_search_ctx {
struct attr_record *attr;
bool is_first;
struct ntfs_inode *ntfs_ino;
- struct attr_list_entry *al_entry;
struct ntfs_inode *base_ntfs_ino;
struct mft_record *base_mrec;
bool mapped_base_mrec;
struct attr_record *base_attr;
+ struct ntfs_attrlist_cursor al_cursor;
+ struct ntfs_attrlist_anchor al_insert;
+ struct ntfs_attrlist_exact al_exact;
};
enum { /* ways of processing holes when expanding */
@@ -89,6 +121,14 @@ void ntfs_attr_reinit_search_ctx(struct ntfs_attr_search_ctx *ctx);
struct ntfs_attr_search_ctx *ntfs_attr_get_search_ctx(struct ntfs_inode *ni,
struct mft_record *mrec);
void ntfs_attr_put_search_ctx(struct ntfs_attr_search_ctx *ctx);
+void ntfs_attrlist_reset_locators(struct ntfs_attr_search_ctx *ctx);
+void ntfs_attrlist_capture_exact(struct ntfs_attr_search_ctx *ctx,
+ struct ntfs_inode *base_ni,
+ struct attr_list_entry *ale, u8 *al_start);
+void ntfs_attrlist_exact_key_from_ale(struct ntfs_attrlist_exact_key *key,
+ const struct attr_list_entry *ale);
+bool ntfs_attrlist_exact_key_eq(const struct attr_list_entry *ale,
+ const struct ntfs_attrlist_exact_key *key);
int ntfs_attr_size_bounds_check(const struct ntfs_volume *vol,
const __le32 type, const s64 size);
int ntfs_attr_can_be_resident(const struct ntfs_volume *vol,
diff --git a/fs/ntfs/attrlist.c b/fs/ntfs/attrlist.c
index be3086d34338..b8594037df40 100644
--- a/fs/ntfs/attrlist.c
+++ b/fs/ntfs/attrlist.c
@@ -182,15 +182,16 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
0 : le32_to_cpu(attr->data.resident.value_length), ctx);
if (!err) {
/* Found some extent, check it to be before new extent. */
- if (ctx->al_entry->lowest_vcn == lowest_vcn) {
+ if (ctx->al_exact.key.lowest_vcn == lowest_vcn) {
err = -EEXIST;
ntfs_debug("Such attribute already present in the attribute list.\n");
ntfs_attr_put_search_ctx(ctx);
goto err_out;
}
/* Add new entry after this extent. */
- ale = (struct attr_list_entry *)((u8 *)ctx->al_entry +
- le16_to_cpu(ctx->al_entry->length));
+ entry_offset = ctx->al_exact.off +
+ le16_to_cpu(((struct attr_list_entry *)(ni->attr_list +
+ ctx->al_exact.off))->length);
} else {
/* Check for real errors. */
if (err != -ENOENT) {
@@ -199,13 +200,11 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
goto err_out;
}
/* No previous extents found. */
- ale = ctx->al_entry;
+ entry_offset = ctx->al_insert.off;
}
/* Don't need it anymore, @ctx->al_entry points to @ni->attr_list. */
ntfs_attr_put_search_ctx(ctx);
- /* Determine new entry offset. */
- entry_offset = ((u8 *)ale - ni->attr_list);
/* Set pointer to new entry. */
ale = (struct attr_list_entry *)(new_al + entry_offset);
memset(ale, 0, entry_len);
@@ -261,7 +260,7 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
struct ntfs_inode *base_ni;
struct attr_list_entry *ale;
- if (!ctx || !ctx->ntfs_ino || !ctx->al_entry) {
+ if (!ctx || !ctx->ntfs_ino || !ctx->al_exact.valid) {
ntfs_debug("Invalid arguments.\n");
return -EINVAL;
}
@@ -270,12 +269,14 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
base_ni = ctx->base_ntfs_ino;
else
base_ni = ctx->ntfs_ino;
- ale = ctx->al_entry;
+ if (ctx->al_exact.off >= base_ni->attr_list_size)
+ return -EIO;
+ ale = (struct attr_list_entry *)(base_ni->attr_list + ctx->al_exact.off);
ntfs_debug("Entering for inode 0x%llx, attr 0x%x, lowest_vcn %lld.\n",
- (long long)ctx->ntfs_ino->mft_no,
- (unsigned int)le32_to_cpu(ctx->al_entry->type),
- (long long)le64_to_cpu(ctx->al_entry->lowest_vcn));
+ (long long)ctx->ntfs_ino->mft_no,
+ (unsigned int)le32_to_cpu(ctx->al_exact.key.type),
+ (long long)le64_to_cpu(ctx->al_exact.key.lowest_vcn));
if (!NInoAttrList(base_ni)) {
ntfs_debug("Attribute list isn't present.\n");
diff --git a/fs/ntfs/index.c b/fs/ntfs/index.c
index 409759eab55d..082fe9226c72 100644
--- a/fs/ntfs/index.c
+++ b/fs/ntfs/index.c
@@ -1346,7 +1346,8 @@ static int ntfs_ir_reparent(struct ntfs_index_context *icx)
* When there is no space to build a non-resident
* index, we may have to move the root to an extent
*/
- if ((ret == -ENOSPC) && (ctx->al_entry || !ntfs_inode_add_attrlist(icx->idx_ni))) {
+ if ((ret == -ENOSPC) &&
+ (ctx->al_cursor.valid || !ntfs_inode_add_attrlist(icx->idx_ni))) {
ntfs_attr_put_search_ctx(ctx);
ctx = NULL;
ir = ntfs_ir_lookup(icx->idx_ni, icx->name, icx->name_len, &ctx);
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 39c7fd8c1149..38206e2009e2 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -475,6 +475,8 @@ void __ntfs_init_inode(struct super_block *sb, struct ntfs_inode *ni)
ni->folio = NULL;
ni->folio_ofs = 0;
ni->mrec = NULL;
+ init_rwsem(&ni->attr_list_lock);
+ ni->attr_list_gen = 0;
ni->attr_list_size = 0;
ni->attr_list = NULL;
ni->itype.index.block_size = 0;
@@ -3327,19 +3329,25 @@ static int ntfs_attr_position(__le32 type, struct ntfs_attr_search_ctx *ctx)
if (atype == AT_END)
return -ENOSPC;
- /*
- * if ntfs_external_attr_lookup return -ENOENT, ctx->al_entry
- * could point to an attribute in an extent mft record, but
- * ctx->attr and ctx->ntfs_ino always points to an attibute in
- * a base mft record.
- */
- if (ctx->al_entry &&
- MREF_LE(ctx->al_entry->mft_reference) != ctx->ntfs_ino->mft_no) {
- ntfs_attr_reinit_search_ctx(ctx);
- err = ntfs_attr_lookup(atype, NULL, 0, CASE_SENSITIVE, 0, NULL,
- 0, ctx);
- if (err)
- return err;
+ if (ctx->al_insert.valid && !ctx->al_insert.at_end &&
+ ctx->al_insert.off < ntfs_inode_base(ctx->ntfs_ino)->attr_list_size) {
+ struct ntfs_inode *base_ni =
+ ntfs_inode_base(ctx->ntfs_ino);
+ struct attr_list_entry *ale =
+ (struct attr_list_entry *)(base_ni->attr_list +
+ ctx->al_insert.off);
+
+ if (MREF_LE(ale->mft_reference) != ctx->ntfs_ino->mft_no) {
+ ntfs_attr_reinit_search_ctx(ctx);
+ err = ntfs_attr_lookup(atype, NULL, 0,
+ CASE_SENSITIVE, 0, NULL,
+ 0, ctx);
+ if (err)
+ return err;
+ } else {
+ ntfs_attrlist_capture_exact(ctx, base_ni, ale,
+ base_ni->attr_list);
+ }
}
}
return 0;
diff --git a/fs/ntfs/inode.h b/fs/ntfs/inode.h
index c6d065aaecd5..ed00e06c5d14 100644
--- a/fs/ntfs/inode.h
+++ b/fs/ntfs/inode.h
@@ -10,6 +10,8 @@
#ifndef _LINUX_NTFS_INODE_H
#define _LINUX_NTFS_INODE_H
+#include <linux/rwsem.h>
+
#include "debug.h"
enum ntfs_inode_mutex_lock_class {
@@ -66,6 +68,8 @@ enum ntfs_inode_mutex_lock_class {
* Attribute list support (only for use by the attribute lookup
* functions). Setup during read_inode for all inodes with attribute
* lists. Only valid if NI_AttrList is set in state.
+ * @attr_list_lock: Protects in-memory attribute list state.
+ * @attr_list_gen: Generation of the in-memory attribute list state.
* @attr_list_size: Length of attribute list value in bytes.
* @attr_list: Attribute list value itself.
*
@@ -118,6 +122,8 @@ struct ntfs_inode {
int folio_ofs;
s64 mft_lcn[2];
unsigned int mft_lcn_count;
+ struct rw_semaphore attr_list_lock;
+ u64 attr_list_gen;
u32 attr_list_size;
u8 *attr_list;
union {
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v7 2/4] ntfs: protect attribute-list buffer replacement with attr_list_persist_lock
2026-08-12 1:18 [PATCH v7 0/4] ntfs: serialize attribute-list access Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 1/4] ntfs: replace raw ctx->al_entry with restartable attr-list locators Hyunchul Lee
@ 2026-08-12 1:18 ` Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 3/4] ntfs: protect attribute-list creation/teardown " Hyunchul Lee
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Hyunchul Lee @ 2026-08-12 1:18 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, ntfs, Cen Zhang, Copilot, stable
ntfs_attrlist_entry_add() and ntfs_attrlist_entry_rm() replace the
in-memory attribute-list buffer and then persist it to disk. The swap
is protected by attr_list_lock, but persisting can sleep and recurse,
so that lock cannot be held across it.
Add attr_list_persist_lock, a mutex that covers the entire
publish/swap/persist/rollback transaction for each caller.
Reported-by: Cen Zhang <zzzccc427@gmail.com>
Link: https://lore.kernel.org/all/20260702161710.2606853-1-zzzccc427@gmail.com/
Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org
Tested-by: Cen Zhang <zzzccc427@gmail.com>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
fs/ntfs/attrib.c | 18 ++++--
fs/ntfs/attrib.h | 2 +
fs/ntfs/attrlist.c | 172 ++++++++++++++++++++++++++++++++++++++++++++---------
fs/ntfs/attrlist.h | 1 +
fs/ntfs/inode.c | 3 +-
fs/ntfs/inode.h | 3 +
6 files changed, 165 insertions(+), 34 deletions(-)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 0f48dd923b72..ef918a73fb0c 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -83,8 +83,8 @@ bool ntfs_attrlist_exact_key_eq(const struct attr_list_entry *ale,
return ale->name_length == key->name_len;
}
-static struct attr_list_entry *ntfs_attrlist_find_exact_locked(struct ntfs_inode *base_ni,
- struct ntfs_attrlist_exact *exact)
+struct attr_list_entry *ntfs_attrlist_find_exact_locked(struct ntfs_inode *base_ni,
+ struct ntfs_attrlist_exact *exact)
{
struct attr_list_entry *ale;
u8 *al_end;
@@ -3792,7 +3792,7 @@ static int ntfs_attr_update_meta(struct attr_record *a, struct ntfs_inode *ni,
goto out;
}
- err = ntfs_attrlist_update(base_ni);
+ err = ntfs_attrlist_update_locked(base_ni);
if (err)
goto out;
err = -EAGAIN;
@@ -4168,7 +4168,7 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
}
if (attrlist_changed) {
- err = ntfs_attrlist_update(base_ni);
+ err = ntfs_attrlist_update_locked(base_ni);
if (err)
goto put_err_out;
}
@@ -5029,7 +5029,15 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
"Couldn't free space in the MFT record to make attribute list non resident");
return err;
}
- err = ntfs_attrlist_update(base_ni);
+ /*
+ * Resizing the $ATTRIBUTE_LIST attribute itself only happens
+ * underneath ntfs_attrlist_update_locked() while already holding
+ * attr_list_persist_lock.
+ */
+ if (attr_ni->type == AT_ATTRIBUTE_LIST)
+ err = ntfs_attrlist_update_locked(base_ni);
+ else
+ err = ntfs_attrlist_update(base_ni);
if (err)
return err;
goto attr_resize_again;
diff --git a/fs/ntfs/attrib.h b/fs/ntfs/attrib.h
index a39719f75d06..2e5f87abd852 100644
--- a/fs/ntfs/attrib.h
+++ b/fs/ntfs/attrib.h
@@ -129,6 +129,8 @@ void ntfs_attrlist_exact_key_from_ale(struct ntfs_attrlist_exact_key *key,
const struct attr_list_entry *ale);
bool ntfs_attrlist_exact_key_eq(const struct attr_list_entry *ale,
const struct ntfs_attrlist_exact_key *key);
+struct attr_list_entry *ntfs_attrlist_find_exact_locked(struct ntfs_inode *base_ni,
+ struct ntfs_attrlist_exact *exact);
int ntfs_attr_size_bounds_check(const struct ntfs_volume *vol,
const __le32 type, const s64 size);
int ntfs_attr_can_be_resident(const struct ntfs_volume *vol,
diff --git a/fs/ntfs/attrlist.c b/fs/ntfs/attrlist.c
index b8594037df40..aa6d5f10bf96 100644
--- a/fs/ntfs/attrlist.c
+++ b/fs/ntfs/attrlist.c
@@ -51,7 +51,13 @@ int ntfs_attrlist_need(struct ntfs_inode *ni)
return 0;
}
-int ntfs_attrlist_update(struct ntfs_inode *base_ni)
+/*
+ * ntfs_attrlist_update_locked - persist the in-memory attribute list to disk
+ * @base_ni: base ntfs inode containing the attribute list
+ *
+ * Caller must hold @base_ni->attr_list_persist_lock.
+ */
+int ntfs_attrlist_update_locked(struct ntfs_inode *base_ni)
{
struct inode *attr_vi;
struct ntfs_inode *attr_ni;
@@ -111,6 +117,23 @@ int ntfs_attrlist_update(struct ntfs_inode *base_ni)
return 0;
}
+/*
+ * ntfs_attrlist_update - persist the in-memory attribute list to disk
+ * @base_ni: base ntfs inode containing the attribute list
+ *
+ * Serialize the persist against concurrent attribute-list replacement
+ * transactions.
+ */
+int ntfs_attrlist_update(struct ntfs_inode *base_ni)
+{
+ int err;
+
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ err = ntfs_attrlist_update_locked(base_ni);
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ return err;
+}
+
/*
* ntfs_attrlist_entry_add - add an attribute list attribute entry
* @ni: opened ntfs inode, which contains that attribute
@@ -122,12 +145,13 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
{
struct attr_list_entry *ale;
__le64 mref;
- struct ntfs_attr_search_ctx *ctx;
- u8 *new_al;
+ struct ntfs_attr_search_ctx *ctx = NULL;
+ u8 *new_al = NULL;
int entry_len, entry_offset, err;
struct mft_record *ni_mrec;
u8 *old_al;
__le64 lowest_vcn;
+ bool found, rollback;
if (!ni || !attr) {
ntfs_debug("Invalid arguments.\n");
@@ -154,12 +178,21 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
return -ENOENT;
}
- /* Determine size and allocate memory for new attribute list. */
+ mutex_lock(&ni->attr_list_persist_lock);
+
+ if (!NInoAttrList(ni) || !ni->attr_list) {
+ err = -ENOENT;
+ goto err_out;
+ }
+
+ /* Determine size of new attribute list entry. */
entry_len = (sizeof(struct attr_list_entry) + sizeof(__le16) *
attr->name_length + 7) & ~7;
- new_al = kvzalloc(ni->attr_list_size + entry_len, GFP_NOFS);
- if (!new_al)
- return -ENOMEM;
+
+retry_lookup:
+ new_al = NULL;
+ found = false;
+ rollback = false;
/* Find place for the new entry. */
ctx = ntfs_attr_get_search_ctx(ni, NULL);
@@ -181,29 +214,54 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
le16_to_cpu(attr->data.resident.value_offset)), (attr->non_resident) ?
0 : le32_to_cpu(attr->data.resident.value_length), ctx);
if (!err) {
+ found = true;
/* Found some extent, check it to be before new extent. */
if (ctx->al_exact.key.lowest_vcn == lowest_vcn) {
err = -EEXIST;
ntfs_debug("Such attribute already present in the attribute list.\n");
- ntfs_attr_put_search_ctx(ctx);
goto err_out;
}
- /* Add new entry after this extent. */
- entry_offset = ctx->al_exact.off +
- le16_to_cpu(((struct attr_list_entry *)(ni->attr_list +
- ctx->al_exact.off))->length);
} else {
/* Check for real errors. */
if (err != -ENOENT) {
ntfs_debug("Attribute lookup failed.\n");
- ntfs_attr_put_search_ctx(ctx);
goto err_out;
}
/* No previous extents found. */
+ }
+
+ down_write(&ni->attr_list_lock);
+ if (found) {
+ if (ctx->al_exact.gen != ni->attr_list_gen) {
+ up_write(&ni->attr_list_lock);
+ ntfs_attr_put_search_ctx(ctx);
+ ctx = NULL;
+ goto retry_lookup;
+ }
+ ale = ntfs_attrlist_find_exact_locked(ni, &ctx->al_exact);
+ if (!ale) {
+ up_write(&ni->attr_list_lock);
+ err = -EIO;
+ goto err_out;
+ }
+ entry_offset = (u8 *)ale - ni->attr_list + le16_to_cpu(ale->length);
+ } else {
+ if (!ctx->al_insert.valid ||
+ ctx->al_insert.gen != ni->attr_list_gen) {
+ up_write(&ni->attr_list_lock);
+ ntfs_attr_put_search_ctx(ctx);
+ ctx = NULL;
+ goto retry_lookup;
+ }
entry_offset = ctx->al_insert.off;
}
- /* Don't need it anymore, @ctx->al_entry points to @ni->attr_list. */
- ntfs_attr_put_search_ctx(ctx);
+
+ new_al = kvzalloc(ni->attr_list_size + entry_len, GFP_NOFS);
+ if (!new_al) {
+ up_write(&ni->attr_list_lock);
+ err = -ENOMEM;
+ goto err_out;
+ }
/* Set pointer to new entry. */
ale = (struct attr_list_entry *)(new_al + entry_offset);
@@ -231,17 +289,34 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
old_al = ni->attr_list;
ni->attr_list = new_al;
ni->attr_list_size = ni->attr_list_size + entry_len;
+ ni->attr_list_gen++;
+ up_write(&ni->attr_list_lock);
+ ntfs_attr_put_search_ctx(ctx);
+ ctx = NULL;
- err = ntfs_attrlist_update(ni);
+ err = ntfs_attrlist_update_locked(ni);
if (err) {
- ni->attr_list = old_al;
- ni->attr_list_size -= entry_len;
+ down_write(&ni->attr_list_lock);
+ if (ni->attr_list == new_al) {
+ ni->attr_list = old_al;
+ ni->attr_list_size -= entry_len;
+ ni->attr_list_gen++;
+ rollback = true;
+ }
+ up_write(&ni->attr_list_lock);
+ if (!rollback)
+ new_al = NULL;
goto err_out;
}
kvfree(old_al);
+ mutex_unlock(&ni->attr_list_persist_lock);
return 0;
err_out:
- kvfree(new_al);
+ if (ctx)
+ ntfs_attr_put_search_ctx(ctx);
+ if (new_al)
+ kvfree(new_al);
+ mutex_unlock(&ni->attr_list_persist_lock);
return err;
}
@@ -255,10 +330,12 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
*/
int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
{
- u8 *new_al;
- int new_al_len;
+ u8 *new_al = NULL;
+ int err, new_al_len;
+ bool rollback;
struct ntfs_inode *base_ni;
struct attr_list_entry *ale;
+ u8 *old_al;
if (!ctx || !ctx->ntfs_ino || !ctx->al_exact.valid) {
ntfs_debug("Invalid arguments.\n");
@@ -269,9 +346,6 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
base_ni = ctx->base_ntfs_ino;
else
base_ni = ctx->ntfs_ino;
- if (ctx->al_exact.off >= base_ni->attr_list_size)
- return -EIO;
- ale = (struct attr_list_entry *)(base_ni->attr_list + ctx->al_exact.off);
ntfs_debug("Entering for inode 0x%llx, attr 0x%x, lowest_vcn %lld.\n",
(long long)ctx->ntfs_ino->mft_no,
@@ -282,12 +356,33 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
ntfs_debug("Attribute list isn't present.\n");
return -ENOENT;
}
+ mutex_lock(&base_ni->attr_list_persist_lock);
+
+ /*
+ * Another thread may have removed the attribute list while we were
+ * waiting for the mutex.
+ */
+ if (!NInoAttrList(base_ni) || !base_ni->attr_list) {
+ err = -ENOENT;
+ goto out_unlock;
+ }
+
+ down_write(&base_ni->attr_list_lock);
+ ale = ntfs_attrlist_find_exact_locked(base_ni, &ctx->al_exact);
+ if (!ale) {
+ up_write(&base_ni->attr_list_lock);
+ err = -EIO;
+ goto out_unlock;
+ }
/* Allocate memory for new attribute list. */
new_al_len = base_ni->attr_list_size - le16_to_cpu(ale->length);
new_al = kvzalloc(new_al_len, GFP_NOFS);
- if (!new_al)
- return -ENOMEM;
+ if (!new_al) {
+ up_write(&base_ni->attr_list_lock);
+ err = -ENOMEM;
+ goto out_unlock;
+ }
/* Copy entries from old attribute list to new. */
memcpy(new_al, base_ni->attr_list, (u8 *)ale - base_ni->attr_list);
@@ -295,9 +390,30 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
ale->length), new_al_len - ((u8 *)ale - base_ni->attr_list));
/* Set new runlist. */
- kvfree(base_ni->attr_list);
+ old_al = base_ni->attr_list;
base_ni->attr_list = new_al;
base_ni->attr_list_size = new_al_len;
+ base_ni->attr_list_gen++;
+ up_write(&base_ni->attr_list_lock);
- return ntfs_attrlist_update(base_ni);
+ err = ntfs_attrlist_update_locked(base_ni);
+ if (err) {
+ rollback = false;
+ down_write(&base_ni->attr_list_lock);
+ if (base_ni->attr_list == new_al) {
+ base_ni->attr_list = old_al;
+ base_ni->attr_list_size += le16_to_cpu(ale->length);
+ base_ni->attr_list_gen++;
+ rollback = true;
+ }
+ up_write(&base_ni->attr_list_lock);
+ if (rollback)
+ kvfree(new_al);
+ goto out_unlock;
+ }
+ kvfree(old_al);
+ err = 0;
+out_unlock:
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ return err;
}
diff --git a/fs/ntfs/attrlist.h b/fs/ntfs/attrlist.h
index 1892a3934d3a..ecb8a8c957fa 100644
--- a/fs/ntfs/attrlist.h
+++ b/fs/ntfs/attrlist.h
@@ -16,5 +16,6 @@ int ntfs_attrlist_need(struct ntfs_inode *ni);
int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr);
int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx);
int ntfs_attrlist_update(struct ntfs_inode *base_ni);
+int ntfs_attrlist_update_locked(struct ntfs_inode *base_ni);
#endif /* defined _NTFS_ATTRLIST_H */
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 38206e2009e2..ede16196ed0c 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -476,6 +476,7 @@ void __ntfs_init_inode(struct super_block *sb, struct ntfs_inode *ni)
ni->folio_ofs = 0;
ni->mrec = NULL;
init_rwsem(&ni->attr_list_lock);
+ mutex_init(&ni->attr_list_persist_lock);
ni->attr_list_gen = 0;
ni->attr_list_size = 0;
ni->attr_list = NULL;
@@ -3147,7 +3148,7 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
goto rollback;
}
- err = ntfs_attrlist_update(ni);
+ err = ntfs_attrlist_update_locked(ni);
if (err < 0)
goto remove_attrlist_record;
diff --git a/fs/ntfs/inode.h b/fs/ntfs/inode.h
index ed00e06c5d14..a6c5b15d648d 100644
--- a/fs/ntfs/inode.h
+++ b/fs/ntfs/inode.h
@@ -69,6 +69,7 @@ enum ntfs_inode_mutex_lock_class {
* functions). Setup during read_inode for all inodes with attribute
* lists. Only valid if NI_AttrList is set in state.
* @attr_list_lock: Protects in-memory attribute list state.
+ * @attr_list_persist_lock: Serializes attribute list replacement and persist.
* @attr_list_gen: Generation of the in-memory attribute list state.
* @attr_list_size: Length of attribute list value in bytes.
* @attr_list: Attribute list value itself.
@@ -123,6 +124,8 @@ struct ntfs_inode {
s64 mft_lcn[2];
unsigned int mft_lcn_count;
struct rw_semaphore attr_list_lock;
+ /* Serializes attribute list replacement and persist. */
+ struct mutex attr_list_persist_lock;
u64 attr_list_gen;
u32 attr_list_size;
u8 *attr_list;
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v7 3/4] ntfs: protect attribute-list creation/teardown with attr_list_persist_lock
2026-08-12 1:18 [PATCH v7 0/4] ntfs: serialize attribute-list access Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 1/4] ntfs: replace raw ctx->al_entry with restartable attr-list locators Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 2/4] ntfs: protect attribute-list buffer replacement with attr_list_persist_lock Hyunchul Lee
@ 2026-08-12 1:18 ` Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 4/4] ntfs: protect mapping-pairs attr-list updates " Hyunchul Lee
2026-10-17 2:54 ` [PATCH v7 0/4] ntfs: serialize attribute-list access Cen Zhang
4 siblings, 0 replies; 7+ messages in thread
From: Hyunchul Lee @ 2026-08-12 1:18 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, ntfs, Cen Zhang, Copilot, stable
ntfs_attr_record_rm() and ntfs_inode_add_attrlist() update the
in-memory attribute list without any lock, so a concurrent reader can
observe an inconsistent trio. Teardown can also free a buffer that a
concurrent ntfs_attrlist_entry_add()/rm() transaction still owns while
it sleeps in ntfs_attrlist_update().
Protect both functions with attr_list_persist_lock for their whole
publish/persist or teardown sequence, and with attr_list_lock (write)
around the actual buffer pointer/size/gen updates.
Take attr_list_persist_lock for every non-$ATTRIBUTE_LIST removal even
when no list is currently present, so creation cannot publish a stale
entry after the initial state check. Initialize the shared removal
result for successful paths, and route all attribute-list creation
failures through a common unlock path.
Reported-by: Cen Zhang <zzzccc427@gmail.com>
Link: https://lore.kernel.org/all/20260702161710.2606853-1-zzzccc427@gmail.com/
Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org
Tested-by: Cen Zhang <zzzccc427@gmail.com>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
fs/ntfs/attrib.c | 141 ++++++++++++++++++++++++++++++++++++++++++-----------
fs/ntfs/attrib.h | 2 +-
fs/ntfs/attrlist.c | 34 ++++++-------
fs/ntfs/attrlist.h | 2 +-
fs/ntfs/index.c | 23 +++++++--
fs/ntfs/inode.c | 33 ++++++++++---
fs/ntfs/namei.c | 2 +-
fs/ntfs/super.c | 2 +-
8 files changed, 179 insertions(+), 60 deletions(-)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index ef918a73fb0c..e52acbced9c5 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -2801,16 +2801,20 @@ static int ntfs_non_resident_attr_record_add(struct ntfs_inode *ni, __le32 type,
/*
* ntfs_attr_record_rm - remove attribute extent
- * @ctx: search context describing the attribute which should be removed
+ * @ctx: search context describing the attribute which should be removed
+ * @persist_locked: true if the caller already holds
+ * base_ni->attr_list_persist_lock for the in-flight
+ * transaction this removal is part of
*
* If this function succeed, user should reinit search context if he/she wants
* use it anymore.
*/
-int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
+int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx, bool persist_locked)
{
struct ntfs_inode *base_ni, *ni;
__le32 type;
- int err;
+ int err = 0;
+ bool attrlist_locked = persist_locked;
if (!ctx || !ctx->ntfs_ino || !ctx->mrec || !ctx->attr)
return -EINVAL;
@@ -2825,10 +2829,17 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
else
base_ni = ctx->ntfs_ino;
+ /* Keep ALE removal and the follow-up need check in one transaction. */
+ if (!attrlist_locked && type != AT_ATTRIBUTE_LIST) {
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ attrlist_locked = true;
+ }
+
/* Remove attribute itself. */
if (ntfs_attr_record_resize(ctx->mrec, ctx->attr, 0)) {
ntfs_debug("Couldn't remove attribute record. Bug or damaged MFT record.\n");
- return -EIO;
+ err = -EIO;
+ goto out_unlock;
}
mark_mft_record_dirty(ni);
@@ -2837,19 +2848,42 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
* delete $ATTRIBUTE_LIST itself.
*/
if (NInoAttrList(base_ni) && type != AT_ATTRIBUTE_LIST) {
- err = ntfs_attrlist_entry_rm(ctx);
+ err = ntfs_attrlist_entry_rm_locked(ctx);
if (err) {
ntfs_debug("Couldn't delete record from $ATTRIBUTE_LIST.\n");
- return err;
+ goto out_unlock;
}
}
/* Post $ATTRIBUTE_LIST delete setup. */
if (type == AT_ATTRIBUTE_LIST) {
+ /*
+ * attr_list_persist_lock serializes this in-memory attr_list
+ * teardown against a concurrent ntfs_attrlist_entry_add()/rm()
+ * transaction on the same inode.
+ *
+ * @persist_locked is true when we got here from
+ * ntfs_attr_update_mapping_pairs() rebuilding the mapping
+ * pairs of the $ATTRIBUTE_LIST attribute itself: that call
+ * only happens underneath ntfs_attrlist_update_locked(), which
+ * is always invoked while already holding this same mutex for
+ * the in-flight transaction. Taking it again here would
+ * deadlock the caller against itself, so skip the
+ * (re-)acquisition in that case and rely on the lock already
+ * held further up the stack.
+ */
+ if (!persist_locked)
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ down_write(&base_ni->attr_list_lock);
if (NInoAttrList(base_ni) && base_ni->attr_list)
kvfree(base_ni->attr_list);
base_ni->attr_list = NULL;
+ base_ni->attr_list_size = 0;
+ base_ni->attr_list_gen++;
NInoClearAttrList(base_ni);
+ up_write(&base_ni->attr_list_lock);
+ if (!persist_locked)
+ mutex_unlock(&base_ni->attr_list_persist_lock);
}
/* Free MFT record, if it doesn't contain attributes. */
@@ -2857,17 +2891,18 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
le16_to_cpu(ctx->mrec->attrs_offset) == 8) {
if (ntfs_mft_record_free(ni->vol, ni)) {
ntfs_debug("Couldn't free MFT record.\n");
- return -EIO;
+ err = -EIO;
+ goto out_unlock;
}
/* Remove done if we freed base inode. */
if (ni == base_ni)
- return 0;
+ goto out_unlock;
ntfs_inode_close(ni);
ctx->ntfs_ino = ni = NULL;
}
if (type == AT_ATTRIBUTE_LIST || !NInoAttrList(base_ni))
- return 0;
+ goto out_unlock;
/* Remove attribute list if we don't need it any more. */
if (!ntfs_attrlist_need(base_ni)) {
@@ -2878,7 +2913,7 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
if (ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0, CASE_SENSITIVE,
0, NULL, 0, ctx)) {
ntfs_debug("Couldn't find attribute list. Succeed anyway.\n");
- return 0;
+ goto out_unlock;
}
/* Deallocate clusters. */
if (ctx->attr->non_resident) {
@@ -2889,16 +2924,16 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
ctx->attr, NULL, &new_rl_count);
if (IS_ERR(al_rl)) {
ntfs_debug("Couldn't decompress attribute list runlist. Succeed anyway.\n");
- return 0;
+ goto out_unlock;
}
if (ntfs_cluster_free_from_rl(base_ni->vol, al_rl))
ntfs_debug("Leaking clusters! Run chkdsk. Couldn't free clusters from attribute list runlist.\n");
kvfree(al_rl);
}
/* Remove attribute record itself. */
- if (ntfs_attr_record_rm(ctx)) {
+ if (ntfs_attr_record_rm(ctx, true)) {
ntfs_debug("Couldn't remove attribute list. Succeed anyway.\n");
- return 0;
+ goto out_unlock;
}
na.mft_no = VFS_I(base_ni)->i_ino;
@@ -2914,7 +2949,10 @@ int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx)
}
}
- return 0;
+out_unlock:
+ if (attrlist_locked && !persist_locked)
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ return err;
}
/*
@@ -3545,6 +3583,12 @@ int ntfs_attr_map_whole_runlist(struct ntfs_inode *ni)
* ntfs_attr_record_move_to - move attribute record to target inode
* @ctx: attribute search context describing the attribute record
* @ni: opened ntfs inode to which move attribute record
+ *
+ * Caller must hold the base inode's attr_list_persist_lock: this rewrites
+ * the moved attribute's ALE in place, and that mutation has to be part of
+ * the same transaction as the persist that follows it. Otherwise a
+ * concurrent ntfs_attrlist_update_locked() can copy the attribute list to
+ * disk while only one of @mft_reference/@instance has been updated.
*/
int ntfs_attr_record_move_to(struct ntfs_attr_search_ctx *ctx, struct ntfs_inode *ni)
{
@@ -3627,6 +3671,8 @@ int ntfs_attr_record_move_to(struct ntfs_attr_search_ctx *ctx, struct ntfs_inode
a = (struct attr_record *)nctx->attr;
base_ni = ntfs_attr_ctx_base_ni(ctx);
+ lockdep_assert_held(&base_ni->attr_list_persist_lock);
+
down_write(&base_ni->attr_list_lock);
ale = ntfs_attrlist_find_exact_locked(base_ni, &ctx->al_exact);
if (!ale) {
@@ -4049,7 +4095,14 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
ntfs_debug("Attribute list is too big. Defragment the volume\n");
return -ENOSPC;
}
- if (ntfs_attrlist_update(base_ni))
+ /*
+ * This call is only ever reached while
+ * rebuilding the $ATTRIBUTE_LIST attribute's
+ * own mapping pairs, which happens underneath
+ * ntfs_attrlist_update_locked() while holding
+ * persist lock.
+ */
+ if (ntfs_attrlist_update_locked(base_ni))
return -EIO;
goto retry;
}
@@ -4185,8 +4238,16 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
if (le64_to_cpu(ctx->attr->data.non_resident.highest_vcn) !=
NTFS_VCN_DELETE_MARK)
continue;
- /* Remove unused attribute record. */
- err = ntfs_attr_record_rm(ctx);
+ /*
+ * Remove unused attribute record. When @ni is the
+ * $ATTRIBUTE_LIST attribute itself, we only get here
+ * underneath ntfs_attrlist_update_locked(), whose
+ * caller already holds
+ * base_ni->attr_list_persist_lock for this
+ * transaction, so tell ntfs_attr_record_rm() not to
+ * recurse into it.
+ */
+ err = ntfs_attr_record_rm(ctx, ni->type == AT_ATTRIBUTE_LIST);
if (err) {
ntfs_error(sb, "Could not remove unused attr");
goto put_err_out;
@@ -5014,30 +5075,42 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
*/
if (attr_ni->type == AT_STANDARD_INFORMATION ||
attr_ni->type == AT_ATTRIBUTE_LIST) {
+ /*
+ * Resizing the $ATTRIBUTE_LIST attribute itself only happens
+ * underneath ntfs_attrlist_update_locked() while already holding
+ * attr_list_persist_lock.
+ */
+ bool persist_locked = attr_ni->type == AT_ATTRIBUTE_LIST;
+
ntfs_attr_put_search_ctx(ctx);
if (!NInoAttrList(base_ni)) {
+ /* This takes attr_list_persist_lock on its own. */
err = ntfs_inode_add_attrlist(base_ni);
if (err)
return err;
}
+ /*
+ * ntfs_inode_free_space() moves attributes out of the base MFT
+ * record, rewriting their ALEs in place. Keep those mutations
+ * and the persist below inside one transaction.
+ */
+ if (!persist_locked)
+ mutex_lock(&base_ni->attr_list_persist_lock);
+
err = ntfs_inode_free_space(base_ni, sizeof(struct attr_record));
if (err) {
+ if (!persist_locked)
+ mutex_unlock(&base_ni->attr_list_persist_lock);
err = -ENOSPC;
ntfs_error(sb,
"Couldn't free space in the MFT record to make attribute list non resident");
return err;
}
- /*
- * Resizing the $ATTRIBUTE_LIST attribute itself only happens
- * underneath ntfs_attrlist_update_locked() while already holding
- * attr_list_persist_lock.
- */
- if (attr_ni->type == AT_ATTRIBUTE_LIST)
- err = ntfs_attrlist_update_locked(base_ni);
- else
- err = ntfs_attrlist_update(base_ni);
+ err = ntfs_attrlist_update_locked(base_ni);
+ if (!persist_locked)
+ mutex_unlock(&base_ni->attr_list_persist_lock);
if (err)
return err;
goto attr_resize_again;
@@ -5085,15 +5158,27 @@ static int ntfs_resident_attr_resize(struct ntfs_inode *attr_ni, const s64 newsi
}
unmap_mft_record(ext_ni);
+ /*
+ * Move the attribute and persist the resulting attribute list as one
+ * transaction, so the ALE rewritten by ntfs_attr_record_move_to()
+ * cannot be copied to disk half-updated. attr_ni->type is neither
+ * $STANDARD_INFORMATION nor $ATTRIBUTE_LIST here (both are handled
+ * above), so we cannot already hold the lock via
+ * ntfs_attrlist_update_locked().
+ */
+ mutex_lock(&base_ni->attr_list_persist_lock);
+
/* Move attribute to it. */
err = ntfs_attr_record_move_to(ctx, ext_ni);
if (err) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
ntfs_error(sb, "Couldn't move attribute to new MFT record");
err = -ENOMEM;
goto put_err_out;
}
- err = ntfs_attrlist_update(base_ni);
+ err = ntfs_attrlist_update_locked(base_ni);
+ mutex_unlock(&base_ni->attr_list_persist_lock);
if (err < 0)
goto put_err_out;
@@ -5454,7 +5539,7 @@ int ntfs_attr_rm(struct ntfs_inode *ni)
}
while (!(err = ntfs_attr_lookup(ni->type, ni->name, ni->name_len,
CASE_SENSITIVE, 0, NULL, 0, ctx))) {
- err = ntfs_attr_record_rm(ctx);
+ err = ntfs_attr_record_rm(ctx, false);
if (err) {
ntfs_error(sb,
"Failed to remove attribute extent. Leaving inconstant metadata.\n");
diff --git a/fs/ntfs/attrib.h b/fs/ntfs/attrib.h
index 2e5f87abd852..ec0c846febf1 100644
--- a/fs/ntfs/attrib.h
+++ b/fs/ntfs/attrib.h
@@ -161,7 +161,7 @@ int ntfs_attr_exist(struct ntfs_inode *ni, const __le32 type, __le16 *name,
u32 name_len);
int ntfs_attr_remove(struct ntfs_inode *ni, const __le32 type, __le16 *name,
u32 name_len);
-int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx);
+int ntfs_attr_record_rm(struct ntfs_attr_search_ctx *ctx, bool persist_locked);
int ntfs_attr_record_move_to(struct ntfs_attr_search_ctx *ctx, struct ntfs_inode *ni);
int ntfs_attr_add(struct ntfs_inode *ni, __le32 type,
__le16 *name, u8 name_len, u8 *val, s64 size);
diff --git a/fs/ntfs/attrlist.c b/fs/ntfs/attrlist.c
index aa6d5f10bf96..bdea09efe284 100644
--- a/fs/ntfs/attrlist.c
+++ b/fs/ntfs/attrlist.c
@@ -117,23 +117,6 @@ int ntfs_attrlist_update_locked(struct ntfs_inode *base_ni)
return 0;
}
-/*
- * ntfs_attrlist_update - persist the in-memory attribute list to disk
- * @base_ni: base ntfs inode containing the attribute list
- *
- * Serialize the persist against concurrent attribute-list replacement
- * transactions.
- */
-int ntfs_attrlist_update(struct ntfs_inode *base_ni)
-{
- int err;
-
- mutex_lock(&base_ni->attr_list_persist_lock);
- err = ntfs_attrlist_update_locked(base_ni);
- mutex_unlock(&base_ni->attr_list_persist_lock);
- return err;
-}
-
/*
* ntfs_attrlist_entry_add - add an attribute list attribute entry
* @ni: opened ntfs inode, which contains that attribute
@@ -328,7 +311,7 @@ int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr)
*
* Return 0 on success and -errno on error.
*/
-int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
+int ntfs_attrlist_entry_rm_locked(struct ntfs_attr_search_ctx *ctx)
{
u8 *new_al = NULL;
int err, new_al_len;
@@ -356,7 +339,7 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
ntfs_debug("Attribute list isn't present.\n");
return -ENOENT;
}
- mutex_lock(&base_ni->attr_list_persist_lock);
+ lockdep_assert_held(&base_ni->attr_list_persist_lock);
/*
* Another thread may have removed the attribute list while we were
@@ -414,6 +397,19 @@ int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
kvfree(old_al);
err = 0;
out_unlock:
+ return err;
+}
+
+int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx)
+{
+ struct ntfs_inode *base_ni;
+ int err;
+
+ if (!ctx || !ctx->ntfs_ino)
+ return -EINVAL;
+ base_ni = ctx->base_ntfs_ino ? ctx->base_ntfs_ino : ctx->ntfs_ino;
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ err = ntfs_attrlist_entry_rm_locked(ctx);
mutex_unlock(&base_ni->attr_list_persist_lock);
return err;
}
diff --git a/fs/ntfs/attrlist.h b/fs/ntfs/attrlist.h
index ecb8a8c957fa..ccf61971c01e 100644
--- a/fs/ntfs/attrlist.h
+++ b/fs/ntfs/attrlist.h
@@ -15,7 +15,7 @@
int ntfs_attrlist_need(struct ntfs_inode *ni);
int ntfs_attrlist_entry_add(struct ntfs_inode *ni, struct attr_record *attr);
int ntfs_attrlist_entry_rm(struct ntfs_attr_search_ctx *ctx);
-int ntfs_attrlist_update(struct ntfs_inode *base_ni);
+int ntfs_attrlist_entry_rm_locked(struct ntfs_attr_search_ctx *ctx);
int ntfs_attrlist_update_locked(struct ntfs_inode *base_ni);
#endif /* defined _NTFS_ATTRLIST_H */
diff --git a/fs/ntfs/index.c b/fs/ntfs/index.c
index 082fe9226c72..8c44ec25c2a2 100644
--- a/fs/ntfs/index.c
+++ b/fs/ntfs/index.c
@@ -1348,18 +1348,35 @@ static int ntfs_ir_reparent(struct ntfs_index_context *icx)
*/
if ((ret == -ENOSPC) &&
(ctx->al_cursor.valid || !ntfs_inode_add_attrlist(icx->idx_ni))) {
+ struct ntfs_inode *base_ni;
+
ntfs_attr_put_search_ctx(ctx);
ctx = NULL;
ir = ntfs_ir_lookup(icx->idx_ni, icx->name, icx->name_len, &ctx);
- if (ir && !ntfs_attr_record_move_away(ctx, ix_root_size -
+ if (!ir)
+ goto clear_bmp;
+
+ base_ni = ctx->base_ntfs_ino ? ctx->base_ntfs_ino : ctx->ntfs_ino;
+ /*
+ * ntfs_attr_record_move_away() rewrites the moved attribute's
+ * ALE in place, so it and the persist below have to form one
+ * transaction. Nothing up this call chain holds the lock:
+ * ntfs_inode_add_attrlist() above takes it on its own, and
+ * ntfs_attrlist_update_locked() never reaches index code.
+ */
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ if (!ntfs_attr_record_move_away(ctx, ix_root_size -
le32_to_cpu(ctx->attr->data.resident.value_length))) {
- if (ntfs_attrlist_update(ctx->base_ntfs_ino ?
- ctx->base_ntfs_ino : ctx->ntfs_ino))
+ if (ntfs_attrlist_update_locked(base_ni)) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
goto clear_bmp;
+ }
+ mutex_unlock(&base_ni->attr_list_persist_lock);
ntfs_attr_put_search_ctx(ctx);
ctx = NULL;
goto retry;
}
+ mutex_unlock(&base_ni->attr_list_persist_lock);
}
clear_bmp:
ntfs_ibm_clear(icx, new_ib_vcn);
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index ede16196ed0c..a304a2ca22d8 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -3054,14 +3054,20 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
ntfs_debug("inode %llu\n", ni->mft_no);
+ /* Serialize the initial state check and attribute-list publication. */
+ mutex_lock(&ni->attr_list_persist_lock);
+
if (NInoAttrList(ni) || ni->nr_extents) {
ntfs_error(ni->vol->sb, "Inode already has attribute list");
- return -EEXIST;
+ err = -EEXIST;
+ goto out_unlock;
}
ni_mrec = map_mft_record(ni);
- if (IS_ERR(ni_mrec))
- return -EIO;
+ if (IS_ERR(ni_mrec)) {
+ err = -EIO;
+ goto out_unlock;
+ }
/* Form attribute list. */
ctx = ntfs_attr_get_search_ctx(ni, ni_mrec);
@@ -3123,9 +3129,12 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
}
/* Set in-memory attribute list. */
+ down_write(&ni->attr_list_lock);
ni->attr_list = al;
ni->attr_list_size = al_len;
+ ni->attr_list_gen++;
NInoSetAttrList(ni);
+ up_write(&ni->attr_list_lock);
attr_al_len = offsetof(struct attr_record, data.resident.reserved) + 1 +
((al_len + 7) & ~7);
@@ -3151,29 +3160,35 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
err = ntfs_attrlist_update_locked(ni);
if (err < 0)
goto remove_attrlist_record;
-
+ mutex_unlock(&ni->attr_list_persist_lock);
ntfs_attr_put_search_ctx(ctx);
unmap_mft_record(ni);
return 0;
remove_attrlist_record:
- /* Prevent ntfs_attr_recorm_rm from freeing attribute list. */
+ down_write(&ni->attr_list_lock);
ni->attr_list = NULL;
+ ni->attr_list_gen++;
NInoClearAttrList(ni);
+ up_write(&ni->attr_list_lock);
+
/* Remove $ATTRIBUTE_LIST record. */
ntfs_attr_reinit_search_ctx(ctx);
if (!ntfs_attr_lookup(AT_ATTRIBUTE_LIST, NULL, 0,
CASE_SENSITIVE, 0, NULL, 0, ctx)) {
- if (ntfs_attr_record_rm(ctx))
+ if (ntfs_attr_record_rm(ctx, true))
ntfs_error(ni->vol->sb, "Rollback failed to remove attrlist");
} else {
ntfs_error(ni->vol->sb, "Rollback failed to find attrlist");
}
/* Setup back in-memory runlist. */
+ down_write(&ni->attr_list_lock);
ni->attr_list = al;
ni->attr_list_size = al_len;
+ ni->attr_list_gen++;
NInoSetAttrList(ni);
+ up_write(&ni->attr_list_lock);
rollback:
/*
* Scan attribute list for attributes that placed not in the base MFT
@@ -3200,15 +3215,21 @@ int ntfs_inode_add_attrlist(struct ntfs_inode *ni)
}
/* Remove in-memory attribute list. */
+ down_write(&ni->attr_list_lock);
ni->attr_list = NULL;
ni->attr_list_size = 0;
+ ni->attr_list_gen++;
NInoClearAttrList(ni);
NInoClearAttrListDirty(ni);
+ up_write(&ni->attr_list_lock);
+
put_err_out:
ntfs_attr_put_search_ctx(ctx);
err_out:
kvfree(al);
unmap_mft_record(ni);
+out_unlock:
+ mutex_unlock(&ni->attr_list_persist_lock);
return err;
}
diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
index 96045face63f..09deb96533d1 100644
--- a/fs/ntfs/namei.c
+++ b/fs/ntfs/namei.c
@@ -938,7 +938,7 @@ static int ntfs_delete(struct ntfs_inode *ni, struct ntfs_inode *dir_ni,
if (err)
goto err_out;
- err = ntfs_attr_record_rm(actx);
+ err = ntfs_attr_record_rm(actx, false);
if (err)
goto err_out;
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 8abe7bee4c0d..6d28c4c25a05 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -485,7 +485,7 @@ int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
ret = ntfs_attr_lookup(AT_VOLUME_NAME, NULL, 0, 0, 0, NULL, 0,
ctx);
if (!ret)
- ret = ntfs_attr_record_rm(ctx);
+ ret = ntfs_attr_record_rm(ctx, false);
else if (ret == -ENOENT)
ret = 0;
ntfs_attr_put_search_ctx(ctx);
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v7 4/4] ntfs: protect mapping-pairs attr-list updates with attr_list_persist_lock
2026-08-12 1:18 [PATCH v7 0/4] ntfs: serialize attribute-list access Hyunchul Lee
` (2 preceding siblings ...)
2026-08-12 1:18 ` [PATCH v7 3/4] ntfs: protect attribute-list creation/teardown " Hyunchul Lee
@ 2026-08-12 1:18 ` Hyunchul Lee
2026-10-17 2:54 ` [PATCH v7 0/4] ntfs: serialize attribute-list access Cen Zhang
4 siblings, 0 replies; 7+ messages in thread
From: Hyunchul Lee @ 2026-08-12 1:18 UTC (permalink / raw)
To: Namjae Jeon; +Cc: Hyunchul Lee, ntfs, Cen Zhang, Copilot, stable
ntfs_attr_update_mapping_pairs() updates several ALEs while rebuilding
a fragmented attribute's mapping pairs and persists the attribute
list once after the loop. Each ALE update is covered by
attr_list_lock, but the overall publish/persist sequence is not
serialized against concurrent ntfs_attrlist_entry_add()/rm() on the
same base inode.
Acquire attr_list_persist_lock once after resolving base_ni whenever
the call can touch the attribute list, and hold it across every ALE
update and the deferred ntfs_attrlist_update() call.
Reported-by: Cen Zhang <zzzccc427@gmail.com>
Link: https://lore.kernel.org/all/20260702161710.2606853-1-zzzccc427@gmail.com/
Fixes: 495e90fa3348 ("ntfs: update attrib operations")
Cc: stable@vger.kernel.org
Tested-by: Cen Zhang <zzzccc427@gmail.com>
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
fs/ntfs/attrib.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
fs/ntfs/inode.c | 46 ++++++++++++++++++++++++++++++++++++++++----
fs/ntfs/inode.h | 1 +
3 files changed, 97 insertions(+), 8 deletions(-)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index e52acbced9c5..d3621e5c24df 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -3717,6 +3717,7 @@ int ntfs_attr_record_move_away(struct ntfs_attr_search_ctx *ctx, int extra)
base_ni = ctx->base_ntfs_ino;
else
base_ni = ctx->ntfs_ino;
+ lockdep_assert_held(&base_ni->attr_list_persist_lock);
sb = ctx->ntfs_ino->vol->sb;
if (!NInoAttrList(base_ni)) {
@@ -3725,7 +3726,7 @@ int ntfs_attr_record_move_away(struct ntfs_attr_search_ctx *ctx, int extra)
return -EINVAL;
}
- err = ntfs_inode_attach_all_extents(ctx->ntfs_ino);
+ err = ntfs_inode_attach_all_extents_locked(ctx->ntfs_ino);
if (err) {
ntfs_error(sb, "Couldn't attach extents, inode=%llu",
(unsigned long long)base_ni->mft_no);
@@ -3789,7 +3790,8 @@ int ntfs_attr_record_move_away(struct ntfs_attr_search_ctx *ctx, int extra)
* update allocated and compressed size.
*/
static int ntfs_attr_update_meta(struct attr_record *a, struct ntfs_inode *ni,
- struct mft_record *m, struct ntfs_attr_search_ctx *ctx)
+ struct mft_record *m, struct ntfs_attr_search_ctx *ctx,
+ bool *attrlist_locked)
{
int sparse, err = 0;
struct ntfs_inode *base_ni;
@@ -3826,6 +3828,19 @@ static int ntfs_attr_update_meta(struct attr_record *a, struct ntfs_inode *ni,
!(le32_to_cpu(m->bytes_allocated) - le32_to_cpu(m->bytes_in_use))) {
if (!NInoAttrList(base_ni)) {
+ /*
+ * ntfs_inode_add_attrlist() acquires
+ * attr_list_persist_lock itself, so drop it
+ * first if our caller took it for us. Nothing
+ * is in flight that needs protecting: without
+ * an attribute list no ALE has been touched
+ * yet. The -EAGAIN below makes the caller
+ * restart and re-acquire the lock.
+ */
+ if (*attrlist_locked) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ *attrlist_locked = false;
+ }
err = ntfs_inode_add_attrlist(base_ni);
if (err)
goto out;
@@ -3952,7 +3967,7 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
struct attr_record *a;
s64 stop_vcn;
int err = 0, mp_size, cur_max_mp_size, exp_max_mp_size;
- bool finished_build, attrlist_changed = false;
+ bool finished_build, attrlist_changed = false, attrlist_locked = false;
bool first_updated = false;
struct super_block *sb;
struct runlist_element *start_rl;
@@ -3977,9 +3992,16 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
else
base_ni = ni;
+ if (ni->type != AT_ATTRIBUTE_LIST) {
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ attrlist_locked = true;
+ }
+
ctx = ntfs_attr_get_search_ctx(base_ni, NULL);
if (!ctx) {
ntfs_error(sb, "%s: Failed to get search context", __func__);
+ if (attrlist_locked)
+ mutex_unlock(&base_ni->attr_list_persist_lock);
return -ENOMEM;
}
@@ -4050,9 +4072,13 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
continue;
}
- err = ntfs_attr_update_meta(a, ni, m, ctx);
+ err = ntfs_attr_update_meta(a, ni, m, ctx, &attrlist_locked);
if (err < 0) {
if (err == -EAGAIN) {
+ if (attrlist_locked) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ attrlist_locked = false;
+ }
ntfs_attr_put_search_ctx(ctx);
goto retry;
}
@@ -4089,6 +4115,10 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
* attributes and try again.
*/
if (ni->type == AT_ATTRIBUTE_LIST) {
+ if (WARN_ON_ONCE(attrlist_locked)) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ attrlist_locked = false;
+ }
ntfs_attr_put_search_ctx(ctx);
if (ntfs_inode_free_space(base_ni, mp_size -
cur_max_mp_size)) {
@@ -4109,6 +4139,18 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
/* Add attribute list if it isn't present, and retry. */
if (!NInoAttrList(base_ni)) {
+ /*
+ * We hold attr_list_persist_lock even when the
+ * inode has no attribute list yet, since one
+ * can appear under us. ntfs_inode_add_attrlist()
+ * takes the same lock, so drop it here; no ALE
+ * has been touched, so there is no transaction
+ * to break, and the retry re-acquires it.
+ */
+ if (attrlist_locked) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ attrlist_locked = false;
+ }
ntfs_attr_put_search_ctx(ctx);
if (ntfs_inode_add_attrlist(base_ni)) {
ntfs_error(sb, "Can not add attrlist");
@@ -4225,6 +4267,12 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
if (err)
goto put_err_out;
}
+ if (attrlist_locked) {
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+ attrlist_locked = false;
+ }
+ if (attrlist_changed && err)
+ goto put_err_out;
/* Deallocate not used attribute extents and return with success. */
if (finished_build) {
@@ -4351,6 +4399,8 @@ int ntfs_attr_update_mapping_pairs(struct ntfs_inode *ni, s64 from_vcn)
return 0;
put_err_out:
+ if (attrlist_locked)
+ mutex_unlock(&base_ni->attr_list_persist_lock);
if (ctx)
ntfs_attr_put_search_ctx(ctx);
return err;
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index a304a2ca22d8..60a1b690dbea 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -2921,6 +2921,7 @@ static struct ntfs_inode *ntfs_extent_inode_open(struct ntfs_inode *base_ni,
if (!base_ni)
return NULL;
+ lockdep_assert_held(&base_ni->extent_lock);
sb = base_ni->vol->sb;
ntfs_debug("Opening extent inode %llu (base mft record %llu).\n",
@@ -2988,15 +2989,20 @@ static struct ntfs_inode *ntfs_extent_inode_open(struct ntfs_inode *base_ni,
}
/*
- * ntfs_inode_attach_all_extents - attach all extents for target inode
+ * ntfs_inode_attach_all_extents_locked - attach all extents for target inode
* @ni: opened ntfs inode for which perform attach
*
+ * Caller must hold the base inode's attr_list_persist_lock. The attribute
+ * list buffer is replaced and freed by transactions serialized by this lock,
+ * so walking it without the lock can dereference a stale buffer.
+ *
* Return 0 on success and error.
*/
-int ntfs_inode_attach_all_extents(struct ntfs_inode *ni)
+int ntfs_inode_attach_all_extents_locked(struct ntfs_inode *ni)
{
struct attr_list_entry *ale;
u64 prev_attached = 0;
+ int err = 0;
if (!ni) {
ntfs_debug("Invalid arguments.\n");
@@ -3005,6 +3011,7 @@ int ntfs_inode_attach_all_extents(struct ntfs_inode *ni)
if (NInoAttr(ni))
ni = ni->ext.base_ntfs_ino;
+ lockdep_assert_held(&ni->attr_list_persist_lock);
ntfs_debug("Entering for inode 0x%llx.\n", ni->mft_no);
@@ -3018,19 +3025,50 @@ int ntfs_inode_attach_all_extents(struct ntfs_inode *ni)
}
/* Walk through attribute list and attach all extents. */
+ mutex_lock(&ni->extent_lock);
ale = (struct attr_list_entry *)ni->attr_list;
while ((u8 *)ale < ni->attr_list + ni->attr_list_size) {
if (ni->mft_no != MREF_LE(ale->mft_reference) &&
prev_attached != MREF_LE(ale->mft_reference)) {
if (!ntfs_extent_inode_open(ni, ale->mft_reference)) {
ntfs_debug("Couldn't attach extent inode.\n");
- return -1;
+ err = -1;
+ break;
}
prev_attached = MREF_LE(ale->mft_reference);
}
ale = (struct attr_list_entry *)((u8 *)ale + le16_to_cpu(ale->length));
}
- return 0;
+ mutex_unlock(&ni->extent_lock);
+ return err;
+}
+
+/*
+ * ntfs_inode_attach_all_extents - attach all extents for target inode
+ * @ni: opened ntfs inode for which perform attach
+ *
+ * Return 0 on success and error.
+ */
+int ntfs_inode_attach_all_extents(struct ntfs_inode *ni)
+{
+ struct ntfs_inode *base_ni;
+ int err;
+
+ if (!ni) {
+ ntfs_debug("Invalid arguments.\n");
+ return -EINVAL;
+ }
+
+ if (NInoAttr(ni))
+ base_ni = ni->ext.base_ntfs_ino;
+ else
+ base_ni = ni;
+
+ mutex_lock(&base_ni->attr_list_persist_lock);
+ err = ntfs_inode_attach_all_extents_locked(base_ni);
+ mutex_unlock(&base_ni->attr_list_persist_lock);
+
+ return err;
}
/*
diff --git a/fs/ntfs/inode.h b/fs/ntfs/inode.h
index a6c5b15d648d..a02ef1e59044 100644
--- a/fs/ntfs/inode.h
+++ b/fs/ntfs/inode.h
@@ -346,6 +346,7 @@ int ntfs_getattr(struct mnt_idmap *idmap, const struct path *path,
int ntfs_get_block_mft_record(struct ntfs_inode *mft_ni, struct ntfs_inode *ni);
int __ntfs_write_inode(struct inode *vi, int sync);
int ntfs_inode_attach_all_extents(struct ntfs_inode *ni);
+int ntfs_inode_attach_all_extents_locked(struct ntfs_inode *ni);
int ntfs_inode_add_attrlist(struct ntfs_inode *ni);
void ntfs_destroy_ext_inode(struct ntfs_inode *ni);
int ntfs_inode_free_space(struct ntfs_inode *ni, int size);
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v7 0/4] ntfs: serialize attribute-list access
2026-08-12 1:18 [PATCH v7 0/4] ntfs: serialize attribute-list access Hyunchul Lee
` (3 preceding siblings ...)
2026-08-12 1:18 ` [PATCH v7 4/4] ntfs: protect mapping-pairs attr-list updates " Hyunchul Lee
@ 2026-10-17 2:54 ` Cen Zhang
2026-10-19 6:23 ` Hyunchul Lee
4 siblings, 1 reply; 7+ messages in thread
From: Cen Zhang @ 2026-10-17 2:54 UTC (permalink / raw)
To: Hyunchul Lee; +Cc: Namjae Jeon, ntfs, Copilot, stable
Hi Hyunchul,
Hyunchul Lee <hyc.lee@gmail.com> 于2026年8月12日周三 09:18写道：
>
> 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/20260702161710.2606853-1-zzzccc427@gmail.com/
>
> 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 attr_list_lock, an rwsem that protects the live buffer,
> its size and generation, and in-place ALE changes. Readers hold it
> only while dereferencing the buffer; writers hold it while replacing the
> buffer or changing an ALE, then increment the generation. Pointers into
> the buffer never survive an unlock, and the rwsem is dropped before
> persistence or an operation that can take runlist.lock or extent_lock.
>
> attr_list_persist_lock is a non-recursive mutex that serializes a
> mutation through persistence and rollback, including list creation and
> teardown. ntfs_attrlist_update_locked() writes the live buffer without
> attr_list_lock, so the caller holds this mutex from mutation through the
> write. It also stabilizes the buffer while attaching extents. When both
> locks are needed, take attr_list_persist_lock first. Creation owns the
> mutex itself, so callers must create first and then restart; re-entrant
> teardown uses persist_locked instead of recursively acquiring it.
>
> ---
> * Changes in v7:
> * Patch 3: serialize non-$ATTRIBUTE_LIST removal with concurrent
> attribute-list creation.
> * Patch 3: initialize successful removal results and release
> attr_list_persist_lock on every early creation error.
> * Changes in v6:
> * Patch 2: initialize new_al before an early error path can reach its
> cleanup, fixing an x86_64 allmodconfig Clang build warning.
> * Changes in v5:
> * Patch 3: keep each in-place ALE update and its persist in one
> transaction. This fixes the Patch 2 review: a record move could
> update an ALE while persistence copied the same buffer.
> * Patch 3: serialize attribute-list creation, teardown, and extent
> attachment.
> * Patch 4: acquire the mutex unconditionally for mapping-pairs updates to
> close the attribute-list creation TOCTOU; drop it before creating a
> missing list and restart afterward to avoid self-deadlock.
> * Changes in v4:
> * Patch 1: fix attr-list lock/unlock and uninitialized locator on
> the AT_END path
> * Patch 2: hold attr_list_persist_lock around all
> ntfs_attrlist_update() callers and recheck the attribute-list
> state after locking
> * Patch 4: avoid self-deadlock rebuilding $ATTRIBUTE_LIST's own
> mapping pairs
> * Changes in v3:
> * Patch 1: fix $ATTRIBUTE_LIST extent double-delivery during
> mapping-pairs rebuild (xfstests generic/040)
> * Patch 3: fix self-deadlock on attr_list_persist_lock during
> attr-list rebuild (xfstests generic/040)
> * Changes in v2:
> * add Fixes tags
> * add Reported-by and Tested-by tags
>
> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
>
> ---
> Hyunchul Lee (4):
> ntfs: replace raw ctx->al_entry with restartable attr-list locators
> ntfs: protect attribute-list buffer replacement with attr_list_persist_lock
> ntfs: protect attribute-list creation/teardown with attr_list_persist_lock
> ntfs: protect mapping-pairs attr-list updates with attr_list_persist_lock
>
> fs/ntfs/attrib.c | 568 ++++++++++++++++++++++++++++++++++++++++++++---------
> fs/ntfs/attrib.h | 48 ++++-
> fs/ntfs/attrlist.c | 181 +++++++++++++----
> fs/ntfs/attrlist.h | 3 +-
> fs/ntfs/index.c | 26 ++-
> fs/ntfs/inode.c | 116 ++++++++---
> fs/ntfs/inode.h | 10 +
> fs/ntfs/namei.c | 2 +-
> fs/ntfs/super.c | 2 +-
> 9 files changed, 795 insertions(+), 161 deletions(-)
> ---
> base-commit: 5c244a0e9ca95468f4c2087b2c9269825429a13f
> change-id: 20260727-topic-attr-list-lock-v6-f1a909e3c859
>
> Best regards,
> --
> Thanks,
> Hyunchul
>
Just checking if there is any update on the v7 series, or if the issue has
been resolved otherwise.
Thanks again.
Best regards,
Cen Zhang
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v7 0/4] ntfs: serialize attribute-list access
2026-10-17 2:54 ` [PATCH v7 0/4] ntfs: serialize attribute-list access Cen Zhang
@ 2026-10-19 6:23 ` Hyunchul Lee
0 siblings, 0 replies; 7+ messages in thread
From: Hyunchul Lee @ 2026-10-19 6:23 UTC (permalink / raw)
To: Cen Zhang; +Cc: Namjae Jeon, ntfs, stable
Hi Cen,
>
> Just checking if there is any update on the v7 series, or if the issue has
> been resolved otherwise.
I am prepairing a v8 version of this series. I will share the patch
as it is ready.
>
> Thanks again.
>
> Best regards,
> Cen Zhang
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-10-19 6:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-12 1:18 [PATCH v7 0/4] ntfs: serialize attribute-list access Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 1/4] ntfs: replace raw ctx->al_entry with restartable attr-list locators Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 2/4] ntfs: protect attribute-list buffer replacement with attr_list_persist_lock Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 3/4] ntfs: protect attribute-list creation/teardown " Hyunchul Lee
2026-08-12 1:18 ` [PATCH v7 4/4] ntfs: protect mapping-pairs attr-list updates " Hyunchul Lee
2026-10-17 2:54 ` [PATCH v7 0/4] ntfs: serialize attribute-list access Cen Zhang
2026-10-19 6:23 ` Hyunchul Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox