Re: [PATCH v2 1/3] ntfs: fix volume flag update races
From: liubaolin
Date: Fri Sep 04 2026 - 08:14:12 EST
在 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@xxxxxxxxxxxxxxx
Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
---
- 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.