* [PATCH v2 1/3] ntfs: fix volume flag update races
@ 2026-09-03 7:54 Hongling Zeng
2026-09-03 7:54 ` [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit Hongling Zeng
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Hongling Zeng @ 2026-09-03 7:54 UTC (permalink / raw)
To: linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, Hongling Zeng, stable
ntfs_set_volume_flags() and ntfs_clear_volume_flags() both read
vol->vol_flags outside any lock to compute the new value before handing
it to ntfs_write_volume_flags(), which only takes ni->mrec_lock around
the actual write. The read-modify-write is therefore not atomic, and two
concurrent callers can lose an update: ntfs_sync_fs() may derive a
"clean" value from vol->vol_flags while a writer concurrently records an
error and sets VOLUME_IS_DIRTY; the locked write then silently
overwrites the freshly-set dirty bit. The on-disk volume looks clean
despite the recorded errors, so chkdsk will not run on the next mount
and corrupted metadata can persist.
Fix by moving the read-modify-write inside the mrec_lock: pass the bits
to set and to clear separately, and combine them with the current flag
state under the lock inside ntfs_write_volume_flags(). The set/clear
helpers pass only the bits to modify, not the complete flag state. The
bit manipulation is done on CPU-endian values, and the result is
converted back to little-endian before storing it. The wrappers keep
their signatures so callers are unchanged.
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
- Also fix the ntfs_sync_fs() race by checking NVolErrors() and clearing
VOLUME_IS_DIRTY under ni->mrec_lock.
- Keep ntfs_set_volume_flags() and ntfs_clear_volume_flags() semantics
unchanged.
- Do not tie setting VOLUME_IS_DIRTY to NVolSetErrors() in the generic
set helper.
---
fs/ntfs/super.c | 62 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 45 insertions(+), 17 deletions(-)
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index a1813093222b..a7977b95b967 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -353,31 +353,45 @@ void ntfs_handle_error(struct super_block *sb)
}
/*
- * ntfs_write_volume_flags - write new flags to the volume information flags
+ * ntfs_write_volume_flags - apply flag changes to the volume information flags
* @vol: ntfs volume on which to modify the flags
- * @flags: new flags value for the volume information flags
+ * @set_bits: bits to set in the volume information flags
+ * @clear_bits: bits to clear in the volume information flags
*
* Internal function. You probably want to use ntfs_{set,clear}_volume_flags()
* instead (see below).
*
- * Replace the volume information flags on the volume @vol with the value
- * supplied in @flags. Note, this overwrites the volume information flags, so
- * make sure to combine the flags you want to modify with the old flags and use
- * the result when calling ntfs_write_volume_flags().
+ * Combine @set_bits and @clear_bits with the current in-memory flag state and
+ * write the result back. The set/clear helpers pass only the bits to modify,
+ * not the complete flag state. The read-modify-write happens under
+ * ni->mrec_lock so that concurrent set/clear operations cannot lose updates.
+ * All bit manipulation is done on CPU-endian values, and the result is
+ * converted back to little-endian before storing it.
*
* Return 0 on success and -errno on error.
*/
-static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
+static int ntfs_write_volume_flags(struct ntfs_volume *vol,
+ const __le16 set_bits, const __le16 clear_bits,
+ const bool skip_if_errors)
{
struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
struct volume_information *vi;
struct ntfs_attr_search_ctx *ctx;
+ u16 flags;
int err;
- ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
- le16_to_cpu(vol->vol_flags), le16_to_cpu(flags));
mutex_lock(&ni->mrec_lock);
- if (vol->vol_flags == flags)
+
+ if (skip_if_errors && NVolErrors(vol))
+ goto done;
+
+ flags = le16_to_cpu(vol->vol_flags);
+ flags |= le16_to_cpu(set_bits) & le16_to_cpu(VOLUME_FLAGS_MASK);
+ flags &= ~(le16_to_cpu(clear_bits) & le16_to_cpu(VOLUME_FLAGS_MASK));
+ ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
+ le16_to_cpu(vol->vol_flags), flags);
+
+ if (le16_to_cpu(vol->vol_flags) == flags)
goto done;
ctx = ntfs_attr_get_search_ctx(ni, NULL);
@@ -393,7 +407,7 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
vi = (struct volume_information *)((u8 *)ctx->attr +
le16_to_cpu(ctx->attr->data.resident.value_offset));
- vol->vol_flags = vi->flags = flags;
+ vol->vol_flags = vi->flags = cpu_to_le16(flags);
mark_mft_record_dirty(ctx->ntfs_ino);
ntfs_attr_put_search_ctx(ctx);
done:
@@ -414,13 +428,14 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
* @flags: flags to set on the volume
*
* Set the bits in @flags in the volume information flags on the volume @vol.
+ * The bits are combined with the current flag state under the lock in
+ * ntfs_write_volume_flags(), so concurrent updates are not lost.
*
* Return 0 on success and -errno on error.
*/
int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
{
- flags &= VOLUME_FLAGS_MASK;
- return ntfs_write_volume_flags(vol, vol->vol_flags | flags);
+ return ntfs_write_volume_flags(vol, flags, 0, false);
}
/*
@@ -429,14 +444,27 @@ int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
* @flags: flags to clear on the volume
*
* Clear the bits in @flags in the volume information flags on the volume @vol.
+ * The bits are combined with the current flag state under the lock in
+ * ntfs_write_volume_flags(), so concurrent updates are not lost.
*
* Return 0 on success and -errno on error.
*/
int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
{
- flags &= VOLUME_FLAGS_MASK;
- flags = vol->vol_flags & cpu_to_le16(~le16_to_cpu(flags));
- return ntfs_write_volume_flags(vol, flags);
+ return ntfs_write_volume_flags(vol, 0, flags, false);
+}
+
+/*
+ * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
+ * @vol: ntfs volume whose dirty bit should be cleared
+ *
+ * Check NVolErrors() and clear VOLUME_IS_DIRTY under the same mrec_lock so
+ * ntfs_sync_fs() cannot clear the dirty bit after a concurrent error has been
+ * recorded.
+ */
+static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
+{
+ return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
}
int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
@@ -1862,7 +1890,7 @@ static int ntfs_sync_fs(struct super_block *sb, int wait)
return 0;
/* If there are some dirty buffers in the bdev inode */
- if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY)) {
+ if (ntfs_clear_volume_dirty_if_no_errors(vol)) {
ntfs_warning(sb, "Failed to clear dirty bit in volume information flags. Run chkdsk.");
err = -EIO;
}
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit
2026-09-03 7:54 [PATCH v2 1/3] ntfs: fix volume flag update races Hongling Zeng
@ 2026-09-03 7:54 ` Hongling Zeng
2026-09-04 12:32 ` liubaolin
2026-09-03 7:54 ` [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors Hongling Zeng
2026-09-04 12:06 ` [PATCH v2 1/3] ntfs: fix volume flag update races liubaolin
2 siblings, 1 reply; 7+ messages in thread
From: Hongling Zeng @ 2026-09-03 7:54 UTC (permalink / raw)
To: linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, Hongling Zeng, stable
The runtime metadata-corruption paths in fs/ntfs record the error flag
with a bare NVolSetErrors() and separately rely on ntfs_set_volume_flags()
or nothing at all to persist VOLUME_IS_DIRTY. Since the clear side
(ntfs_sync_fs() via ntfs_clear_volume_dirty_if_no_errors()) evaluates
NVolErrors() under ni->mrec_lock, a writer that records the error flag
after the lock has been checked leaves a window in which the volume ends
up persisted as clean despite the recorded error, so chkdsk will not run
on the next mount and corrupted metadata can persist.
Introduce ntfs_mark_volume_dirty_with_error(), which records the error
flag before the locked dirty-bit write: the mutex acquire/release in
ntfs_write_volume_flags() then provides the ordering, as any clear path
running after us can only evaluate NVolErrors() under the lock once the
flag is set.
On a read-only mount or before the $Volume inode has been loaded, the
helper only records the in-memory error state because the dirty bit
cannot be persisted. On a writable volume it waits for the lock instead
of silently dropping the dirty-bit update, and propagates any write
error to the caller.
Waiting is safe: the $Volume mrec_lock is a leaf lock. Its critical
sections only look up and update the resident volume information
attribute and never acquire the locks the callers hold (lcnbmp_lock,
runlist locks), so no lock cycle exists. To make that explicit to the
lock validator, give the $Volume inode its own mrec_lock class, the
same way $MFTMirr already gets one; otherwise the lockdep class shared
with regular file inodes would report an inversion for the
lcnbmp_lock -> $Volume mrec_lock ordering that the helper introduces,
even though no deadlock is possible.
Mount and remount paths remain serialized by sb->s_umount and keep
using NVolSetErrors() directly, since they do not race with
ntfs_sync_fs() and may not be allowed to write the volume: before
$Volume is loaded, with a read-only opened bdev, or on a hibernated
volume, which we must not write to at all. The same applies to the
ntfs_attr_lookup() failure paths and the mft record writeback paths,
which hold a caller's mrec_lock and would self-deadlock the helper on
the $Volume inode.
Converting the runtime metadata-corruption call sites in mft.c,
lcnalloc.c, bitmap.c, inode.c and attrib.c to use the helper is done
in the next patch.
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
fs/ntfs/ntfs.h | 1 +
fs/ntfs/super.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/fs/ntfs/ntfs.h b/fs/ntfs/ntfs.h
index df5a75d506f6..63d7900b6dba 100644
--- a/fs/ntfs/ntfs.h
+++ b/fs/ntfs/ntfs.h
@@ -222,6 +222,7 @@ struct option_t {
extern const struct option_t on_errors_arr[];
int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags);
int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags);
+int ntfs_mark_volume_dirty_with_error(struct ntfs_volume *vol);
int ntfs_write_volume_label(struct ntfs_volume *vol, char *label);
/* From fs/ntfs/mst.c */
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index a7977b95b967..64f8d4469af3 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -454,6 +454,45 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
return ntfs_write_volume_flags(vol, 0, flags, false);
}
+/*
+ * ntfs_mark_volume_dirty_with_error - record an error and mark volume dirty
+ * @vol: ntfs volume on which an error has been recorded
+ *
+ * To be called when runtime metadata corruption is detected so that chkdsk
+ * runs on the next mount. NVolErrors() is recorded first, so the in-memory
+ * error state is kept even when the dirty bit cannot be persisted; the
+ * mutex acquire/release in ntfs_write_volume_flags() then provides the
+ * ordering against ntfs_clear_volume_dirty_if_no_errors(), which checks
+ * the flag under the same lock.
+ *
+ * On a read-only mount or before the $Volume inode has been loaded, it
+ * only records the in-memory error state because the dirty bit cannot be
+ * persisted. On a writable volume it waits for the lock instead of
+ * silently dropping the dirty-bit update, and propagates any write error
+ * to the caller.
+ *
+ * Only for runtime error paths that can race sync and hold no mrec_lock.
+ * The $Volume mrec_lock is a leaf lock: its critical sections never
+ * acquire the locks those paths hold (lcnbmp_lock, runlist locks), so
+ * blocking here cannot deadlock. Do not call while holding the $Volume
+ * mrec_lock itself (ntfs_attr_lookup() failure and mft writeback paths),
+ * on mount/remount paths (serialized by sb->s_umount, keep using
+ * NVolSetErrors()), or on volumes we must not write to, such as
+ * hibernated ones.
+ *
+ * Return 0 on success and -errno on error.
+ */
+int ntfs_mark_volume_dirty_with_error(struct ntfs_volume *vol)
+{
+ NVolSetErrors(vol);
+
+ /* Nothing to persist on a read-only or still-mounting volume. */
+ if (!vol->vol_ino || sb_rdonly(vol->sb))
+ return 0;
+
+ return ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
+}
+
/*
* ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
* @vol: ntfs volume whose dirty bit should be cleared
@@ -918,7 +957,8 @@ static void ntfs_setup_allocators(struct ntfs_volume *vol)
}
static struct lock_class_key mftmirr_runlist_lock_key,
- mftmirr_mrec_lock_key;
+ mftmirr_mrec_lock_key,
+ volume_mrec_lock_key;
/*
* load_and_init_mft_mirror - load and setup the mft mirror inode for a volume
* @vol: ntfs super block describing device whose mft mirror to load
@@ -1502,6 +1542,15 @@ static bool load_system_files(struct ntfs_volume *vol)
ntfs_error(sb, "Failed to load $Volume.");
goto iput_lcnbmp_err_out;
}
+ /*
+ * Give the $Volume inode its own mrec_lock class: the error paths
+ * take it while holding locks (lcnbmp_lock, runlist locks) that
+ * nest inside mrec_lock elsewhere in the driver, and the lock
+ * validator would otherwise see that as an inversion even though
+ * the $Volume critical sections never acquire those locks.
+ */
+ lockdep_set_class(&NTFS_I(vol->vol_ino)->mrec_lock,
+ &volume_mrec_lock_key);
m = map_mft_record(NTFS_I(vol->vol_ino));
if (IS_ERR(m)) {
iput_volume_failed:
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors
2026-09-03 7:54 [PATCH v2 1/3] ntfs: fix volume flag update races Hongling Zeng
2026-09-03 7:54 ` [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit Hongling Zeng
@ 2026-09-03 7:54 ` Hongling Zeng
2026-09-04 12:59 ` liubaolin
2026-09-04 13:07 ` liubaolin
2026-09-04 12:06 ` [PATCH v2 1/3] ntfs: fix volume flag update races liubaolin
2 siblings, 2 replies; 7+ messages in thread
From: Hongling Zeng @ 2026-09-03 7:54 UTC (permalink / raw)
To: linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, Hongling Zeng, stable
Follow-up to the helper introduction: the runtime metadata-corruption
paths in mft.c, lcnalloc.c, bitmap.c and inode.c still record the error
flag with a bare NVolSetErrors(), leaving the same window in which
ntfs_sync_fs() can acquire ni->mrec_lock, find NVolErrors() unset, and
persist the volume as clean before the erroring thread records its
error.
Convert the call sites that can run concurrently with sync and hold no
mrec_lock:
- the mft record allocation and $MFT data/$BITMAP extend rollback
paths (mft.c), which hold the runlist, mftbmp and lcnbmp locks;
- the cluster allocation/free rollback paths (lcnalloc.c), which
hold vol->lcnbmp_lock;
- the bitmap rollback failure path (bitmap.c), which holds no
relevant lock;
- the inode read and writeback failure paths (inode.c), which run
under I_NEW or with the mrec_lock already released at err_out.
Blocking on the $Volume mrec_lock from those paths is safe: it is a
leaf lock, its critical sections never acquire the locks the callers
hold, and it carries its own lockdep class so the validator does not
report an inversion against the shared mrec_lock class.
The remaining call sites keep calling NVolSetErrors() directly, now
with comments explaining why:
- the mft record writeback paths (write_mft_record_nolock() and
ntfs_write_mft_block()) can run with a caller's mrec_lock held,
which for the $Volume inode itself would self-deadlock the helper;
- the ntfs_attr_lookup() failure paths (ntfs_attr_find(),
ntfs_external_attr_find() and the ntfs_attr_make_non_resident()
rollback) run with the caller's mrec_lock held for the same
reason.
Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
fs/ntfs/attrib.c | 18 ++++++++++++++-
fs/ntfs/bitmap.c | 2 +-
fs/ntfs/inode.c | 8 +++----
fs/ntfs/lcnalloc.c | 4 ++--
fs/ntfs/mft.c | 55 ++++++++++++++++++++++++++++------------------
5 files changed, 58 insertions(+), 29 deletions(-)
diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
index 60264833bb63..5965a67b5971 100644
--- a/fs/ntfs/attrib.c
+++ b/fs/ntfs/attrib.c
@@ -964,6 +964,11 @@ static int ntfs_attr_find(const __le32 type, const __le16 *name,
}
ntfs_error(vol->sb, "mft %#llx, type %#x is corrupt. Run chkdsk.",
(long long)ctx->ntfs_ino->mft_no, le32_to_cpu(type));
+ /*
+ * ntfs_attr_lookup() runs with the caller's mrec_lock held, so the
+ * dirty bit cannot be written here (it would self-deadlock for the
+ * $Volume inode); record the error flag only.
+ */
NVolSetErrors(vol);
return -EIO;
}
@@ -1501,8 +1506,14 @@ static int ntfs_external_attr_find(const __le32 type,
err = -EIO;
}
- if (err != -ENOMEM)
+ if (err != -ENOMEM) {
+ /*
+ * ntfs_attr_lookup() runs with the caller's mrec_lock
+ * held, so the dirty bit cannot be written here; record
+ * the error flag only.
+ */
NVolSetErrors(vol);
+ }
return err;
not_found:
/*
@@ -2236,6 +2247,11 @@ int ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size)
if (ntfs_cluster_free_from_rl(vol, rl) < 0) {
ntfs_error(vol->sb,
"Failed to release allocated cluster(s) in error code path. Run chkdsk to recover the lost cluster(s).");
+ /*
+ * The caller may hold the mrec_lock of the inode
+ * being modified, so the dirty bit cannot be
+ * written here; record the error flag only.
+ */
NVolSetErrors(vol);
}
kvfree(rl);
diff --git a/fs/ntfs/bitmap.c b/fs/ntfs/bitmap.c
index b1436b3151b9..6120cb301665 100644
--- a/fs/ntfs/bitmap.c
+++ b/fs/ntfs/bitmap.c
@@ -284,7 +284,7 @@ int __ntfs_bitmap_set_bits_in_run(struct inode *vi, const s64 start_bit,
ntfs_error(vi->i_sb,
"Failed to map subsequent page (error %i) and rollback failed (error %i). Aborting and leaving inconsistent metadata. Unmount and run chkdsk.",
err, pos);
- NVolSetErrors(NTFS_SB(vi->i_sb));
+ ntfs_mark_volume_dirty_with_error(NTFS_SB(vi->i_sb));
}
return err;
}
diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
index 32edb4045178..0288f6a101dc 100644
--- a/fs/ntfs/inode.c
+++ b/fs/ntfs/inode.c
@@ -1245,7 +1245,7 @@ static int ntfs_read_locked_inode(struct inode *vi)
ntfs_error(vol->sb,
"Failed with error code %i. Marking corrupt inode 0x%llx as bad. Run chkdsk.",
err, ni->mft_no);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
return err;
}
@@ -1473,7 +1473,7 @@ static int ntfs_read_locked_attr_inode(struct inode *base_vi, struct inode *vi)
err, ni->mft_no, ni->type, ni->name_len,
base_ni->mft_no);
if (err != -ENOENT && err != -ENOMEM)
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
return err;
}
@@ -1725,7 +1725,7 @@ static int ntfs_read_locked_index_inode(struct inode *base_vi, struct inode *vi)
"Failed with error code %i while reading index inode (mft_no 0x%llx, name_len %i.",
err, ni->mft_no, ni->name_len);
if (err != -EOPNOTSUPP && err != -ENOMEM)
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
return err;
}
@@ -2883,7 +2883,7 @@ int __ntfs_write_inode(struct inode *vi, int sync)
mark_inode_dirty(vi);
else {
ntfs_error(vi->i_sb, "Failed (error %i): Run chkdsk.", -err);
- NVolSetErrors(ni->vol);
+ ntfs_mark_volume_dirty_with_error(ni->vol);
}
if (need_iput)
iput(vi);
diff --git a/fs/ntfs/lcnalloc.c b/fs/ntfs/lcnalloc.c
index aa2e017a4384..dd6854f16136 100644
--- a/fs/ntfs/lcnalloc.c
+++ b/fs/ntfs/lcnalloc.c
@@ -763,7 +763,7 @@ switch_to_data1_zone: search_zone = 2;
ntfs_error(vol->sb,
"Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
err2);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
/* Free the runlist. */
kvfree(rl);
@@ -1044,7 +1044,7 @@ s64 __ntfs_cluster_free(struct ntfs_inode *ni, const s64 start_vcn, s64 count,
ntfs_error(vol->sb,
"Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
(int)delta);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
ntfs_dec_free_clusters(vol, delta);
up_write(&vol->lcnbmp_lock);
diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 984a0827f9ac..d8791d7b9678 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -657,8 +657,15 @@ int write_mft_record_nolock(struct ntfs_inode *ni, struct mft_record *m, int syn
"Not enough memory to write mft record. Redirtying so the write is retried later.");
mark_mft_record_dirty(ni);
err = 0;
- } else
+ } else {
+ /*
+ * The writeback path can run with the caller's mrec_lock
+ * held, so the dirty bit cannot be written here (it would
+ * self-deadlock for the $Volume inode); record the error
+ * flag only.
+ */
NVolSetErrors(vol);
+ }
return err;
}
@@ -1150,7 +1157,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
if (ntfs_cluster_free_from_rl(vol, rl2)) {
ntfs_error(vol->sb, "Failed to deallocate allocated cluster.%s",
es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
kvfree(rl2);
return PTR_ERR(rl);
@@ -1285,7 +1292,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
* The only thing that is now wrong is ->allocated_size of the
* base attribute extent which chkdsk should be able to fix.
*/
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
return ret;
}
a = ctx->attr;
@@ -1306,7 +1313,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
down_write(&vol->lcnbmp_lock);
if (ntfs_bitmap_clear_bit(vol->lcnbmp_ino, lcn)) {
ntfs_error(vol->sb, "Failed to free allocated cluster.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
} else
ntfs_inc_free_clusters(vol, 1);
up_write(&vol->lcnbmp_lock);
@@ -1317,16 +1324,16 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
a->data.non_resident.mapping_pairs_offset),
rl2, ll, -1, NULL, NULL, NULL)) {
ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
mark_mft_record_dirty(ctx->ntfs_ino);
} else if (status.mp_extended && ntfs_attr_update_mapping_pairs(mftbmp_ni, 0)) {
ntfs_error(vol->sb, "Failed to restore mapping pairs.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (ctx)
ntfs_attr_put_search_ctx(ctx);
@@ -1420,20 +1427,20 @@ static int ntfs_mft_bitmap_extend_initialized_nolock(struct ntfs_volume *vol)
mrec = map_mft_record(mft_ni);
if (IS_ERR(mrec)) {
ntfs_error(vol->sb, "Failed to map mft record.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
return ret;
}
ctx = ntfs_attr_get_search_ctx(mft_ni, mrec);
if (unlikely(!ctx)) {
ntfs_error(vol->sb, "Failed to get search context.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
goto unm_err_out;
}
if (ntfs_attr_lookup(mftbmp_ni->type, mftbmp_ni->name,
mftbmp_ni->name_len, CASE_SENSITIVE, 0, NULL, 0, ctx)) {
ntfs_error(vol->sb,
"Failed to find first attribute extent of mft bitmap attribute.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
put_err_out:
ntfs_attr_put_search_ctx(ctx);
unm_err_out:
@@ -1587,7 +1594,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
if (ntfs_cluster_free_from_rl(vol, rl2)) {
ntfs_error(vol->sb,
"Failed to deallocate clusters from the mft data attribute.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
kvfree(rl2);
return PTR_ERR(rl);
@@ -1721,7 +1728,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
* The only thing that is now wrong is ->allocated_size of the
* base attribute extent which chkdsk should be able to fix.
*/
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
return ret;
}
ctx->attr->data.non_resident.highest_vcn =
@@ -1729,17 +1736,17 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
undo_alloc:
if (ntfs_cluster_free(mft_ni, old_last_vcn, -1, ctx) < 0) {
ntfs_error(vol->sb, "Failed to free clusters from mft data attribute.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
ntfs_error(vol->sb, "Failed to truncate mft data attribute runlist.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (mp_extended && ntfs_attr_update_mapping_pairs(mft_ni, 0)) {
ntfs_error(vol->sb, "Failed to restore mapping pairs.%s",
es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (ctx) {
a = ctx->attr;
@@ -1750,16 +1757,16 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
a->data.non_resident.mapping_pairs_offset),
rl2, ll, -1, NULL, NULL, NULL)) {
ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
mark_mft_record_dirty(ctx->ntfs_ino);
} else if (IS_ERR(ctx->mrec)) {
ntfs_error(vol->sb, "Failed to restore attribute search context.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
ntfs_attr_put_search_ctx(ctx);
}
@@ -2322,7 +2329,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
folio_unlock(folio);
kunmap_local(m);
folio_put(folio);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
goto search_free_rec;
}
/*
@@ -2475,7 +2482,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
undo_mftbmp_alloc_nolock:
if (ntfs_bitmap_clear_bit(vol->mftbmp_ino, bit)) {
ntfs_error(vol->sb, "Failed to clear bit in mft bitmap.%s", es);
- NVolSetErrors(vol);
+ ntfs_mark_volume_dirty_with_error(vol);
}
if (!base_ni || base_ni->mft_no != FILE_MFT)
up_write(&vol->mftbmp_lock);
@@ -2815,8 +2822,14 @@ static int ntfs_write_mft_block(struct folio *folio, struct writeback_control *w
iput(ref_inos[nr_ref_inos]);
}
- if (unlikely(err && err != -ENOMEM))
+ if (unlikely(err && err != -ENOMEM)) {
+ /*
+ * The writeback path can run with a caller's mrec_lock
+ * held, so the dirty bit cannot be written here; record
+ * the error flag only.
+ */
NVolSetErrors(vol);
+ }
if (likely(!err))
ntfs_debug("Done.");
return err;
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] ntfs: fix volume flag update races
2026-09-03 7:54 [PATCH v2 1/3] ntfs: fix volume flag update races Hongling Zeng
2026-09-03 7:54 ` [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit Hongling Zeng
2026-09-03 7:54 ` [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors Hongling Zeng
@ 2026-09-04 12:06 ` liubaolin
2 siblings, 0 replies; 7+ messages in thread
From: liubaolin @ 2026-09-04 12:06 UTC (permalink / raw)
To: Hongling Zeng, linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, stable
在 2026/9/3 15:54, Hongling Zeng 写道:
> ntfs_set_volume_flags() and ntfs_clear_volume_flags() both read
> vol->vol_flags outside any lock to compute the new value before handing
> it to ntfs_write_volume_flags(), which only takes ni->mrec_lock around
> the actual write. The read-modify-write is therefore not atomic, and two
> concurrent callers can lose an update: ntfs_sync_fs() may derive a
> "clean" value from vol->vol_flags while a writer concurrently records an
> error and sets VOLUME_IS_DIRTY; the locked write then silently
> overwrites the freshly-set dirty bit. The on-disk volume looks clean
> despite the recorded errors, so chkdsk will not run on the next mount
> and corrupted metadata can persist.
>
> Fix by moving the read-modify-write inside the mrec_lock: pass the bits
> to set and to clear separately, and combine them with the current flag
> state under the lock inside ntfs_write_volume_flags(). The set/clear
> helpers pass only the bits to modify, not the complete flag state. The
> bit manipulation is done on CPU-endian values, and the result is
> converted back to little-endian before storing it. The wrappers keep
> their signatures so callers are unchanged.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> - Also fix the ntfs_sync_fs() race by checking NVolErrors() and clearing
> VOLUME_IS_DIRTY under ni->mrec_lock.
> - Keep ntfs_set_volume_flags() and ntfs_clear_volume_flags() semantics
> unchanged.
> - Do not tie setting VOLUME_IS_DIRTY to NVolSetErrors() in the generic
> set helper.
> ---
> fs/ntfs/super.c | 62 +++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index a1813093222b..a7977b95b967 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -353,31 +353,45 @@ void ntfs_handle_error(struct super_block *sb)
> }
>
> /*
> - * ntfs_write_volume_flags - write new flags to the volume information flags
> + * ntfs_write_volume_flags - apply flag changes to the volume information flags
> * @vol: ntfs volume on which to modify the flags
> - * @flags: new flags value for the volume information flags
> + * @set_bits: bits to set in the volume information flags
> + * @clear_bits: bits to clear in the volume information flags
> *
> * Internal function. You probably want to use ntfs_{set,clear}_volume_flags()
> * instead (see below).
> *
> - * Replace the volume information flags on the volume @vol with the value
> - * supplied in @flags. Note, this overwrites the volume information flags, so
> - * make sure to combine the flags you want to modify with the old flags and use
> - * the result when calling ntfs_write_volume_flags().
> + * Combine @set_bits and @clear_bits with the current in-memory flag state and
> + * write the result back. The set/clear helpers pass only the bits to modify,
> + * not the complete flag state. The read-modify-write happens under
> + * ni->mrec_lock so that concurrent set/clear operations cannot lose updates.
> + * All bit manipulation is done on CPU-endian values, and the result is
> + * converted back to little-endian before storing it.
> *
> * Return 0 on success and -errno on error.
> */
> -static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
> +static int ntfs_write_volume_flags(struct ntfs_volume *vol,
> + const __le16 set_bits, const __le16 clear_bits,
> + const bool skip_if_errors)
> {
> struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
> struct volume_information *vi;
> struct ntfs_attr_search_ctx *ctx;
> + u16 flags;
> int err;
>
> - ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
> - le16_to_cpu(vol->vol_flags), le16_to_cpu(flags));
> mutex_lock(&ni->mrec_lock);
> - if (vol->vol_flags == flags)
> +
> + if (skip_if_errors && NVolErrors(vol))
> + goto done;
> +
> + flags = le16_to_cpu(vol->vol_flags);
> + flags |= le16_to_cpu(set_bits) & le16_to_cpu(VOLUME_FLAGS_MASK);
> + flags &= ~(le16_to_cpu(clear_bits) & le16_to_cpu(VOLUME_FLAGS_MASK));
> + ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
> + le16_to_cpu(vol->vol_flags), flags);
> +
> + if (le16_to_cpu(vol->vol_flags) == flags)
> goto done;
>
> ctx = ntfs_attr_get_search_ctx(ni, NULL);
> @@ -393,7 +407,7 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
>
> vi = (struct volume_information *)((u8 *)ctx->attr +
> le16_to_cpu(ctx->attr->data.resident.value_offset));
> - vol->vol_flags = vi->flags = flags;
> + vol->vol_flags = vi->flags = cpu_to_le16(flags);
> mark_mft_record_dirty(ctx->ntfs_ino);
> ntfs_attr_put_search_ctx(ctx);
> done:
> @@ -414,13 +428,14 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
> * @flags: flags to set on the volume
> *
> * Set the bits in @flags in the volume information flags on the volume @vol.
> + * The bits are combined with the current flag state under the lock in
> + * ntfs_write_volume_flags(), so concurrent updates are not lost.
> *
> * Return 0 on success and -errno on error.
> */
> int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
> {
> - flags &= VOLUME_FLAGS_MASK;
> - return ntfs_write_volume_flags(vol, vol->vol_flags | flags);
> + return ntfs_write_volume_flags(vol, flags, 0, false);
> }
>
> /*
> @@ -429,14 +444,27 @@ int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
> * @flags: flags to clear on the volume
> *
> * Clear the bits in @flags in the volume information flags on the volume @vol.
> + * The bits are combined with the current flag state under the lock in
> + * ntfs_write_volume_flags(), so concurrent updates are not lost.
> *
> * Return 0 on success and -errno on error.
> */
> int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
> {
> - flags &= VOLUME_FLAGS_MASK;
> - flags = vol->vol_flags & cpu_to_le16(~le16_to_cpu(flags));
> - return ntfs_write_volume_flags(vol, flags);
> + return ntfs_write_volume_flags(vol, 0, flags, false);
> +}
> +
> +/*
> + * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
> + * @vol: ntfs volume whose dirty bit should be cleared
> + *
> + * Check NVolErrors() and clear VOLUME_IS_DIRTY under the same mrec_lock so
> + * ntfs_sync_fs() cannot clear the dirty bit after a concurrent error has been
> + * recorded.
> + */
> +static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
> +{
> + return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
> }
>
> int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
> @@ -1862,7 +1890,7 @@ static int ntfs_sync_fs(struct super_block *sb, int wait)
> return 0;
>
> /* If there are some dirty buffers in the bdev inode */
> - if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY)) {
> + if (ntfs_clear_volume_dirty_if_no_errors(vol)) {
> ntfs_warning(sb, "Failed to clear dirty bit in volume information flags. Run chkdsk.");
> err = -EIO;
> }
Hi Hongling,
Thanks for the update.
Moving the read-modify-write of vol->vol_flags under the $Volume
inode's mrec_lock looks correct and prevents concurrent
ntfs_set_volume_flags() and ntfs_clear_volume_flags() operations from
overwriting each other's updates.
However, I think two races involving the clearing of VOLUME_IS_DIRTY
remain unresolved.
First, there is still no synchronization covering the complete
lifetime of a normal writer against ntfs_sync_fs(). The normal write
paths still contain code such as:
if (!(vol->vol_flags & VOLUME_IS_DIRTY))
ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
/* File data or metadata is modified afterwards. */
The following sequence therefore appears possible:
Writer ntfs_sync_fs()
------ --------------
Observes that dirty is already set
and skips ntfs_set_volume_flags()
Acquires $Volume mrec_lock
Finds no volume errors
Clears VOLUME_IS_DIRTY
Releases $Volume mrec_lock
Continues modifying file data
or metadata
Completes without setting dirty again
Even if the outer check were removed and ntfs_set_volume_flags() were
called unconditionally, this would only serialize the individual flag
update. It would not protect the complete interval between setting the
dirty bit and completing the metadata modification.
The helper introduced by patches 2 and 3 only handles metadata-error
paths. A normal successful writer does not call that helper, so those
patches do not address the race above.
Could you please clarify whether another synchronization mechanism
prevents ntfs_sync_fs() from clearing the dirty bit while a writer is
still in progress?
Second, the forced/emergency remount race mentioned in the previous
discussion also remains. A normal read-only remount goes through
sb_prepare_remount_readonly() and fails if there
are existing writers. The SB_FORCE path only calls
sb_start_ro_state_change(). This prevents new writers from acquiring
write access, but it neither checks nor waits for writers that
have already entered the filesystem.
The following sequence therefore still appears possible:
Writer Forced remount
------ --------------
Acquires write access and enters
the NTFS write path
sb_start_ro_state_change()
Prevents new writers
Remains in progress sync_filesystem()
Finds NVolErrors() clear
Clears VOLUME_IS_DIRTY
Continues modifying metadata,
or detects and records an error
only afterwards
Holding s_umount for write excludes concurrent sync and remount
operations, but it does not exclude an ordinary writer that has already
entered NTFS. Likewise, sync_filesystem() does not prove that such
writers have completed.
I think a forced remount should clear the dirty bit only after
confirming that all existing writers have exited while preventing new
writers from entering. If that cannot be confirmed, the forced remount
may still proceed, but it should preserve VOLUME_IS_DIRTY.
This is my analysis, and further discussion is welcome. If you agree
with it, I suggest adding a follow-up patch to address this race. Please
also correct me if I have overlooked or misunderstood anything.
Thanks,
Baolin.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit
2026-09-03 7:54 ` [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit Hongling Zeng
@ 2026-09-04 12:32 ` liubaolin
0 siblings, 0 replies; 7+ messages in thread
From: liubaolin @ 2026-09-04 12:32 UTC (permalink / raw)
To: Hongling Zeng, linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, stable
在 2026/9/3 15:54, Hongling Zeng 写道:
> The runtime metadata-corruption paths in fs/ntfs record the error flag
> with a bare NVolSetErrors() and separately rely on ntfs_set_volume_flags()
> or nothing at all to persist VOLUME_IS_DIRTY. Since the clear side
> (ntfs_sync_fs() via ntfs_clear_volume_dirty_if_no_errors()) evaluates
> NVolErrors() under ni->mrec_lock, a writer that records the error flag
> after the lock has been checked leaves a window in which the volume ends
> up persisted as clean despite the recorded error, so chkdsk will not run
> on the next mount and corrupted metadata can persist.
>
> Introduce ntfs_mark_volume_dirty_with_error(), which records the error
> flag before the locked dirty-bit write: the mutex acquire/release in
> ntfs_write_volume_flags() then provides the ordering, as any clear path
> running after us can only evaluate NVolErrors() under the lock once the
> flag is set.
>
> On a read-only mount or before the $Volume inode has been loaded, the
> helper only records the in-memory error state because the dirty bit
> cannot be persisted. On a writable volume it waits for the lock instead
> of silently dropping the dirty-bit update, and propagates any write
> error to the caller.
>
> Waiting is safe: the $Volume mrec_lock is a leaf lock. Its critical
> sections only look up and update the resident volume information
> attribute and never acquire the locks the callers hold (lcnbmp_lock,
> runlist locks), so no lock cycle exists. To make that explicit to the
> lock validator, give the $Volume inode its own mrec_lock class, the
> same way $MFTMirr already gets one; otherwise the lockdep class shared
> with regular file inodes would report an inversion for the
> lcnbmp_lock -> $Volume mrec_lock ordering that the helper introduces,
> even though no deadlock is possible.
>
> Mount and remount paths remain serialized by sb->s_umount and keep
> using NVolSetErrors() directly, since they do not race with
> ntfs_sync_fs() and may not be allowed to write the volume: before
> $Volume is loaded, with a read-only opened bdev, or on a hibernated
> volume, which we must not write to at all. The same applies to the
> ntfs_attr_lookup() failure paths and the mft record writeback paths,
> which hold a caller's mrec_lock and would self-deadlock the helper on
> the $Volume inode.
>
> Converting the runtime metadata-corruption call sites in mft.c,
> lcnalloc.c, bitmap.c, inode.c and attrib.c to use the helper is done
> in the next patch.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> fs/ntfs/ntfs.h | 1 +
> fs/ntfs/super.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ntfs/ntfs.h b/fs/ntfs/ntfs.h
> index df5a75d506f6..63d7900b6dba 100644
> --- a/fs/ntfs/ntfs.h
> +++ b/fs/ntfs/ntfs.h
> @@ -222,6 +222,7 @@ struct option_t {
> extern const struct option_t on_errors_arr[];
> int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags);
> int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags);
> +int ntfs_mark_volume_dirty_with_error(struct ntfs_volume *vol);
> int ntfs_write_volume_label(struct ntfs_volume *vol, char *label);
>
> /* From fs/ntfs/mst.c */
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index a7977b95b967..64f8d4469af3 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -454,6 +454,45 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
> return ntfs_write_volume_flags(vol, 0, flags, false);
> }
>
> +/*
> + * ntfs_mark_volume_dirty_with_error - record an error and mark volume dirty
> + * @vol: ntfs volume on which an error has been recorded
> + *
> + * To be called when runtime metadata corruption is detected so that chkdsk
> + * runs on the next mount. NVolErrors() is recorded first, so the in-memory
> + * error state is kept even when the dirty bit cannot be persisted; the
> + * mutex acquire/release in ntfs_write_volume_flags() then provides the
> + * ordering against ntfs_clear_volume_dirty_if_no_errors(), which checks
> + * the flag under the same lock.
> + *
> + * On a read-only mount or before the $Volume inode has been loaded, it
> + * only records the in-memory error state because the dirty bit cannot be
> + * persisted. On a writable volume it waits for the lock instead of
> + * silently dropping the dirty-bit update, and propagates any write error
> + * to the caller.
> + *
> + * Only for runtime error paths that can race sync and hold no mrec_lock.
> + * The $Volume mrec_lock is a leaf lock: its critical sections never
> + * acquire the locks those paths hold (lcnbmp_lock, runlist locks), so
> + * blocking here cannot deadlock. Do not call while holding the $Volume
> + * mrec_lock itself (ntfs_attr_lookup() failure and mft writeback paths),
> + * on mount/remount paths (serialized by sb->s_umount, keep using
> + * NVolSetErrors()), or on volumes we must not write to, such as
> + * hibernated ones.
> + *
> + * Return 0 on success and -errno on error.
> + */
> +int ntfs_mark_volume_dirty_with_error(struct ntfs_volume *vol)
> +{
> + NVolSetErrors(vol);
> +
> + /* Nothing to persist on a read-only or still-mounting volume. */
> + if (!vol->vol_ino || sb_rdonly(vol->sb))
> + return 0;
Hi Hongling,
It seems that vol->vol_ino != NULL alone may not be sufficient to
determine whether it is safe to update $Volume here.
In load_system_files(), both the $Volume inode and the root inode are
loaded before the hibernation status is checked:
vol->vol_ino = ntfs_iget(sb, FILE_Volume);
...
vol->root_ino = ntfs_iget(sb, FILE_root);
...
/*
* Check if Windows is suspended to disk on the target volume. If it
* is hibernated, we must not write *anything* to the disk so set
* NVolErrors() without setting the dirty volume flag and mount
* read-only. This will prevent read-write remounting and it will also
* prevent all writes.
*/
err = check_windows_hibernation_status(vol);
The existing comment above explicitly states that, when hibernation
is detected, only NVolErrors() should be set, without setting the
on-disk dirty volume flag or issuing any write to the volume. This is
necessary because the resumed system continues using filesystem state
saved in the hibernation image, which is based on the on-disk contents
at the time of hibernation. Any intervening write by Linux may make the
saved state inconsistent with the disk and lead to filesystem corruption.
With patch 3 applied, an error path in ntfs_read_locked_inode() may
call ntfs_mark_volume_dirty_with_error() while loading the root inode or
the hiberfil.sys inode. At that point, vol->vol_ino is already valid. On
a writable mount using errors=continue, sb_rdonly() is also false, so
the helper may write VOLUME_IS_DIRTY before the hibernation status has
been established.
Therefore, I think "$Volume has been loaded" and "volume flags may
be updated" need to be represented as separate states. The helper should
persist VOLUME_IS_DIRTY only after the hibernation check has completed
successfully and the mount has been confirmed writable. Before that,it
should only set NVolErrors().
Thanks,
Baolin.
> +
> + return ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +}
> +
> /*
> * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
> * @vol: ntfs volume whose dirty bit should be cleared
> @@ -918,7 +957,8 @@ static void ntfs_setup_allocators(struct ntfs_volume *vol)
> }
>
> static struct lock_class_key mftmirr_runlist_lock_key,
> - mftmirr_mrec_lock_key;
> + mftmirr_mrec_lock_key,
> + volume_mrec_lock_key;
> /*
> * load_and_init_mft_mirror - load and setup the mft mirror inode for a volume
> * @vol: ntfs super block describing device whose mft mirror to load
> @@ -1502,6 +1542,15 @@ static bool load_system_files(struct ntfs_volume *vol)
> ntfs_error(sb, "Failed to load $Volume.");
> goto iput_lcnbmp_err_out;
> }
> + /*
> + * Give the $Volume inode its own mrec_lock class: the error paths
> + * take it while holding locks (lcnbmp_lock, runlist locks) that
> + * nest inside mrec_lock elsewhere in the driver, and the lock
> + * validator would otherwise see that as an inversion even though
> + * the $Volume critical sections never acquire those locks.
> + */
> + lockdep_set_class(&NTFS_I(vol->vol_ino)->mrec_lock,
> + &volume_mrec_lock_key);
> m = map_mft_record(NTFS_I(vol->vol_ino));
> if (IS_ERR(m)) {
> iput_volume_failed:
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors
2026-09-03 7:54 ` [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors Hongling Zeng
@ 2026-09-04 12:59 ` liubaolin
2026-09-04 13:07 ` liubaolin
1 sibling, 0 replies; 7+ messages in thread
From: liubaolin @ 2026-09-04 12:59 UTC (permalink / raw)
To: Hongling Zeng, linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, stable
在 2026/9/3 15:54, Hongling Zeng 写道:
> Follow-up to the helper introduction: the runtime metadata-corruption
> paths in mft.c, lcnalloc.c, bitmap.c and inode.c still record the error
> flag with a bare NVolSetErrors(), leaving the same window in which
> ntfs_sync_fs() can acquire ni->mrec_lock, find NVolErrors() unset, and
> persist the volume as clean before the erroring thread records its
> error.
>
> Convert the call sites that can run concurrently with sync and hold no
> mrec_lock:
>
> - the mft record allocation and $MFT data/$BITMAP extend rollback
> paths (mft.c), which hold the runlist, mftbmp and lcnbmp locks;
>
> - the cluster allocation/free rollback paths (lcnalloc.c), which
> hold vol->lcnbmp_lock;
>
> - the bitmap rollback failure path (bitmap.c), which holds no
> relevant lock;
>
> - the inode read and writeback failure paths (inode.c), which run
> under I_NEW or with the mrec_lock already released at err_out.
>
> Blocking on the $Volume mrec_lock from those paths is safe: it is a
> leaf lock, its critical sections never acquire the locks the callers
> hold, and it carries its own lockdep class so the validator does not
> report an inversion against the shared mrec_lock class.
>
> The remaining call sites keep calling NVolSetErrors() directly, now
> with comments explaining why:
>
> - the mft record writeback paths (write_mft_record_nolock() and
> ntfs_write_mft_block()) can run with a caller's mrec_lock held,
> which for the $Volume inode itself would self-deadlock the helper;
>
> - the ntfs_attr_lookup() failure paths (ntfs_attr_find(),
> ntfs_external_attr_find() and the ntfs_attr_make_non_resident()
> rollback) run with the caller's mrec_lock held for the same
> reason.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> fs/ntfs/attrib.c | 18 ++++++++++++++-
> fs/ntfs/bitmap.c | 2 +-
> fs/ntfs/inode.c | 8 +++----
> fs/ntfs/lcnalloc.c | 4 ++--
> fs/ntfs/mft.c | 55 ++++++++++++++++++++++++++++------------------
> 5 files changed, 58 insertions(+), 29 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 60264833bb63..5965a67b5971 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -964,6 +964,11 @@ static int ntfs_attr_find(const __le32 type, const __le16 *name,
> }
> ntfs_error(vol->sb, "mft %#llx, type %#x is corrupt. Run chkdsk.",
> (long long)ctx->ntfs_ino->mft_no, le32_to_cpu(type));
> + /*
> + * ntfs_attr_lookup() runs with the caller's mrec_lock held, so the
> + * dirty bit cannot be written here (it would self-deadlock for the
> + * $Volume inode); record the error flag only.
> + */
> NVolSetErrors(vol);
> return -EIO;
> }
> @@ -1501,8 +1506,14 @@ static int ntfs_external_attr_find(const __le32 type,
> err = -EIO;
> }
>
> - if (err != -ENOMEM)
> + if (err != -ENOMEM) {
> + /*
> + * ntfs_attr_lookup() runs with the caller's mrec_lock
> + * held, so the dirty bit cannot be written here; record
> + * the error flag only.
> + */
> NVolSetErrors(vol);
> + }
> return err;
> not_found:
> /*
> @@ -2236,6 +2247,11 @@ int ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size)
> if (ntfs_cluster_free_from_rl(vol, rl) < 0) {
> ntfs_error(vol->sb,
> "Failed to release allocated cluster(s) in error code path. Run chkdsk to recover the lost cluster(s).");
> + /*
> + * The caller may hold the mrec_lock of the inode
> + * being modified, so the dirty bit cannot be
> + * written here; record the error flag only.
> + */
> NVolSetErrors(vol);
> }
> kvfree(rl);
> diff --git a/fs/ntfs/bitmap.c b/fs/ntfs/bitmap.c
> index b1436b3151b9..6120cb301665 100644
> --- a/fs/ntfs/bitmap.c
> +++ b/fs/ntfs/bitmap.c
> @@ -284,7 +284,7 @@ int __ntfs_bitmap_set_bits_in_run(struct inode *vi, const s64 start_bit,
> ntfs_error(vi->i_sb,
> "Failed to map subsequent page (error %i) and rollback failed (error %i). Aborting and leaving inconsistent metadata. Unmount and run chkdsk.",
> err, pos);
> - NVolSetErrors(NTFS_SB(vi->i_sb));
> + ntfs_mark_volume_dirty_with_error(NTFS_SB(vi->i_sb));
Hi Hongling,
There may still be a case where the dirty bit is not persisted. This
call site invokes ntfs_error() before
ntfs_mark_volume_dirty_with_error(). When errors=remount-ro is in
use, ntfs_error() sets SB_RDONLY through the following path:
ntfs_error()
-> ntfs_handle_error()
-> sb->s_flags |= SB_RDONLY
Therefore, when ntfs_mark_volume_dirty_with_error() is called,
NVolErrors() is set first, but sb_rdonly() is already true. The helper
returns without writing VOLUME_IS_DIRTY to disk.
For example, the following sequence still appears possible:
ntfs_sync_fs() error path
-------------- ----------
acquire $Volume mrec_lock
observe NVolErrors() == false
ntfs_error()
-> set SB_RDONLY
set NVolErrors()
sb_rdonly() == true
skip the dirty-bit update
clear VOLUME_IS_DIRTY
release $Volume mrec_lock
This leaves NVolErrors() set while the on-disk VOLUME_IS_DIRTY bit
has been cleared.
The same issue should apply to the other call sites in this patch
that invoke ntfs_error() before ntfs_mark_volume_dirty_with_error().
Should the dirty bit be persisted before setting SB_RDONLY?
Thanks,
Baolin.
> }
> return err;
> }
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 32edb4045178..0288f6a101dc 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1245,7 +1245,7 @@ static int ntfs_read_locked_inode(struct inode *vi)
> ntfs_error(vol->sb,
> "Failed with error code %i. Marking corrupt inode 0x%llx as bad. Run chkdsk.",
> err, ni->mft_no);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> return err;
> }
> @@ -1473,7 +1473,7 @@ static int ntfs_read_locked_attr_inode(struct inode *base_vi, struct inode *vi)
> err, ni->mft_no, ni->type, ni->name_len,
> base_ni->mft_no);
> if (err != -ENOENT && err != -ENOMEM)
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return err;
> }
>
> @@ -1725,7 +1725,7 @@ static int ntfs_read_locked_index_inode(struct inode *base_vi, struct inode *vi)
> "Failed with error code %i while reading index inode (mft_no 0x%llx, name_len %i.",
> err, ni->mft_no, ni->name_len);
> if (err != -EOPNOTSUPP && err != -ENOMEM)
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return err;
> }
>
> @@ -2883,7 +2883,7 @@ int __ntfs_write_inode(struct inode *vi, int sync)
> mark_inode_dirty(vi);
> else {
> ntfs_error(vi->i_sb, "Failed (error %i): Run chkdsk.", -err);
> - NVolSetErrors(ni->vol);
> + ntfs_mark_volume_dirty_with_error(ni->vol);
> }
> if (need_iput)
> iput(vi);
> diff --git a/fs/ntfs/lcnalloc.c b/fs/ntfs/lcnalloc.c
> index aa2e017a4384..dd6854f16136 100644
> --- a/fs/ntfs/lcnalloc.c
> +++ b/fs/ntfs/lcnalloc.c
> @@ -763,7 +763,7 @@ switch_to_data1_zone: search_zone = 2;
> ntfs_error(vol->sb,
> "Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
> err2);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> /* Free the runlist. */
> kvfree(rl);
> @@ -1044,7 +1044,7 @@ s64 __ntfs_cluster_free(struct ntfs_inode *ni, const s64 start_vcn, s64 count,
> ntfs_error(vol->sb,
> "Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
> (int)delta);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> ntfs_dec_free_clusters(vol, delta);
> up_write(&vol->lcnbmp_lock);
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 984a0827f9ac..d8791d7b9678 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -657,8 +657,15 @@ int write_mft_record_nolock(struct ntfs_inode *ni, struct mft_record *m, int syn
> "Not enough memory to write mft record. Redirtying so the write is retried later.");
> mark_mft_record_dirty(ni);
> err = 0;
> - } else
> + } else {
> + /*
> + * The writeback path can run with the caller's mrec_lock
> + * held, so the dirty bit cannot be written here (it would
> + * self-deadlock for the $Volume inode); record the error
> + * flag only.
> + */
> NVolSetErrors(vol);
> + }
> return err;
> }
>
> @@ -1150,7 +1157,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> if (ntfs_cluster_free_from_rl(vol, rl2)) {
> ntfs_error(vol->sb, "Failed to deallocate allocated cluster.%s",
> es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> kvfree(rl2);
> return PTR_ERR(rl);
> @@ -1285,7 +1292,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> * The only thing that is now wrong is ->allocated_size of the
> * base attribute extent which chkdsk should be able to fix.
> */
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return ret;
> }
> a = ctx->attr;
> @@ -1306,7 +1313,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> down_write(&vol->lcnbmp_lock);
> if (ntfs_bitmap_clear_bit(vol->lcnbmp_ino, lcn)) {
> ntfs_error(vol->sb, "Failed to free allocated cluster.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> } else
> ntfs_inc_free_clusters(vol, 1);
> up_write(&vol->lcnbmp_lock);
> @@ -1317,16 +1324,16 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> a->data.non_resident.mapping_pairs_offset),
> rl2, ll, -1, NULL, NULL, NULL)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> mark_mft_record_dirty(ctx->ntfs_ino);
> } else if (status.mp_extended && ntfs_attr_update_mapping_pairs(mftbmp_ni, 0)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ctx)
> ntfs_attr_put_search_ctx(ctx);
> @@ -1420,20 +1427,20 @@ static int ntfs_mft_bitmap_extend_initialized_nolock(struct ntfs_volume *vol)
> mrec = map_mft_record(mft_ni);
> if (IS_ERR(mrec)) {
> ntfs_error(vol->sb, "Failed to map mft record.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return ret;
> }
> ctx = ntfs_attr_get_search_ctx(mft_ni, mrec);
> if (unlikely(!ctx)) {
> ntfs_error(vol->sb, "Failed to get search context.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> goto unm_err_out;
> }
> if (ntfs_attr_lookup(mftbmp_ni->type, mftbmp_ni->name,
> mftbmp_ni->name_len, CASE_SENSITIVE, 0, NULL, 0, ctx)) {
> ntfs_error(vol->sb,
> "Failed to find first attribute extent of mft bitmap attribute.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> put_err_out:
> ntfs_attr_put_search_ctx(ctx);
> unm_err_out:
> @@ -1587,7 +1594,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> if (ntfs_cluster_free_from_rl(vol, rl2)) {
> ntfs_error(vol->sb,
> "Failed to deallocate clusters from the mft data attribute.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> kvfree(rl2);
> return PTR_ERR(rl);
> @@ -1721,7 +1728,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> * The only thing that is now wrong is ->allocated_size of the
> * base attribute extent which chkdsk should be able to fix.
> */
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return ret;
> }
> ctx->attr->data.non_resident.highest_vcn =
> @@ -1729,17 +1736,17 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> undo_alloc:
> if (ntfs_cluster_free(mft_ni, old_last_vcn, -1, ctx) < 0) {
> ntfs_error(vol->sb, "Failed to free clusters from mft data attribute.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
>
> if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
> ntfs_error(vol->sb, "Failed to truncate mft data attribute runlist.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (mp_extended && ntfs_attr_update_mapping_pairs(mft_ni, 0)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs.%s",
> es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ctx) {
> a = ctx->attr;
> @@ -1750,16 +1757,16 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> a->data.non_resident.mapping_pairs_offset),
> rl2, ll, -1, NULL, NULL, NULL)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> mark_mft_record_dirty(ctx->ntfs_ino);
> } else if (IS_ERR(ctx->mrec)) {
> ntfs_error(vol->sb, "Failed to restore attribute search context.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> ntfs_attr_put_search_ctx(ctx);
> }
> @@ -2322,7 +2329,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
> folio_unlock(folio);
> kunmap_local(m);
> folio_put(folio);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> goto search_free_rec;
> }
> /*
> @@ -2475,7 +2482,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
> undo_mftbmp_alloc_nolock:
> if (ntfs_bitmap_clear_bit(vol->mftbmp_ino, bit)) {
> ntfs_error(vol->sb, "Failed to clear bit in mft bitmap.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (!base_ni || base_ni->mft_no != FILE_MFT)
> up_write(&vol->mftbmp_lock);
> @@ -2815,8 +2822,14 @@ static int ntfs_write_mft_block(struct folio *folio, struct writeback_control *w
> iput(ref_inos[nr_ref_inos]);
> }
>
> - if (unlikely(err && err != -ENOMEM))
> + if (unlikely(err && err != -ENOMEM)) {
> + /*
> + * The writeback path can run with a caller's mrec_lock
> + * held, so the dirty bit cannot be written here; record
> + * the error flag only.
> + */
> NVolSetErrors(vol);
> + }
> if (likely(!err))
> ntfs_debug("Done.");
> return err;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors
2026-09-03 7:54 ` [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors Hongling Zeng
2026-09-04 12:59 ` liubaolin
@ 2026-09-04 13:07 ` liubaolin
1 sibling, 0 replies; 7+ messages in thread
From: liubaolin @ 2026-09-04 13:07 UTC (permalink / raw)
To: Hongling Zeng, linkinjeon, hyc.lee
Cc: ntfs, linux-kernel, zhongling0719, stable
在 2026/9/3 15:54, Hongling Zeng 写道:
> Follow-up to the helper introduction: the runtime metadata-corruption
> paths in mft.c, lcnalloc.c, bitmap.c and inode.c still record the error
> flag with a bare NVolSetErrors(), leaving the same window in which
> ntfs_sync_fs() can acquire ni->mrec_lock, find NVolErrors() unset, and
> persist the volume as clean before the erroring thread records its
> error.
>
> Convert the call sites that can run concurrently with sync and hold no
> mrec_lock:
>
> - the mft record allocation and $MFT data/$BITMAP extend rollback
> paths (mft.c), which hold the runlist, mftbmp and lcnbmp locks;
>
> - the cluster allocation/free rollback paths (lcnalloc.c), which
> hold vol->lcnbmp_lock;
>
> - the bitmap rollback failure path (bitmap.c), which holds no
> relevant lock;
>
> - the inode read and writeback failure paths (inode.c), which run
> under I_NEW or with the mrec_lock already released at err_out.
>
> Blocking on the $Volume mrec_lock from those paths is safe: it is a
> leaf lock, its critical sections never acquire the locks the callers
> hold, and it carries its own lockdep class so the validator does not
> report an inversion against the shared mrec_lock class.
>
> The remaining call sites keep calling NVolSetErrors() directly, now
> with comments explaining why:
>
> - the mft record writeback paths (write_mft_record_nolock() and
> ntfs_write_mft_block()) can run with a caller's mrec_lock held,
> which for the $Volume inode itself would self-deadlock the helper;
>
> - the ntfs_attr_lookup() failure paths (ntfs_attr_find(),
> ntfs_external_attr_find() and the ntfs_attr_make_non_resident()
> rollback) run with the caller's mrec_lock held for the same
> reason.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> fs/ntfs/attrib.c | 18 ++++++++++++++-
> fs/ntfs/bitmap.c | 2 +-
> fs/ntfs/inode.c | 8 +++----
> fs/ntfs/lcnalloc.c | 4 ++--
> fs/ntfs/mft.c | 55 ++++++++++++++++++++++++++++------------------
> 5 files changed, 58 insertions(+), 29 deletions(-)
>
> diff --git a/fs/ntfs/attrib.c b/fs/ntfs/attrib.c
> index 60264833bb63..5965a67b5971 100644
> --- a/fs/ntfs/attrib.c
> +++ b/fs/ntfs/attrib.c
> @@ -964,6 +964,11 @@ static int ntfs_attr_find(const __le32 type, const __le16 *name,
> }
> ntfs_error(vol->sb, "mft %#llx, type %#x is corrupt. Run chkdsk.",
> (long long)ctx->ntfs_ino->mft_no, le32_to_cpu(type));
> + /*
> + * ntfs_attr_lookup() runs with the caller's mrec_lock held, so the
> + * dirty bit cannot be written here (it would self-deadlock for the
> + * $Volume inode); record the error flag only.
> + */
> NVolSetErrors(vol);
Hi Hongling,
I understand why ntfs_mark_volume_dirty_with_error() cannot be
called directly here: if the caller is holding the $Volume inode's
mrec_lock, the helper would deadlock trying to acquire the same lock again.
However, keeping the bare NVolSetErrors() does not seem to resolve
the dirty-bit issue. If the caller is holding a regular inode's
mrec_lock, it is different from $Volume->mrec_lock, so the following
sequence is still possible:
ntfs_sync_fs() error path
-------------- ----------
acquire $Volume->mrec_lock hold a regular inode's mrec_lock
observe NVolErrors() == false
detect metadata corruption
set NVolErrors()
do not set VOLUME_IS_DIRTY
clear VOLUME_IS_DIRTY
release $Volume->mrec_lock
If the caller is holding $Volume->mrec_lock itself, the sequence
above cannot occur concurrently, but the operations may still be ordered
as follows:
ntfs_sync_fs() $Volume error path
-------------- ------------------
acquire $Volume->mrec_lock
observe NVolErrors() == false
clear VOLUME_IS_DIRTY
release $Volume->mrec_lock
acquire $Volume->mrec_lock
detect metadata corruption
set NVolErrors()
do not set VOLUME_IS_DIRTY
release $Volume->mrec_lock
In either case, only the in-memory NVolErrors() flag may be set while
the on-disk VOLUME_IS_DIRTY bit remains clear.
The other runtime error paths in this patch that retain a bare
NVolSetErrors() because they may hold an mrec_lock appear to have the
same issue. The new comments explain why the helper cannot be called
directly, but these paths still do not set the on-disk dirty bit again.
Thanks,
Baolin.
> return -EIO;
> }
> @@ -1501,8 +1506,14 @@ static int ntfs_external_attr_find(const __le32 type,
> err = -EIO;
> }
>
> - if (err != -ENOMEM)
> + if (err != -ENOMEM) {
> + /*
> + * ntfs_attr_lookup() runs with the caller's mrec_lock
> + * held, so the dirty bit cannot be written here; record
> + * the error flag only.
> + */
> NVolSetErrors(vol);
> + }
> return err;
> not_found:
> /*
> @@ -2236,6 +2247,11 @@ int ntfs_attr_make_non_resident(struct ntfs_inode *ni, const u32 data_size)
> if (ntfs_cluster_free_from_rl(vol, rl) < 0) {
> ntfs_error(vol->sb,
> "Failed to release allocated cluster(s) in error code path. Run chkdsk to recover the lost cluster(s).");
> + /*
> + * The caller may hold the mrec_lock of the inode
> + * being modified, so the dirty bit cannot be
> + * written here; record the error flag only.
> + */
> NVolSetErrors(vol);
> }
> kvfree(rl);
> diff --git a/fs/ntfs/bitmap.c b/fs/ntfs/bitmap.c
> index b1436b3151b9..6120cb301665 100644
> --- a/fs/ntfs/bitmap.c
> +++ b/fs/ntfs/bitmap.c
> @@ -284,7 +284,7 @@ int __ntfs_bitmap_set_bits_in_run(struct inode *vi, const s64 start_bit,
> ntfs_error(vi->i_sb,
> "Failed to map subsequent page (error %i) and rollback failed (error %i). Aborting and leaving inconsistent metadata. Unmount and run chkdsk.",
> err, pos);
> - NVolSetErrors(NTFS_SB(vi->i_sb));
> + ntfs_mark_volume_dirty_with_error(NTFS_SB(vi->i_sb));
> }
> return err;
> }
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 32edb4045178..0288f6a101dc 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -1245,7 +1245,7 @@ static int ntfs_read_locked_inode(struct inode *vi)
> ntfs_error(vol->sb,
> "Failed with error code %i. Marking corrupt inode 0x%llx as bad. Run chkdsk.",
> err, ni->mft_no);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> return err;
> }
> @@ -1473,7 +1473,7 @@ static int ntfs_read_locked_attr_inode(struct inode *base_vi, struct inode *vi)
> err, ni->mft_no, ni->type, ni->name_len,
> base_ni->mft_no);
> if (err != -ENOENT && err != -ENOMEM)
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return err;
> }
>
> @@ -1725,7 +1725,7 @@ static int ntfs_read_locked_index_inode(struct inode *base_vi, struct inode *vi)
> "Failed with error code %i while reading index inode (mft_no 0x%llx, name_len %i.",
> err, ni->mft_no, ni->name_len);
> if (err != -EOPNOTSUPP && err != -ENOMEM)
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return err;
> }
>
> @@ -2883,7 +2883,7 @@ int __ntfs_write_inode(struct inode *vi, int sync)
> mark_inode_dirty(vi);
> else {
> ntfs_error(vi->i_sb, "Failed (error %i): Run chkdsk.", -err);
> - NVolSetErrors(ni->vol);
> + ntfs_mark_volume_dirty_with_error(ni->vol);
> }
> if (need_iput)
> iput(vi);
> diff --git a/fs/ntfs/lcnalloc.c b/fs/ntfs/lcnalloc.c
> index aa2e017a4384..dd6854f16136 100644
> --- a/fs/ntfs/lcnalloc.c
> +++ b/fs/ntfs/lcnalloc.c
> @@ -763,7 +763,7 @@ switch_to_data1_zone: search_zone = 2;
> ntfs_error(vol->sb,
> "Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
> err2);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> /* Free the runlist. */
> kvfree(rl);
> @@ -1044,7 +1044,7 @@ s64 __ntfs_cluster_free(struct ntfs_inode *ni, const s64 start_vcn, s64 count,
> ntfs_error(vol->sb,
> "Failed to rollback (error %i). Leaving inconsistent metadata! Unmount and run chkdsk.",
> (int)delta);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> ntfs_dec_free_clusters(vol, delta);
> up_write(&vol->lcnbmp_lock);
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 984a0827f9ac..d8791d7b9678 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -657,8 +657,15 @@ int write_mft_record_nolock(struct ntfs_inode *ni, struct mft_record *m, int syn
> "Not enough memory to write mft record. Redirtying so the write is retried later.");
> mark_mft_record_dirty(ni);
> err = 0;
> - } else
> + } else {
> + /*
> + * The writeback path can run with the caller's mrec_lock
> + * held, so the dirty bit cannot be written here (it would
> + * self-deadlock for the $Volume inode); record the error
> + * flag only.
> + */
> NVolSetErrors(vol);
> + }
> return err;
> }
>
> @@ -1150,7 +1157,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> if (ntfs_cluster_free_from_rl(vol, rl2)) {
> ntfs_error(vol->sb, "Failed to deallocate allocated cluster.%s",
> es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> kvfree(rl2);
> return PTR_ERR(rl);
> @@ -1285,7 +1292,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> * The only thing that is now wrong is ->allocated_size of the
> * base attribute extent which chkdsk should be able to fix.
> */
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return ret;
> }
> a = ctx->attr;
> @@ -1306,7 +1313,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> down_write(&vol->lcnbmp_lock);
> if (ntfs_bitmap_clear_bit(vol->lcnbmp_ino, lcn)) {
> ntfs_error(vol->sb, "Failed to free allocated cluster.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> } else
> ntfs_inc_free_clusters(vol, 1);
> up_write(&vol->lcnbmp_lock);
> @@ -1317,16 +1324,16 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(struct ntfs_volume *vol)
> a->data.non_resident.mapping_pairs_offset),
> rl2, ll, -1, NULL, NULL, NULL)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> mark_mft_record_dirty(ctx->ntfs_ino);
> } else if (status.mp_extended && ntfs_attr_update_mapping_pairs(mftbmp_ni, 0)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ctx)
> ntfs_attr_put_search_ctx(ctx);
> @@ -1420,20 +1427,20 @@ static int ntfs_mft_bitmap_extend_initialized_nolock(struct ntfs_volume *vol)
> mrec = map_mft_record(mft_ni);
> if (IS_ERR(mrec)) {
> ntfs_error(vol->sb, "Failed to map mft record.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return ret;
> }
> ctx = ntfs_attr_get_search_ctx(mft_ni, mrec);
> if (unlikely(!ctx)) {
> ntfs_error(vol->sb, "Failed to get search context.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> goto unm_err_out;
> }
> if (ntfs_attr_lookup(mftbmp_ni->type, mftbmp_ni->name,
> mftbmp_ni->name_len, CASE_SENSITIVE, 0, NULL, 0, ctx)) {
> ntfs_error(vol->sb,
> "Failed to find first attribute extent of mft bitmap attribute.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> put_err_out:
> ntfs_attr_put_search_ctx(ctx);
> unm_err_out:
> @@ -1587,7 +1594,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> if (ntfs_cluster_free_from_rl(vol, rl2)) {
> ntfs_error(vol->sb,
> "Failed to deallocate clusters from the mft data attribute.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> kvfree(rl2);
> return PTR_ERR(rl);
> @@ -1721,7 +1728,7 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> * The only thing that is now wrong is ->allocated_size of the
> * base attribute extent which chkdsk should be able to fix.
> */
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> return ret;
> }
> ctx->attr->data.non_resident.highest_vcn =
> @@ -1729,17 +1736,17 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> undo_alloc:
> if (ntfs_cluster_free(mft_ni, old_last_vcn, -1, ctx) < 0) {
> ntfs_error(vol->sb, "Failed to free clusters from mft data attribute.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
>
> if (ntfs_rl_truncate_nolock(vol, &mft_ni->runlist, old_last_vcn)) {
> ntfs_error(vol->sb, "Failed to truncate mft data attribute runlist.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (mp_extended && ntfs_attr_update_mapping_pairs(mft_ni, 0)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs.%s",
> es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ctx) {
> a = ctx->attr;
> @@ -1750,16 +1757,16 @@ static int ntfs_mft_data_extend_allocation_nolock(struct ntfs_volume *vol)
> a->data.non_resident.mapping_pairs_offset),
> rl2, ll, -1, NULL, NULL, NULL)) {
> ntfs_error(vol->sb, "Failed to restore mapping pairs array.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (ntfs_attr_record_resize(ctx->mrec, a, old_alen)) {
> ntfs_error(vol->sb, "Failed to restore attribute record.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> mark_mft_record_dirty(ctx->ntfs_ino);
> } else if (IS_ERR(ctx->mrec)) {
> ntfs_error(vol->sb, "Failed to restore attribute search context.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> ntfs_attr_put_search_ctx(ctx);
> }
> @@ -2322,7 +2329,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
> folio_unlock(folio);
> kunmap_local(m);
> folio_put(folio);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> goto search_free_rec;
> }
> /*
> @@ -2475,7 +2482,7 @@ int ntfs_mft_record_alloc(struct ntfs_volume *vol, const int mode,
> undo_mftbmp_alloc_nolock:
> if (ntfs_bitmap_clear_bit(vol->mftbmp_ino, bit)) {
> ntfs_error(vol->sb, "Failed to clear bit in mft bitmap.%s", es);
> - NVolSetErrors(vol);
> + ntfs_mark_volume_dirty_with_error(vol);
> }
> if (!base_ni || base_ni->mft_no != FILE_MFT)
> up_write(&vol->mftbmp_lock);
> @@ -2815,8 +2822,14 @@ static int ntfs_write_mft_block(struct folio *folio, struct writeback_control *w
> iput(ref_inos[nr_ref_inos]);
> }
>
> - if (unlikely(err && err != -ENOMEM))
> + if (unlikely(err && err != -ENOMEM)) {
> + /*
> + * The writeback path can run with a caller's mrec_lock
> + * held, so the dirty bit cannot be written here; record
> + * the error flag only.
> + */
> NVolSetErrors(vol);
> + }
> if (likely(!err))
> ntfs_debug("Done.");
> return err;
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-04 13:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-03 7:54 [PATCH v2 1/3] ntfs: fix volume flag update races Hongling Zeng
2026-09-03 7:54 ` [PATCH v2 2/3] ntfs: add helper to record error flag before setting volume dirty bit Hongling Zeng
2026-09-04 12:32 ` liubaolin
2026-09-03 7:54 ` [PATCH v2 3/3] ntfs: convert runtime error paths to mark volume dirty after recording errors Hongling Zeng
2026-09-04 12:59 ` liubaolin
2026-09-04 13:07 ` liubaolin
2026-09-04 12:06 ` [PATCH v2 1/3] ntfs: fix volume flag update races liubaolin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®