[PATCH v3 0/3] ntfs: fix volume flag update races and persist the recorded error state

[PATCH v3 0/3] ntfs: fix volume flag update races and persist the recorded error state

[Hongling Zeng]

Hi all,

First of all, my apologies for the many versions of this patch.  I
should have folded the reviewer feedback in and resent a single coherent
series sooner; the split across v1/v2 and follow-ups made the thread
harder to follow than it needed to be.  This v3 consolidates everything
into one series of three patches.

To recap the review discussion:

The original patch (now 1/3) fixed the lost-update race where
ntfs_set_volume_flags() and ntfs_clear_volume_flags() computed the new
value from vol->vol_flags outside any lock.  The review correctly
pointed out that a race remained in ntfs_sync_fs(): it checked
NVolErrors() outside the critical section, so a concurrent error path
could still record an error after the check and before the clear, and
the volume would end up persisted as clean despite the recorded error,
meaning chkdsk would not run on the next mount.

That race is now fixed in 1/3 itself: the NVolErrors() check and the
clearing of VOLUME_IS_DIRTY both happen inside the same mrec_lock
critical section, via a dedicated ntfs_clear_volume_dirty_if_no_errors()
helper.  With that, every interleaving of the sync thread and the error
path leaves the volume dirty on disk.

Patches 2/3 then close the writer-side half of the window: the runtime
metadata-corruption paths in fs/ntfs recorded only the in-memory
NVolErrors() flag and never persisted VOLUME_IS_DIRTY at all, so a
volume could unmount with a clean on-disk flag despite recorded
corruption.  Persisting from the error paths themselves is not an
option: they run under a wide variety of ntfs locks (runlist locks,
vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks) while the dirty-bit
write needs the $Volume mrec_lock and can take the $MFT runlist lock,
which self-deadlocks or forms ABBA cycles.  Instead, 2/3 makes
persistence a property of the lock-free sync contexts: the new
ntfs_sync_volume_dirty_state() sets VOLUME_IS_DIRTY when NVolErrors()
is recorded and clears it otherwise, evaluating the error flag under
the $Volume mrec_lock.  It is called from ntfs_sync_fs(), the
remount-to-read-only path and ntfs_put_super(), with NV_Hibernated
gating so a hibernated volume is never written by these paths.  The
guarantee is eventual rather than instantaneous (a crash between the
error record and the next persistence point remains a window);
ntfs_put_super() runs on a quiesced filesystem after evict_inodes(),
so a volume that is read-write at unmount time cannot unmount clean.

Patch 3/3 is pure hygiene found during review: the load_system_files()
error unwind leaves a stale vol->vol_ino pointer in place, and carries
a dead iput() inside the IS_ERR(vol->vol_ino) branch; NULL the pointer
like ntfs_put_super() and the ntfs_fill_super() error path already do.

One housekeeping note: this series was rebased onto the current ntfs
tree after the first posting failed to apply there.  Dennis Tighe's
"ntfs: do not mark the volume clean in sync_fs when errors were
recorded" landed in the meantime and fixed part of the same sync_fs
problem this series addresses; patch 2/3 supersedes that fix with the
ntfs_sync_volume_dirty_state() helper, which guards the clear in every
persistence context, not just sync_fs.  No conflict with its intent.

Patch layout:

  1/3 ntfs: fix volume flag update races
      Move the read-modify-write of vol->vol_flags inside the
      $Volume mrec_lock; ntfs_sync_fs() clears the dirty bit under
      the lock with the NVolErrors() check.

  2/3 ntfs: sync the volume dirty bit with the recorded error state
      New ntfs_sync_volume_dirty_state(), called from the lock-free
      sync contexts (sync_fs / remount-ro / put_super); NV_Hibernated
      gating.

  3/3 ntfs: NULL vol->vol_ino in the load_system_files() error
      teardown
      Drop the stale pointer and the dead iput().

The series builds cleanly with W=1.

Thanks,
Hongling

[PATCH v3 1/3] ntfs: fix volume flag update races

[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>
---
 fs/ntfs/super.c | 63 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 18 deletions(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 60d43339c590..ce159ae8169a 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,8 +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 (!NVolErrors(vol) &&
-	    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

[PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[Hongling Zeng]

The runtime metadata-corruption paths in fs/ntfs only record the
in-memory NVolErrors() flag; whether VOLUME_IS_DIRTY ever reaches disk
depends on ntfs_set_volume_flags() being called by some other path,
which for most error sites never happens.  A volume can therefore
unmount with a clean on-disk flag despite recorded corruption, and
chkdsk will not run on the next mount.

Persisting the dirty bit from the error paths themselves does not work:
they run under a wide variety of ntfs locks, and the dirty-bit write
takes the $Volume mrec_lock and maps the $Volume mft record, which on
an $MFT page-cache miss takes the $MFT runlist lock for writing.  That
is enough to self-deadlock or form ABBA cycles from several of them:
the $MFT extend undo paths hold the $MFT runlist lock and then take
vol->lcnbmp_lock inside ntfs_cluster_free(); the cluster allocation and
free rollback paths hold vol->lcnbmp_lock; and the whole mft record
allocation tree is reachable from ntfs_write_volume_label()'s
attribute-list maintenance while it holds the $Volume mrec_lock itself.

Instead, make the persistence a property of the sync paths, which run
without ntfs locks held.  The new ntfs_sync_volume_dirty_state() sets
VOLUME_IS_DIRTY when NVolErrors() is recorded and clears it otherwise,
evaluating the error flag under the $Volume mrec_lock.  It is called
from ntfs_sync_fs(), from the remount-to-read-only path of
ntfs_reconfigure(), and from ntfs_put_super(), which previously
evaluated NVolErrors() outside the lock before clearing the dirty bit
unconditionally, and which now also persists the dirty bit for volumes
with recorded errors so they unmount with chkdsk scheduled.

The guarantee this provides is eventual, not instantaneous: the error
paths record NVolErrors() with a lock-free set_bit(), so a persistence
point that evaluates the flag just before an error is recorded can
still leave the on-disk bit clean until the next one.  This is sound
because NVolErrors() is sticky for the lifetime of the mount and every
persistence point re-derives the on-disk bit from it; the last one,
ntfs_put_super(), runs after evict_inodes() on a quiesced filesystem,
so a volume that is read-write at unmount time cannot unmount clean.
A volume that is already read-only when the error is recorded
(errors=remount-ro flips the superblock on the first error, as does an
earlier remount-ro) has no persistence point left and keeps whatever
on-disk bit it had; that behaviour is unchanged.  The residual window
is a crash between the error and the next persistence point.

The persistence paths never write a hibernated volume: resuming Windows
from a modified image corrupts it.  Record the mount-time hibernation
verdict in the new NV_Hibernated volume flag and make
ntfs_sync_volume_dirty_state() a no-op while it is set, so the dirty
bit is left exactly as it is on disk and only the in-memory error
state is kept.  Without this, an rw mount of a hibernated volume with
the default errors=continue would gain a filesystem-internal write on
the first sync, remount or unmount.  Other writes to such a mount,
like the mount-time logfile emptying, are pre-existing and unchanged.

Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 fs/ntfs/super.c  | 113 ++++++++++++++++++++++++++++++++++++-----------
 fs/ntfs/volume.h |   4 ++
 2 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index ce159ae8169a..4b4af1ea8b09 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -262,6 +262,8 @@ static int ntfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	return 0;
 }
 
+static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol);
+
 static int ntfs_reconfigure(struct fs_context *fc)
 {
 	struct super_block *sb = fc->root->d_sb;
@@ -312,11 +314,19 @@ static int ntfs_reconfigure(struct fs_context *fc)
 		}
 	} else if (!sb_rdonly(sb) && (fc->sb_flags & SB_RDONLY)) {
 		/* Remounting read-only. */
-		if (!NVolErrors(vol)) {
-			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
-				ntfs_warning(sb,
-					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
-		}
+		/*
+		 * With errors recorded the dirty bit is set rather than
+		 * cleared, so it survives until the unmount.  Note that
+		 * once this remount succeeds no further persistence point
+		 * exists: ntfs_sync_fs() is only ever invoked for
+		 * read-write superblocks (all its VFS callers skip
+		 * read-only ones) and ntfs_put_super() skips them, so an
+		 * error recorded only after the remount is never
+		 * persisted.
+		 */
+		if (ntfs_sync_volume_dirty_state(vol))
+			ntfs_warning(sb,
+				"Failed to update dirty bit in volume information flags.  Run chkdsk.");
 	}
 
 	ntfs_debug("Done.");
@@ -357,9 +367,10 @@ void ntfs_handle_error(struct super_block *sb)
  * @vol:	ntfs volume on which to modify the flags
  * @set_bits:	bits to set in the volume information flags
  * @clear_bits:	bits to clear in the volume information flags
+ * @dirty_if_errors:	force VOLUME_IS_DIRTY on when NVolErrors() is set
  *
  * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
- * instead (see below).
+ * or ntfs_sync_volume_dirty_state() instead (see below).
  *
  * 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,
@@ -368,11 +379,18 @@ void ntfs_handle_error(struct super_block *sb)
  * All bit manipulation is done on CPU-endian values, and the result is
  * converted back to little-endian before storing it.
  *
+ * When @dirty_if_errors is true and errors have been recorded on @vol,
+ * VOLUME_IS_DIRTY is forced on after the requested changes.  NVolErrors() is
+ * evaluated under the same mrec_lock, which orders this against other
+ * locked flag updates; the runtime error paths themselves record the flag
+ * lock-free, so see ntfs_sync_volume_dirty_state() for the guarantee this
+ * provides against them.
+ *
  * Return 0 on success and -errno on error.
  */
 static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 		const __le16 set_bits, const __le16 clear_bits,
-		const bool skip_if_errors)
+		const bool dirty_if_errors)
 {
 	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
 	struct volume_information *vi;
@@ -382,12 +400,11 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 
 	mutex_lock(&ni->mrec_lock);
 
-	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));
+	if (dirty_if_errors && NVolErrors(vol))
+		flags |= le16_to_cpu(VOLUME_IS_DIRTY);
 	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
 			le16_to_cpu(vol->vol_flags), flags);
 
@@ -455,15 +472,43 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
 }
 
 /*
- * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
- * @vol:	ntfs volume whose dirty bit should be cleared
+ * ntfs_sync_volume_dirty_state - persist the dirty bit per the error state
+ * @vol:	ntfs volume whose dirty bit to persist
+ *
+ * Set VOLUME_IS_DIRTY if errors have been recorded on @vol and clear it
+ * otherwise, under the $Volume mrec_lock.
+ *
+ * The guarantee this provides is eventual, not instantaneous: the runtime
+ * error paths record NVolErrors() with a lock-free set_bit(), so a
+ * persistence point that evaluates the flag just before an error is
+ * recorded can still leave the on-disk bit clean.  This is sound because
+ * NVolErrors() is sticky (nothing clears it for the lifetime of the mount)
+ * and every persistence point re-derives the on-disk bit from it; the
+ * last one, ntfs_put_super(), runs after evict_inodes() on a quiesced
+ * filesystem, so a volume that is read-write at unmount time cannot
+ * unmount clean.  A volume that is already read-only when the error is
+ * recorded (errors=remount-ro flips the superblock on the first error,
+ * as does an earlier remount-ro) has no persistence point left and
+ * keeps whatever on-disk bit it had; that behaviour is unchanged.  The
+ * residual window is a crash between the error and the next
+ * persistence point.
+ *
+ * This is the single point that persists the in-memory error state to disk.
+ * The runtime error paths only record NVolErrors() because they run under a
+ * variety of ntfs locks the dirty-bit write cannot be taken under (runlist
+ * locks, vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks); the first
+ * ntfs_sync_fs(), a remount, or the unmount then persists the flag here.
  *
- * 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.
+ * A hibernated volume is not written from these persistence paths:
+ * resuming Windows from a modified image corrupts it, so the dirty bit
+ * is left as it is on disk and only the in-memory error state is kept.
+ *
+ * Return 0 on success and -errno on error.
  */
-static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
+static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol)
 {
+	if (NVolHibernated(vol))
+		return 0;
 	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
 }
 
@@ -1621,6 +1666,11 @@ static bool load_system_files(struct ntfs_volume *vol)
 			ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
 		}
 		NVolSetErrors(vol);
+		/*
+		 * Remember it for the lifetime of the mount: see
+		 * ntfs_sync_volume_dirty_state().
+		 */
+		NVolSetHibernated(vol);
 	}
 
 	/* If (still) a read-write mount, empty the logfile. */
@@ -1776,22 +1826,31 @@ static void ntfs_put_super(struct super_block *sb)
 	ntfs_commit_inode(vol->mft_ino);
 
 	/*
-	 * If a read-write mount and no volume errors have occurred, mark the
-	 * volume clean.  Also, re-commit all affected inodes.
+	 * If a read-write mount, persist the error state in the volume flags:
+	 * mark the volume clean if no volume errors have occurred, and make
+	 * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs on the
+	 * next mount.  Also, re-commit all affected inodes.
 	 */
 	if (!sb_rdonly(sb)) {
+		if (ntfs_sync_volume_dirty_state(vol)) {
+			ntfs_warning(sb,
+				"Failed to sync dirty bit in volume information flags.  Run chkdsk.");
+		} else if (NVolErrors(vol)) {
+			/*
+			 * The dirty bit is on disk now; only warn when the
+			 * sync actually succeeded, or this message would
+			 * contradict the one above.
+			 */
+			ntfs_warning(sb,
+				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
+		}
+		/* Commits the updated volume flags if they were written. */
+		ntfs_commit_inode(vol->vol_ino);
 		if (!NVolErrors(vol)) {
-			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
-				ntfs_warning(sb,
-					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
-			ntfs_commit_inode(vol->vol_ino);
 			ntfs_commit_inode(vol->root_ino);
 			if (vol->mftmirr_ino)
 				ntfs_commit_inode(vol->mftmirr_ino);
 			ntfs_commit_inode(vol->mft_ino);
-		} else {
-			ntfs_warning(sb,
-				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
 		}
 	}
 
@@ -1890,8 +1949,8 @@ 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_dirty_if_no_errors(vol)) {
-		ntfs_warning(sb, "Failed to clear dirty bit in volume information flags.  Run chkdsk.");
+	if (ntfs_sync_volume_dirty_state(vol)) {
+		ntfs_warning(sb, "Failed to sync dirty bit in volume information flags.  Run chkdsk.");
 		err = -EIO;
 	}
 	sync_inodes_sb(sb);
diff --git a/fs/ntfs/volume.h b/fs/ntfs/volume.h
index 65fd3908af26..b946263153db 100644
--- a/fs/ntfs/volume.h
+++ b/fs/ntfs/volume.h
@@ -175,6 +175,8 @@ struct ntfs_volume {
  *				Windows-reserved names (CON, AUX, NUL, COM1,
  *				LPT1, etc.) or invalid characters.
  *
+ * NV_Hibernated		Windows is hibernated on the volume; the sync
+ *				paths must not write the volume flags.
  * NV_Discard			Issue discard/TRIM commands for freed clusters.
  * NV_DisableSparse		Disable creation of sparse regions.
  * NV_NativeSymlinkRel		Translate absolute Windows reparse targets (native_symlink=rel).
@@ -193,6 +195,7 @@ enum {
 	NV_ShowHiddenFiles,
 	NV_HideDotFiles,
 	NV_CheckWindowsNames,
+	NV_Hibernated,
 	NV_Discard,
 	NV_DisableSparse,
 	NV_NativeSymlinkRel,
@@ -231,6 +234,7 @@ DEFINE_NVOL_BIT_OPS(SysImmutable)
 DEFINE_NVOL_BIT_OPS(ShowHiddenFiles)
 DEFINE_NVOL_BIT_OPS(HideDotFiles)
 DEFINE_NVOL_BIT_OPS(CheckWindowsNames)
+DEFINE_NVOL_BIT_OPS(Hibernated)
 DEFINE_NVOL_BIT_OPS(Discard)
 DEFINE_NVOL_BIT_OPS(DisableSparse)
 DEFINE_NVOL_BIT_OPS(NativeSymlinkRel)
-- 
2.25.1

[PATCH v3 3/3] ntfs: NULL vol->vol_ino in the load_system_files() error teardown

[Hongling Zeng]

The error unwind of load_system_files() drops vol->vol_ino on two
paths but leaves the stale pointer in place while it keeps iput()ing
the remaining system inodes ($Bitmap, $MFT bitmap, $MFTMirr; $MFT
itself is dropped by the caller, ntfs_fill_super()).  A third path
carried an iput() that can never execute: it sits inside the
IS_ERR(vol->vol_ino) branch guarded by !IS_ERR(vol->vol_ino), and is
removed along with the stale pointers.  Nothing in the teardown
dereferences vol_ino today, so this is pure hygiene, but a stale
pointer to a freed inode surviving the unwind is a trap for any
future code walking the volume during teardown.

ntfs_put_super() and the ntfs_fill_super() error path already NULL it
after their iput(); do the same here.

Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 fs/ntfs/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 4b4af1ea8b09..2d4132aa39d3 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -1541,8 +1541,7 @@ static bool load_system_files(struct ntfs_volume *vol)
 	 */
 	vol->vol_ino = ntfs_iget(sb, FILE_Volume);
 	if (IS_ERR(vol->vol_ino)) {
-		if (!IS_ERR(vol->vol_ino))
-			iput(vol->vol_ino);
+		vol->vol_ino = NULL;
 volume_failed:
 		ntfs_error(sb, "Failed to load $Volume.");
 		goto iput_lcnbmp_err_out;
@@ -1551,6 +1550,7 @@ static bool load_system_files(struct ntfs_volume *vol)
 	if (IS_ERR(m)) {
 iput_volume_failed:
 		iput(vol->vol_ino);
+		vol->vol_ino = NULL;
 		goto volume_failed;
 	}
 
@@ -1716,6 +1716,8 @@ static bool load_system_files(struct ntfs_volume *vol)
 	if (vol->logfile_ino)
 		iput(vol->logfile_ino);
 	iput(vol->vol_ino);
+	/* Do not leave a stale pointer behind for the rest of the teardown. */
+	vol->vol_ino = NULL;
 iput_lcnbmp_err_out:
 	iput(vol->lcnbmp_ino);
 iput_attrdef_err_out:
-- 
2.25.1

Re: [PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[Namjae Jeon]

> @@ -1621,6 +1666,11 @@ static bool load_system_files(struct ntfs_volume *vol)
>                         ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
>                 }
>                 NVolSetErrors(vol);
> +               /*
> +                * Remember it for the lifetime of the mount: see
> +                * ntfs_sync_volume_dirty_state().
> +                */
> +               NVolSetHibernated(vol);
Could you explain why NVolSetHibernated(vol) is called for non-zero
return from check_windows_hibernation_status() ?

Re: [PATCH v3 0/3] ntfs: fix volume flag update races and persist the recorded error state

[liubaolin]

在 2026/9/7 15:08, Hongling Zeng 写道:
> Hi all,
> 
> First of all, my apologies for the many versions of this patch.  I
> should have folded the reviewer feedback in and resent a single coherent
> series sooner; the split across v1/v2 and follow-ups made the thread
> harder to follow than it needed to be.  This v3 consolidates everything
> into one series of three patches.
> 
> To recap the review discussion:
> 
> The original patch (now 1/3) fixed the lost-update race where
> ntfs_set_volume_flags() and ntfs_clear_volume_flags() computed the new
> value from vol->vol_flags outside any lock.  The review correctly
> pointed out that a race remained in ntfs_sync_fs(): it checked
> NVolErrors() outside the critical section, so a concurrent error path
> could still record an error after the check and before the clear, and
> the volume would end up persisted as clean despite the recorded error,
> meaning chkdsk would not run on the next mount.
> 
> That race is now fixed in 1/3 itself: the NVolErrors() check and the
> clearing of VOLUME_IS_DIRTY both happen inside the same mrec_lock
> critical section, via a dedicated ntfs_clear_volume_dirty_if_no_errors()
> helper.  With that, every interleaving of the sync thread and the error
> path leaves the volume dirty on disk.
> 
> Patches 2/3 then close the writer-side half of the window: the runtime
> metadata-corruption paths in fs/ntfs recorded only the in-memory
> NVolErrors() flag and never persisted VOLUME_IS_DIRTY at all, so a
> volume could unmount with a clean on-disk flag despite recorded
> corruption.  Persisting from the error paths themselves is not an
> option: they run under a wide variety of ntfs locks (runlist locks,
> vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks) while the dirty-bit
> write needs the $Volume mrec_lock and can take the $MFT runlist lock,
> which self-deadlocks or forms ABBA cycles.  Instead, 2/3 makes
> persistence a property of the lock-free sync contexts: the new
> ntfs_sync_volume_dirty_state() sets VOLUME_IS_DIRTY when NVolErrors()
> is recorded and clears it otherwise, evaluating the error flag under
> the $Volume mrec_lock.  It is called from ntfs_sync_fs(), the
> remount-to-read-only path and ntfs_put_super(), with NV_Hibernated
> gating so a hibernated volume is never written by these paths.  The
> guarantee is eventual rather than instantaneous (a crash between the
> error record and the next persistence point remains a window);
> ntfs_put_super() runs on a quiesced filesystem after evict_inodes(),
> so a volume that is read-write at unmount time cannot unmount clean.
> 
> Patch 3/3 is pure hygiene found during review: the load_system_files()
> error unwind leaves a stale vol->vol_ino pointer in place, and carries
> a dead iput() inside the IS_ERR(vol->vol_ino) branch; NULL the pointer
> like ntfs_put_super() and the ntfs_fill_super() error path already do.
> 
> One housekeeping note: this series was rebased onto the current ntfs
> tree after the first posting failed to apply there.  Dennis Tighe's
> "ntfs: do not mark the volume clean in sync_fs when errors were
> recorded" landed in the meantime and fixed part of the same sync_fs
> problem this series addresses; patch 2/3 supersedes that fix with the
> ntfs_sync_volume_dirty_state() helper, which guards the clear in every
> persistence context, not just sync_fs.  No conflict with its intent.
> 
> Patch layout:
> 
>    1/3 ntfs: fix volume flag update races
>        Move the read-modify-write of vol->vol_flags inside the
>        $Volume mrec_lock; ntfs_sync_fs() clears the dirty bit under
>        the lock with the NVolErrors() check.
> 
>    2/3 ntfs: sync the volume dirty bit with the recorded error state
>        New ntfs_sync_volume_dirty_state(), called from the lock-free
>        sync contexts (sync_fs / remount-ro / put_super); NV_Hibernated
>        gating.
> 
>    3/3 ntfs: NULL vol->vol_ino in the load_system_files() error
>        teardown
>        Drop the stale pointer and the dead iput().
> 
> The series builds cleanly with W=1.
> 
> Thanks,
> Hongling

Hi Hongling,
    Thanks for putting this together. I left a couple of comments on 1/3 
and 2/3. Patch 3/3 looks good to me.

Thanks,
Baolin.

Re: [PATCH v3 3/3] ntfs: NULL vol->vol_ino in the load_system_files() error teardown

[liubaolin]

在 2026/9/7 15:08, Hongling Zeng 写道:
> The error unwind of load_system_files() drops vol->vol_ino on two
> paths but leaves the stale pointer in place while it keeps iput()ing
> the remaining system inodes ($Bitmap, $MFT bitmap, $MFTMirr; $MFT
> itself is dropped by the caller, ntfs_fill_super()).  A third path
> carried an iput() that can never execute: it sits inside the
> IS_ERR(vol->vol_ino) branch guarded by !IS_ERR(vol->vol_ino), and is
> removed along with the stale pointers.  Nothing in the teardown
> dereferences vol_ino today, so this is pure hygiene, but a stale
> pointer to a freed inode surviving the unwind is a trap for any
> future code walking the volume during teardown.
> 
> ntfs_put_super() and the ntfs_fill_super() error path already NULL it
> after their iput(); do the same here.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>   fs/ntfs/super.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 4b4af1ea8b09..2d4132aa39d3 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -1541,8 +1541,7 @@ static bool load_system_files(struct ntfs_volume *vol)
>   	 */
>   	vol->vol_ino = ntfs_iget(sb, FILE_Volume);
>   	if (IS_ERR(vol->vol_ino)) {
> -		if (!IS_ERR(vol->vol_ino))
> -			iput(vol->vol_ino);
> +		vol->vol_ino = NULL;
>   volume_failed:
>   		ntfs_error(sb, "Failed to load $Volume.");
>   		goto iput_lcnbmp_err_out;
> @@ -1551,6 +1550,7 @@ static bool load_system_files(struct ntfs_volume *vol)
>   	if (IS_ERR(m)) {
>   iput_volume_failed:
>   		iput(vol->vol_ino);
> +		vol->vol_ino = NULL;
>   		goto volume_failed;
>   	}
>   
> @@ -1716,6 +1716,8 @@ static bool load_system_files(struct ntfs_volume *vol)
>   	if (vol->logfile_ino)
>   		iput(vol->logfile_ino);
>   	iput(vol->vol_ino);
> +	/* Do not leave a stale pointer behind for the rest of the teardown. */
> +	vol->vol_ino = NULL;
>   iput_lcnbmp_err_out:
>   	iput(vol->lcnbmp_ino);
>   iput_attrdef_err_out:

Looks good to me.

Reviewed-by: Baolin Liu <liubaolin@kylinos.cn>

Re: [PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[liubaolin]

在 2026/9/7 15:08, Hongling Zeng 写道:
> The runtime metadata-corruption paths in fs/ntfs only record the
> in-memory NVolErrors() flag; whether VOLUME_IS_DIRTY ever reaches disk
> depends on ntfs_set_volume_flags() being called by some other path,
> which for most error sites never happens.  A volume can therefore
> unmount with a clean on-disk flag despite recorded corruption, and
> chkdsk will not run on the next mount.
> 
> Persisting the dirty bit from the error paths themselves does not work:
> they run under a wide variety of ntfs locks, and the dirty-bit write
> takes the $Volume mrec_lock and maps the $Volume mft record, which on
> an $MFT page-cache miss takes the $MFT runlist lock for writing.  That
> is enough to self-deadlock or form ABBA cycles from several of them:
> the $MFT extend undo paths hold the $MFT runlist lock and then take
> vol->lcnbmp_lock inside ntfs_cluster_free(); the cluster allocation and
> free rollback paths hold vol->lcnbmp_lock; and the whole mft record
> allocation tree is reachable from ntfs_write_volume_label()'s
> attribute-list maintenance while it holds the $Volume mrec_lock itself.
> 
> Instead, make the persistence a property of the sync paths, which run
> without ntfs locks held.  The new ntfs_sync_volume_dirty_state() sets
> VOLUME_IS_DIRTY when NVolErrors() is recorded and clears it otherwise,
> evaluating the error flag under the $Volume mrec_lock.  It is called
> from ntfs_sync_fs(), from the remount-to-read-only path of
> ntfs_reconfigure(), and from ntfs_put_super(), which previously
> evaluated NVolErrors() outside the lock before clearing the dirty bit
> unconditionally, and which now also persists the dirty bit for volumes
> with recorded errors so they unmount with chkdsk scheduled.
> 
> The guarantee this provides is eventual, not instantaneous: the error
> paths record NVolErrors() with a lock-free set_bit(), so a persistence
> point that evaluates the flag just before an error is recorded can
> still leave the on-disk bit clean until the next one.  This is sound
> because NVolErrors() is sticky for the lifetime of the mount and every
> persistence point re-derives the on-disk bit from it; the last one,
> ntfs_put_super(), runs after evict_inodes() on a quiesced filesystem,
> so a volume that is read-write at unmount time cannot unmount clean.
> A volume that is already read-only when the error is recorded
> (errors=remount-ro flips the superblock on the first error, as does an
> earlier remount-ro) has no persistence point left and keeps whatever
> on-disk bit it had; that behaviour is unchanged.  The residual window
> is a crash between the error and the next persistence point.
> 
> The persistence paths never write a hibernated volume: resuming Windows
> from a modified image corrupts it.  Record the mount-time hibernation
> verdict in the new NV_Hibernated volume flag and make
> ntfs_sync_volume_dirty_state() a no-op while it is set, so the dirty
> bit is left exactly as it is on disk and only the in-memory error
> state is kept.  Without this, an rw mount of a hibernated volume with
> the default errors=continue would gain a filesystem-internal write on
> the first sync, remount or unmount.  Other writes to such a mount,
> like the mount-time logfile emptying, are pre-existing and unchanged.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
>   fs/ntfs/super.c  | 113 ++++++++++++++++++++++++++++++++++++-----------
>   fs/ntfs/volume.h |   4 ++
>   2 files changed, 90 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index ce159ae8169a..4b4af1ea8b09 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -262,6 +262,8 @@ static int ntfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>   	return 0;
>   }
>   
> +static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol);
> +
>   static int ntfs_reconfigure(struct fs_context *fc)
>   {
>   	struct super_block *sb = fc->root->d_sb;
> @@ -312,11 +314,19 @@ static int ntfs_reconfigure(struct fs_context *fc)
>   		}
>   	} else if (!sb_rdonly(sb) && (fc->sb_flags & SB_RDONLY)) {
>   		/* Remounting read-only. */
> -		if (!NVolErrors(vol)) {
> -			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
> -				ntfs_warning(sb,
> -					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
> -		}
> +		/*
> +		 * With errors recorded the dirty bit is set rather than
> +		 * cleared, so it survives until the unmount.  Note that
> +		 * once this remount succeeds no further persistence point
> +		 * exists: ntfs_sync_fs() is only ever invoked for
> +		 * read-write superblocks (all its VFS callers skip
> +		 * read-only ones) and ntfs_put_super() skips them, so an
> +		 * error recorded only after the remount is never
> +		 * persisted.
> +		 */
> +		if (ntfs_sync_volume_dirty_state(vol))
> +			ntfs_warning(sb,
> +				"Failed to update dirty bit in volume information flags.  Run chkdsk.");
>   	}
>   
>   	ntfs_debug("Done.");
> @@ -357,9 +367,10 @@ void ntfs_handle_error(struct super_block *sb)
>    * @vol:	ntfs volume on which to modify the flags
>    * @set_bits:	bits to set in the volume information flags
>    * @clear_bits:	bits to clear in the volume information flags
> + * @dirty_if_errors:	force VOLUME_IS_DIRTY on when NVolErrors() is set
>    *
>    * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
> - * instead (see below).
> + * or ntfs_sync_volume_dirty_state() instead (see below).
>    *
>    * 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,
> @@ -368,11 +379,18 @@ void ntfs_handle_error(struct super_block *sb)
>    * All bit manipulation is done on CPU-endian values, and the result is
>    * converted back to little-endian before storing it.
>    *
> + * When @dirty_if_errors is true and errors have been recorded on @vol,
> + * VOLUME_IS_DIRTY is forced on after the requested changes.  NVolErrors() is
> + * evaluated under the same mrec_lock, which orders this against other
> + * locked flag updates; the runtime error paths themselves record the flag
> + * lock-free, so see ntfs_sync_volume_dirty_state() for the guarantee this
> + * provides against them.
> + *
>    * Return 0 on success and -errno on error.
>    */
>   static int ntfs_write_volume_flags(struct ntfs_volume *vol,
>   		const __le16 set_bits, const __le16 clear_bits,
> -		const bool skip_if_errors)
> +		const bool dirty_if_errors)
>   {
>   	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
>   	struct volume_information *vi;
> @@ -382,12 +400,11 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
>   
>   	mutex_lock(&ni->mrec_lock);
>   
> -	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));
> +	if (dirty_if_errors && NVolErrors(vol))
> +		flags |= le16_to_cpu(VOLUME_IS_DIRTY);
>   	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
>   			le16_to_cpu(vol->vol_flags), flags);
>   
> @@ -455,15 +472,43 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
>   }
>   
>   /*
> - * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
> - * @vol:	ntfs volume whose dirty bit should be cleared
> + * ntfs_sync_volume_dirty_state - persist the dirty bit per the error state
> + * @vol:	ntfs volume whose dirty bit to persist
> + *
> + * Set VOLUME_IS_DIRTY if errors have been recorded on @vol and clear it
> + * otherwise, under the $Volume mrec_lock.
> + *
> + * The guarantee this provides is eventual, not instantaneous: the runtime
> + * error paths record NVolErrors() with a lock-free set_bit(), so a
> + * persistence point that evaluates the flag just before an error is
> + * recorded can still leave the on-disk bit clean.  This is sound because
> + * NVolErrors() is sticky (nothing clears it for the lifetime of the mount)
> + * and every persistence point re-derives the on-disk bit from it; the
> + * last one, ntfs_put_super(), runs after evict_inodes() on a quiesced
> + * filesystem, so a volume that is read-write at unmount time cannot
> + * unmount clean.  A volume that is already read-only when the error is
> + * recorded (errors=remount-ro flips the superblock on the first error,
> + * as does an earlier remount-ro) has no persistence point left and
> + * keeps whatever on-disk bit it had; that behaviour is unchanged.  The
> + * residual window is a crash between the error and the next
> + * persistence point.
> + *
> + * This is the single point that persists the in-memory error state to disk.
> + * The runtime error paths only record NVolErrors() because they run under a
> + * variety of ntfs locks the dirty-bit write cannot be taken under (runlist
> + * locks, vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks); the first
> + * ntfs_sync_fs(), a remount, or the unmount then persists the flag here.
>    *
> - * 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.
> + * A hibernated volume is not written from these persistence paths:
> + * resuming Windows from a modified image corrupts it, so the dirty bit
> + * is left as it is on disk and only the in-memory error state is kept.
> + *
> + * Return 0 on success and -errno on error.
>    */
> -static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
> +static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol)
>   {
> +	if (NVolHibernated(vol))
> +		return 0;
>   	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
>   }
>   
> @@ -1621,6 +1666,11 @@ static bool load_system_files(struct ntfs_volume *vol)
>   			ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
>   		}
>   		NVolSetErrors(vol);
> +		/*
> +		 * Remember it for the lifetime of the mount: see
> +		 * ntfs_sync_volume_dirty_state().
> +		 */
> +		NVolSetHibernated(vol);
Hi Hongling,
    check_windows_hibernation_status() returns a positive value when 
hibernation is detected, but a negative value when the check itself 
fails. This sets NV_Hibernated in both cases, so dirty-bit updates are 
also skipped after a detection error.

Thanks,
Baolin.

>   	}
>   
>   	/* If (still) a read-write mount, empty the logfile. */
> @@ -1776,22 +1826,31 @@ static void ntfs_put_super(struct super_block *sb)
>   	ntfs_commit_inode(vol->mft_ino);
>   
>   	/*
> -	 * If a read-write mount and no volume errors have occurred, mark the
> -	 * volume clean.  Also, re-commit all affected inodes.
> +	 * If a read-write mount, persist the error state in the volume flags:
> +	 * mark the volume clean if no volume errors have occurred, and make
> +	 * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs on the
> +	 * next mount.  Also, re-commit all affected inodes.
>   	 */
>   	if (!sb_rdonly(sb)) {
> +		if (ntfs_sync_volume_dirty_state(vol)) {
> +			ntfs_warning(sb,
> +				"Failed to sync dirty bit in volume information flags.  Run chkdsk.");
Hi Hongling,
    This looks a little too early. There are more ntfs_commit_inode() 
calls,and a write_inode_now(), later in ntfs_put_super(). 
__ntfs_write_inode() sets NVolErrors() on failure, so an error recorded 
after this point would not be reflected in the on-disk dirty bit.

   Could the dirty state be synchronized after the remaining commits, 
while vol->vol_ino is still available?

Thanks,
Baolin.

> +		} else if (NVolErrors(vol)) {
> +			/*
> +			 * The dirty bit is on disk now; only warn when the
> +			 * sync actually succeeded, or this message would
> +			 * contradict the one above.
> +			 */
> +			ntfs_warning(sb,
> +				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
> +		}
> +		/* Commits the updated volume flags if they were written. */
> +		ntfs_commit_inode(vol->vol_ino);
>   		if (!NVolErrors(vol)) {
> -			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
> -				ntfs_warning(sb,
> -					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
> -			ntfs_commit_inode(vol->vol_ino);
>   			ntfs_commit_inode(vol->root_ino);
>   			if (vol->mftmirr_ino)
>   				ntfs_commit_inode(vol->mftmirr_ino);
>   			ntfs_commit_inode(vol->mft_ino);
> -		} else {
> -			ntfs_warning(sb,
> -				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
>   		}
>   	}
>   
> @@ -1890,8 +1949,8 @@ 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_dirty_if_no_errors(vol)) {
> -		ntfs_warning(sb, "Failed to clear dirty bit in volume information flags.  Run chkdsk.");
> +	if (ntfs_sync_volume_dirty_state(vol)) {
> +		ntfs_warning(sb, "Failed to sync dirty bit in volume information flags.  Run chkdsk.");
>   		err = -EIO;
>   	}
>   	sync_inodes_sb(sb);
> diff --git a/fs/ntfs/volume.h b/fs/ntfs/volume.h
> index 65fd3908af26..b946263153db 100644
> --- a/fs/ntfs/volume.h
> +++ b/fs/ntfs/volume.h
> @@ -175,6 +175,8 @@ struct ntfs_volume {
>    *				Windows-reserved names (CON, AUX, NUL, COM1,
>    *				LPT1, etc.) or invalid characters.
>    *
> + * NV_Hibernated		Windows is hibernated on the volume; the sync
> + *				paths must not write the volume flags.
>    * NV_Discard			Issue discard/TRIM commands for freed clusters.
>    * NV_DisableSparse		Disable creation of sparse regions.
>    * NV_NativeSymlinkRel		Translate absolute Windows reparse targets (native_symlink=rel).
> @@ -193,6 +195,7 @@ enum {
>   	NV_ShowHiddenFiles,
>   	NV_HideDotFiles,
>   	NV_CheckWindowsNames,
> +	NV_Hibernated,
>   	NV_Discard,
>   	NV_DisableSparse,
>   	NV_NativeSymlinkRel,
> @@ -231,6 +234,7 @@ DEFINE_NVOL_BIT_OPS(SysImmutable)
>   DEFINE_NVOL_BIT_OPS(ShowHiddenFiles)
>   DEFINE_NVOL_BIT_OPS(HideDotFiles)
>   DEFINE_NVOL_BIT_OPS(CheckWindowsNames)
> +DEFINE_NVOL_BIT_OPS(Hibernated)
>   DEFINE_NVOL_BIT_OPS(Discard)
>   DEFINE_NVOL_BIT_OPS(DisableSparse)
>   DEFINE_NVOL_BIT_OPS(NativeSymlinkRel)

Re: [PATCH v3 1/3] ntfs: fix volume flag update races

[liubaolin]

在 2026/9/7 15:08, 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>
> ---
>   fs/ntfs/super.c | 63 +++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 60d43339c590..ce159ae8169a 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);
>   }
Hi Hongling,
    Doesn't the caller-side check still leave a race here? Several paths 
in file.c and namei.c do:

   	if (!(vol->vol_flags & VOLUME_IS_DIRTY))
   		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);

   If the check sees the dirty bit set and sync_fs clears it before the 
metadata update, the caller skips setting it and the volume may be left 
clean. Since ntfs_write_volume_flags() already checks for an unchanged 
value under mrec_lock, should these callers invoke 
ntfs_set_volume_flags() unconditionally?

Thanks,
Baolin.

>   
>   /*
> @@ -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,8 +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 (!NVolErrors(vol) &&
> -	    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;
>   	}

Re: [PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[Hongling Zeng]

在 2026年09月08日 12:10, Namjae Jeon 写道:
>> @@ -1621,6 +1666,11 @@ static bool load_system_files(struct ntfs_volume *vol)
>>                          ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
>>                  }
>>                  NVolSetErrors(vol);
>> +               /*
>> +                * Remember it for the lifetime of the mount: see
>> +                * ntfs_sync_volume_dirty_state().
>> +                */
>> +               NVolSetHibernated(vol);
> Could you explain why NVolSetHibernated(vol) is called for non-zero
> return from check_windows_hibernation_status() ?
Hi:
   A non-zero return does not only mean "definitely hibernated": > 0
   means hibernation was established or assumed (the documented
   "treated as if Windows is suspended" default), while < 0 means the
   check itself failed, so hibernation could not be ruled out.

   NV_Hibernated is a conservative write gate, not a positive claim: it
   keeps ntfs_sync_volume_dirty_state() from writing the $Volume record
   on this mount. It has to cover the whole branch because this series
   changes what NVolErrors() means for writes: it used to suppress the
   flags write, and now the on-disk dirty bit is derived from it on
   read-write mounts. Without the separate gate, an rw mount of a
   hibernated volume (errors=continue) would gain a filesystem-internal
   $Volume write on the first sync_fs() or at unmount - exactly what
   the hibernation check exists to prevent.

   err < 0 is included for the same reason, only more so: when the
   check cannot complete, we cannot prove the volume is safe to write.
   Gating wrongly costs an untouched dirty bit; not gating wrongly
   costs Windows resuming on a modified image.

Thanks!

Re: [PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[Hongling Zeng]

在 2026年09月08日 13:23, liubaolin 写道:
>
>
> 在 2026/9/7 15:08, Hongling Zeng 写道:
>> The runtime metadata-corruption paths in fs/ntfs only record the
>> in-memory NVolErrors() flag; whether VOLUME_IS_DIRTY ever reaches disk
>> depends on ntfs_set_volume_flags() being called by some other path,
>> which for most error sites never happens.  A volume can therefore
>> unmount with a clean on-disk flag despite recorded corruption, and
>> chkdsk will not run on the next mount.
>>
>> Persisting the dirty bit from the error paths themselves does not work:
>> they run under a wide variety of ntfs locks, and the dirty-bit write
>> takes the $Volume mrec_lock and maps the $Volume mft record, which on
>> an $MFT page-cache miss takes the $MFT runlist lock for writing.  That
>> is enough to self-deadlock or form ABBA cycles from several of them:
>> the $MFT extend undo paths hold the $MFT runlist lock and then take
>> vol->lcnbmp_lock inside ntfs_cluster_free(); the cluster allocation and
>> free rollback paths hold vol->lcnbmp_lock; and the whole mft record
>> allocation tree is reachable from ntfs_write_volume_label()'s
>> attribute-list maintenance while it holds the $Volume mrec_lock itself.
>>
>> Instead, make the persistence a property of the sync paths, which run
>> without ntfs locks held.  The new ntfs_sync_volume_dirty_state() sets
>> VOLUME_IS_DIRTY when NVolErrors() is recorded and clears it otherwise,
>> evaluating the error flag under the $Volume mrec_lock.  It is called
>> from ntfs_sync_fs(), from the remount-to-read-only path of
>> ntfs_reconfigure(), and from ntfs_put_super(), which previously
>> evaluated NVolErrors() outside the lock before clearing the dirty bit
>> unconditionally, and which now also persists the dirty bit for volumes
>> with recorded errors so they unmount with chkdsk scheduled.
>>
>> The guarantee this provides is eventual, not instantaneous: the error
>> paths record NVolErrors() with a lock-free set_bit(), so a persistence
>> point that evaluates the flag just before an error is recorded can
>> still leave the on-disk bit clean until the next one.  This is sound
>> because NVolErrors() is sticky for the lifetime of the mount and every
>> persistence point re-derives the on-disk bit from it; the last one,
>> ntfs_put_super(), runs after evict_inodes() on a quiesced filesystem,
>> so a volume that is read-write at unmount time cannot unmount clean.
>> A volume that is already read-only when the error is recorded
>> (errors=remount-ro flips the superblock on the first error, as does an
>> earlier remount-ro) has no persistence point left and keeps whatever
>> on-disk bit it had; that behaviour is unchanged.  The residual window
>> is a crash between the error and the next persistence point.
>>
>> The persistence paths never write a hibernated volume: resuming Windows
>> from a modified image corrupts it.  Record the mount-time hibernation
>> verdict in the new NV_Hibernated volume flag and make
>> ntfs_sync_volume_dirty_state() a no-op while it is set, so the dirty
>> bit is left exactly as it is on disk and only the in-memory error
>> state is kept.  Without this, an rw mount of a hibernated volume with
>> the default errors=continue would gain a filesystem-internal write on
>> the first sync, remount or unmount.  Other writes to such a mount,
>> like the mount-time logfile emptying, are pre-existing and unchanged.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>> ---
>>   fs/ntfs/super.c  | 113 ++++++++++++++++++++++++++++++++++++-----------
>>   fs/ntfs/volume.h |   4 ++
>>   2 files changed, 90 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
>> index ce159ae8169a..4b4af1ea8b09 100644
>> --- a/fs/ntfs/super.c
>> +++ b/fs/ntfs/super.c
>> @@ -262,6 +262,8 @@ static int ntfs_parse_param(struct fs_context 
>> *fc, struct fs_parameter *param)
>>       return 0;
>>   }
>>   +static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol);
>> +
>>   static int ntfs_reconfigure(struct fs_context *fc)
>>   {
>>       struct super_block *sb = fc->root->d_sb;
>> @@ -312,11 +314,19 @@ static int ntfs_reconfigure(struct fs_context *fc)
>>           }
>>       } else if (!sb_rdonly(sb) && (fc->sb_flags & SB_RDONLY)) {
>>           /* Remounting read-only. */
>> -        if (!NVolErrors(vol)) {
>> -            if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
>> -                ntfs_warning(sb,
>> -                    "Failed to clear dirty bit in volume information 
>> flags.  Run chkdsk.");
>> -        }
>> +        /*
>> +         * With errors recorded the dirty bit is set rather than
>> +         * cleared, so it survives until the unmount.  Note that
>> +         * once this remount succeeds no further persistence point
>> +         * exists: ntfs_sync_fs() is only ever invoked for
>> +         * read-write superblocks (all its VFS callers skip
>> +         * read-only ones) and ntfs_put_super() skips them, so an
>> +         * error recorded only after the remount is never
>> +         * persisted.
>> +         */
>> +        if (ntfs_sync_volume_dirty_state(vol))
>> +            ntfs_warning(sb,
>> +                "Failed to update dirty bit in volume information 
>> flags.  Run chkdsk.");
>>       }
>>         ntfs_debug("Done.");
>> @@ -357,9 +367,10 @@ void ntfs_handle_error(struct super_block *sb)
>>    * @vol:    ntfs volume on which to modify the flags
>>    * @set_bits:    bits to set in the volume information flags
>>    * @clear_bits:    bits to clear in the volume information flags
>> + * @dirty_if_errors:    force VOLUME_IS_DIRTY on when NVolErrors() 
>> is set
>>    *
>>    * Internal function.  You probably want to use 
>> ntfs_{set,clear}_volume_flags()
>> - * instead (see below).
>> + * or ntfs_sync_volume_dirty_state() instead (see below).
>>    *
>>    * 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,
>> @@ -368,11 +379,18 @@ void ntfs_handle_error(struct super_block *sb)
>>    * All bit manipulation is done on CPU-endian values, and the 
>> result is
>>    * converted back to little-endian before storing it.
>>    *
>> + * When @dirty_if_errors is true and errors have been recorded on @vol,
>> + * VOLUME_IS_DIRTY is forced on after the requested changes. 
>> NVolErrors() is
>> + * evaluated under the same mrec_lock, which orders this against other
>> + * locked flag updates; the runtime error paths themselves record 
>> the flag
>> + * lock-free, so see ntfs_sync_volume_dirty_state() for the 
>> guarantee this
>> + * provides against them.
>> + *
>>    * Return 0 on success and -errno on error.
>>    */
>>   static int ntfs_write_volume_flags(struct ntfs_volume *vol,
>>           const __le16 set_bits, const __le16 clear_bits,
>> -        const bool skip_if_errors)
>> +        const bool dirty_if_errors)
>>   {
>>       struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
>>       struct volume_information *vi;
>> @@ -382,12 +400,11 @@ static int ntfs_write_volume_flags(struct 
>> ntfs_volume *vol,
>>         mutex_lock(&ni->mrec_lock);
>>   -    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));
>> +    if (dirty_if_errors && NVolErrors(vol))
>> +        flags |= le16_to_cpu(VOLUME_IS_DIRTY);
>>       ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
>>               le16_to_cpu(vol->vol_flags), flags);
>>   @@ -455,15 +472,43 @@ int ntfs_clear_volume_flags(struct 
>> ntfs_volume *vol, __le16 flags)
>>   }
>>     /*
>> - * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no 
>> errors exist
>> - * @vol:    ntfs volume whose dirty bit should be cleared
>> + * ntfs_sync_volume_dirty_state - persist the dirty bit per the 
>> error state
>> + * @vol:    ntfs volume whose dirty bit to persist
>> + *
>> + * Set VOLUME_IS_DIRTY if errors have been recorded on @vol and 
>> clear it
>> + * otherwise, under the $Volume mrec_lock.
>> + *
>> + * The guarantee this provides is eventual, not instantaneous: the 
>> runtime
>> + * error paths record NVolErrors() with a lock-free set_bit(), so a
>> + * persistence point that evaluates the flag just before an error is
>> + * recorded can still leave the on-disk bit clean.  This is sound 
>> because
>> + * NVolErrors() is sticky (nothing clears it for the lifetime of the 
>> mount)
>> + * and every persistence point re-derives the on-disk bit from it; the
>> + * last one, ntfs_put_super(), runs after evict_inodes() on a quiesced
>> + * filesystem, so a volume that is read-write at unmount time cannot
>> + * unmount clean.  A volume that is already read-only when the error is
>> + * recorded (errors=remount-ro flips the superblock on the first error,
>> + * as does an earlier remount-ro) has no persistence point left and
>> + * keeps whatever on-disk bit it had; that behaviour is unchanged.  The
>> + * residual window is a crash between the error and the next
>> + * persistence point.
>> + *
>> + * This is the single point that persists the in-memory error state 
>> to disk.
>> + * The runtime error paths only record NVolErrors() because they run 
>> under a
>> + * variety of ntfs locks the dirty-bit write cannot be taken under 
>> (runlist
>> + * locks, vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks); the first
>> + * ntfs_sync_fs(), a remount, or the unmount then persists the flag 
>> here.
>>    *
>> - * 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.
>> + * A hibernated volume is not written from these persistence paths:
>> + * resuming Windows from a modified image corrupts it, so the dirty bit
>> + * is left as it is on disk and only the in-memory error state is kept.
>> + *
>> + * Return 0 on success and -errno on error.
>>    */
>> -static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume 
>> *vol)
>> +static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol)
>>   {
>> +    if (NVolHibernated(vol))
>> +        return 0;
>>       return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
>>   }
>>   @@ -1621,6 +1666,11 @@ static bool load_system_files(struct 
>> ntfs_volume *vol)
>>               ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
>>           }
>>           NVolSetErrors(vol);
>> +        /*
>> +         * Remember it for the lifetime of the mount: see
>> +         * ntfs_sync_volume_dirty_state().
>> +         */
>> +        NVolSetHibernated(vol);
> Hi Hongling,
>    check_windows_hibernation_status() returns a positive value when 
> hibernation is detected, but a negative value when the check itself 
> fails. This sets NV_Hibernated in both cases, so dirty-bit updates are 
> also skipped after a detection error.
>
> Thanks,
> Baolin.
>
   Thanks, Deliberate: the flag is a write gate, not a detection claim. 
A failed  check means we cannot prove the volume is not held by a hibernated
   Windows, and gating wrongly costs only an untouched dirty bit, while  
writing wrongly costs Windows resuming on a modified image.
>>       }
>>         /* If (still) a read-write mount, empty the logfile. */
>> @@ -1776,22 +1826,31 @@ static void ntfs_put_super(struct super_block 
>> *sb)
>>       ntfs_commit_inode(vol->mft_ino);
>>         /*
>> -     * If a read-write mount and no volume errors have occurred, 
>> mark the
>> -     * volume clean.  Also, re-commit all affected inodes.
>> +     * If a read-write mount, persist the error state in the volume 
>> flags:
>> +     * mark the volume clean if no volume errors have occurred, and 
>> make
>> +     * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs 
>> on the
>> +     * next mount.  Also, re-commit all affected inodes.
>>        */
>>       if (!sb_rdonly(sb)) {
>> +        if (ntfs_sync_volume_dirty_state(vol)) {
>> +            ntfs_warning(sb,
>> +                "Failed to sync dirty bit in volume information 
>> flags.  Run chkdsk.");
> Hi Hongling,
>    This looks a little too early. There are more ntfs_commit_inode() 
> calls,and a write_inode_now(), later in ntfs_put_super(). 
> __ntfs_write_inode() sets NVolErrors() on failure, so an error 
> recorded after this point would not be reflected in the on-disk dirty 
> bit.
>
>   Could the dirty state be synchronized after the remaining commits, 
> while vol->vol_ino is still available?
>
> Thanks,
> Baolin.
   Yes - I'll move the sync and the vol_ino commit after the last 
write_inode_now(), with the vol_ino iput moved to the end of
   ntfs_put_super(), and post the incremental for 2/3, and I'll post it 
once  there is agreement on this approach.

   Thanks,
   Hongling

>
>> +        } else if (NVolErrors(vol)) {
>> +            /*
>> +             * The dirty bit is on disk now; only warn when the
>> +             * sync actually succeeded, or this message would
>> +             * contradict the one above.
>> +             */
>> +            ntfs_warning(sb,
>> +                "Volume has errors.  Leaving volume marked dirty.  
>> Run chkdsk.");
>> +        }
>> +        /* Commits the updated volume flags if they were written. */
>> +        ntfs_commit_inode(vol->vol_ino);
>>           if (!NVolErrors(vol)) {
>> -            if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
>> -                ntfs_warning(sb,
>> -                    "Failed to clear dirty bit in volume information 
>> flags.  Run chkdsk.");
>> -            ntfs_commit_inode(vol->vol_ino);
>>               ntfs_commit_inode(vol->root_ino);
>>               if (vol->mftmirr_ino)
>>                   ntfs_commit_inode(vol->mftmirr_ino);
>>               ntfs_commit_inode(vol->mft_ino);
>> -        } else {
>> -            ntfs_warning(sb,
>> -                "Volume has errors.  Leaving volume marked dirty.  
>> Run chkdsk.");
>>           }
>>       }
>>   @@ -1890,8 +1949,8 @@ 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_dirty_if_no_errors(vol)) {
>> -        ntfs_warning(sb, "Failed to clear dirty bit in volume 
>> information flags.  Run chkdsk.");
>> +    if (ntfs_sync_volume_dirty_state(vol)) {
>> +        ntfs_warning(sb, "Failed to sync dirty bit in volume 
>> information flags.  Run chkdsk.");
>>           err = -EIO;
>>       }
>>       sync_inodes_sb(sb);
>> diff --git a/fs/ntfs/volume.h b/fs/ntfs/volume.h
>> index 65fd3908af26..b946263153db 100644
>> --- a/fs/ntfs/volume.h
>> +++ b/fs/ntfs/volume.h
>> @@ -175,6 +175,8 @@ struct ntfs_volume {
>>    *                Windows-reserved names (CON, AUX, NUL, COM1,
>>    *                LPT1, etc.) or invalid characters.
>>    *
>> + * NV_Hibernated        Windows is hibernated on the volume; the sync
>> + *                paths must not write the volume flags.
>>    * NV_Discard            Issue discard/TRIM commands for freed 
>> clusters.
>>    * NV_DisableSparse        Disable creation of sparse regions.
>>    * NV_NativeSymlinkRel        Translate absolute Windows reparse 
>> targets (native_symlink=rel).
>> @@ -193,6 +195,7 @@ enum {
>>       NV_ShowHiddenFiles,
>>       NV_HideDotFiles,
>>       NV_CheckWindowsNames,
>> +    NV_Hibernated,
>>       NV_Discard,
>>       NV_DisableSparse,
>>       NV_NativeSymlinkRel,
>> @@ -231,6 +234,7 @@ DEFINE_NVOL_BIT_OPS(SysImmutable)
>>   DEFINE_NVOL_BIT_OPS(ShowHiddenFiles)
>>   DEFINE_NVOL_BIT_OPS(HideDotFiles)
>>   DEFINE_NVOL_BIT_OPS(CheckWindowsNames)
>> +DEFINE_NVOL_BIT_OPS(Hibernated)
>>   DEFINE_NVOL_BIT_OPS(Discard)
>>   DEFINE_NVOL_BIT_OPS(DisableSparse)
>>   DEFINE_NVOL_BIT_OPS(NativeSymlinkRel)

Re: [PATCH v3 1/3] ntfs: fix volume flag update races

[Hongling Zeng]

在 2026年09月08日 13:29, liubaolin 写道:
>
>
> 在 2026/9/7 15:08, 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>
>> ---
>>   fs/ntfs/super.c | 63 +++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 45 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
>> index 60d43339c590..ce159ae8169a 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);
>>   }
> Hi Hongling,
>    Doesn't the caller-side check still leave a race here? Several 
> paths in file.c and namei.c do:
>
>       if (!(vol->vol_flags & VOLUME_IS_DIRTY))
>           ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
>
>   If the check sees the dirty bit set and sync_fs clears it before the 
> metadata update, the caller skips setting it and the volume may be 
> left clean. Since ntfs_write_volume_flags() already checks for an 
> unchanged value under mrec_lock, should these callers invoke 
> ntfs_set_volume_flags() unconditionally?
>
> Thanks,
> Baolin.
>
   Yes ，you are right - the check reads vol->vol_flags without the 
lock, so it can race
   with the sync_fs() clear exactly as you describe. I'll drop the 
caller-side checks in file.c and namei.c and call
   ntfs_set_volume_flags() unconditionally; when the bit is already 
set,  the mrec_lock-protected unchanged check keeps that a no-op. I'll post
   an incremental for 1/3 once there is agreement on this.

   Thanks,
   Hongling

>>     /*
>> @@ -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,8 +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 (!NVolErrors(vol) &&
>> -        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;
>>       }

Re: [PATCH v3 3/3] ntfs: NULL vol->vol_ino in the load_system_files() error teardown

[Namjae Jeon]

On Mon, Sep 7, 2026 at 4:09 PM Hongling Zeng <zenghongling@kylinos.cn> wrote:
>
> The error unwind of load_system_files() drops vol->vol_ino on two
> paths but leaves the stale pointer in place while it keeps iput()ing
> the remaining system inodes ($Bitmap, $MFT bitmap, $MFTMirr; $MFT
> itself is dropped by the caller, ntfs_fill_super()).  A third path
> carried an iput() that can never execute: it sits inside the
> IS_ERR(vol->vol_ino) branch guarded by !IS_ERR(vol->vol_ino), and is
> removed along with the stale pointers.  Nothing in the teardown
> dereferences vol_ino today, so this is pure hygiene, but a stale
> pointer to a freed inode surviving the unwind is a trap for any
> future code walking the volume during teardown.
>
> ntfs_put_super() and the ntfs_fill_super() error path already NULL it
> after their iput(); do the same here.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
Applied it to #ntfs-next.
Thanks!

[PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[Hongling Zeng]

The runtime metadata-corruption paths in fs/ntfs only record the
in-memory NVolErrors() flag; whether VOLUME_IS_DIRTY ever reaches disk
depends on ntfs_set_volume_flags() being called by some other path,
which for most error sites never happens.  A volume can therefore
unmount with a clean on-disk flag despite recorded corruption, and
chkdsk will not run on the next mount.

Persisting the dirty bit from the error paths themselves does not work:
they run under a wide variety of ntfs locks, and the dirty-bit write
takes the $Volume mrec_lock and maps the $Volume mft record, which on
an $MFT page-cache miss takes the $MFT runlist lock for writing.  That
is enough to self-deadlock or form ABBA cycles from several of them:
the $MFT extend undo paths hold the $MFT runlist lock and then take
vol->lcnbmp_lock inside ntfs_cluster_free(); the cluster allocation and
free rollback paths hold vol->lcnbmp_lock; and the whole mft record
allocation tree is reachable from ntfs_write_volume_label()'s
attribute-list maintenance while it holds the $Volume mrec_lock itself.

Instead, make the persistence a property of the sync paths, which run
without ntfs locks held.  The new ntfs_sync_volume_dirty_state() sets
VOLUME_IS_DIRTY when NVolErrors() is recorded and clears it otherwise,
evaluating the error flag under the $Volume mrec_lock.  It is called
from ntfs_sync_fs(), from the remount-to-read-only path of
ntfs_reconfigure(), and from ntfs_put_super(), which previously
evaluated NVolErrors() outside the lock before clearing the dirty bit
unconditionally, and which now also persists the dirty bit for volumes
with recorded errors so they unmount with chkdsk scheduled.

The guarantee this provides is eventual, not instantaneous: the error
paths record NVolErrors() with a lock-free set_bit(), so a persistence
point that evaluates the flag just before an error is recorded can
still leave the on-disk bit clean until the next one.  This is sound
because NVolErrors() is sticky for the lifetime of the mount and every
persistence point re-derives the on-disk bit from it; the last one,
ntfs_put_super(), runs after evict_inodes() on a quiesced filesystem,
so a volume that is read-write at unmount time cannot unmount clean.
A volume that is already read-only when the error is recorded
(errors=remount-ro flips the superblock on the first error, as does an
earlier remount-ro) has no persistence point left and keeps whatever
on-disk bit it had; that behaviour is unchanged.  The residual window
is a crash between the error and the next persistence point.

The persistence paths never write a hibernated volume: resuming Windows
from a modified image corrupts it.  Record the mount-time hibernation
verdict in the new NV_Hibernated volume flag and make
ntfs_sync_volume_dirty_state() a no-op while it is set, so the dirty
bit is left exactly as it is on disk and only the in-memory error
state is kept.  Without this, an rw mount of a hibernated volume with
the default errors=continue would gain a filesystem-internal write on
the first sync, remount or unmount.  Other writes to such a mount,
like the mount-time logfile emptying, are pre-existing and unchanged.

Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 fs/ntfs/super.c  | 113 ++++++++++++++++++++++++++++++++++++-----------
 fs/ntfs/volume.h |   4 ++
 2 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index ce159ae8169a..4b4af1ea8b09 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -262,6 +262,8 @@ static int ntfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	return 0;
 }
 
+static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol);
+
 static int ntfs_reconfigure(struct fs_context *fc)
 {
 	struct super_block *sb = fc->root->d_sb;
@@ -312,11 +314,19 @@ static int ntfs_reconfigure(struct fs_context *fc)
 		}
 	} else if (!sb_rdonly(sb) && (fc->sb_flags & SB_RDONLY)) {
 		/* Remounting read-only. */
-		if (!NVolErrors(vol)) {
-			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
-				ntfs_warning(sb,
-					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
-		}
+		/*
+		 * With errors recorded the dirty bit is set rather than
+		 * cleared, so it survives until the unmount.  Note that
+		 * once this remount succeeds no further persistence point
+		 * exists: ntfs_sync_fs() is only ever invoked for
+		 * read-write superblocks (all its VFS callers skip
+		 * read-only ones) and ntfs_put_super() skips them, so an
+		 * error recorded only after the remount is never
+		 * persisted.
+		 */
+		if (ntfs_sync_volume_dirty_state(vol))
+			ntfs_warning(sb,
+				"Failed to update dirty bit in volume information flags.  Run chkdsk.");
 	}
 
 	ntfs_debug("Done.");
@@ -357,9 +367,10 @@ void ntfs_handle_error(struct super_block *sb)
  * @vol:	ntfs volume on which to modify the flags
  * @set_bits:	bits to set in the volume information flags
  * @clear_bits:	bits to clear in the volume information flags
+ * @dirty_if_errors:	force VOLUME_IS_DIRTY on when NVolErrors() is set
  *
  * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
- * instead (see below).
+ * or ntfs_sync_volume_dirty_state() instead (see below).
  *
  * 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,
@@ -368,11 +379,18 @@ void ntfs_handle_error(struct super_block *sb)
  * All bit manipulation is done on CPU-endian values, and the result is
  * converted back to little-endian before storing it.
  *
+ * When @dirty_if_errors is true and errors have been recorded on @vol,
+ * VOLUME_IS_DIRTY is forced on after the requested changes.  NVolErrors() is
+ * evaluated under the same mrec_lock, which orders this against other
+ * locked flag updates; the runtime error paths themselves record the flag
+ * lock-free, so see ntfs_sync_volume_dirty_state() for the guarantee this
+ * provides against them.
+ *
  * Return 0 on success and -errno on error.
  */
 static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 		const __le16 set_bits, const __le16 clear_bits,
-		const bool skip_if_errors)
+		const bool dirty_if_errors)
 {
 	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
 	struct volume_information *vi;
@@ -382,12 +400,11 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 
 	mutex_lock(&ni->mrec_lock);
 
-	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));
+	if (dirty_if_errors && NVolErrors(vol))
+		flags |= le16_to_cpu(VOLUME_IS_DIRTY);
 	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
 			le16_to_cpu(vol->vol_flags), flags);
 
@@ -455,15 +472,43 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
 }
 
 /*
- * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
- * @vol:	ntfs volume whose dirty bit should be cleared
+ * ntfs_sync_volume_dirty_state - persist the dirty bit per the error state
+ * @vol:	ntfs volume whose dirty bit to persist
+ *
+ * Set VOLUME_IS_DIRTY if errors have been recorded on @vol and clear it
+ * otherwise, under the $Volume mrec_lock.
+ *
+ * The guarantee this provides is eventual, not instantaneous: the runtime
+ * error paths record NVolErrors() with a lock-free set_bit(), so a
+ * persistence point that evaluates the flag just before an error is
+ * recorded can still leave the on-disk bit clean.  This is sound because
+ * NVolErrors() is sticky (nothing clears it for the lifetime of the mount)
+ * and every persistence point re-derives the on-disk bit from it; the
+ * last one, ntfs_put_super(), runs after evict_inodes() on a quiesced
+ * filesystem, so a volume that is read-write at unmount time cannot
+ * unmount clean.  A volume that is already read-only when the error is
+ * recorded (errors=remount-ro flips the superblock on the first error,
+ * as does an earlier remount-ro) has no persistence point left and
+ * keeps whatever on-disk bit it had; that behaviour is unchanged.  The
+ * residual window is a crash between the error and the next
+ * persistence point.
+ *
+ * This is the single point that persists the in-memory error state to disk.
+ * The runtime error paths only record NVolErrors() because they run under a
+ * variety of ntfs locks the dirty-bit write cannot be taken under (runlist
+ * locks, vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks); the first
+ * ntfs_sync_fs(), a remount, or the unmount then persists the flag here.
  *
- * 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.
+ * A hibernated volume is not written from these persistence paths:
+ * resuming Windows from a modified image corrupts it, so the dirty bit
+ * is left as it is on disk and only the in-memory error state is kept.
+ *
+ * Return 0 on success and -errno on error.
  */
-static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
+static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol)
 {
+	if (NVolHibernated(vol))
+		return 0;
 	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
 }
 
@@ -1621,6 +1666,11 @@ static bool load_system_files(struct ntfs_volume *vol)
 			ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
 		}
 		NVolSetErrors(vol);
+		/*
+		 * Remember it for the lifetime of the mount: see
+		 * ntfs_sync_volume_dirty_state().
+		 */
+		NVolSetHibernated(vol);
 	}
 
 	/* If (still) a read-write mount, empty the logfile. */
@@ -1776,22 +1826,31 @@ static void ntfs_put_super(struct super_block *sb)
 	ntfs_commit_inode(vol->mft_ino);
 
 	/*
-	 * If a read-write mount and no volume errors have occurred, mark the
-	 * volume clean.  Also, re-commit all affected inodes.
+	 * If a read-write mount, persist the error state in the volume flags:
+	 * mark the volume clean if no volume errors have occurred, and make
+	 * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs on the
+	 * next mount.  Also, re-commit all affected inodes.
 	 */
 	if (!sb_rdonly(sb)) {
+		if (ntfs_sync_volume_dirty_state(vol)) {
+			ntfs_warning(sb,
+				"Failed to sync dirty bit in volume information flags.  Run chkdsk.");
+		} else if (NVolErrors(vol)) {
+			/*
+			 * The dirty bit is on disk now; only warn when the
+			 * sync actually succeeded, or this message would
+			 * contradict the one above.
+			 */
+			ntfs_warning(sb,
+				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
+		}
+		/* Commits the updated volume flags if they were written. */
+		ntfs_commit_inode(vol->vol_ino);
 		if (!NVolErrors(vol)) {
-			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
-				ntfs_warning(sb,
-					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
-			ntfs_commit_inode(vol->vol_ino);
 			ntfs_commit_inode(vol->root_ino);
 			if (vol->mftmirr_ino)
 				ntfs_commit_inode(vol->mftmirr_ino);
 			ntfs_commit_inode(vol->mft_ino);
-		} else {
-			ntfs_warning(sb,
-				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
 		}
 	}
 
@@ -1890,8 +1949,8 @@ 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_dirty_if_no_errors(vol)) {
-		ntfs_warning(sb, "Failed to clear dirty bit in volume information flags.  Run chkdsk.");
+	if (ntfs_sync_volume_dirty_state(vol)) {
+		ntfs_warning(sb, "Failed to sync dirty bit in volume information flags.  Run chkdsk.");
 		err = -EIO;
 	}
 	sync_inodes_sb(sb);
diff --git a/fs/ntfs/volume.h b/fs/ntfs/volume.h
index 65fd3908af26..b946263153db 100644
--- a/fs/ntfs/volume.h
+++ b/fs/ntfs/volume.h
@@ -175,6 +175,8 @@ struct ntfs_volume {
  *				Windows-reserved names (CON, AUX, NUL, COM1,
  *				LPT1, etc.) or invalid characters.
  *
+ * NV_Hibernated		Windows is hibernated on the volume; the sync
+ *				paths must not write the volume flags.
  * NV_Discard			Issue discard/TRIM commands for freed clusters.
  * NV_DisableSparse		Disable creation of sparse regions.
  * NV_NativeSymlinkRel		Translate absolute Windows reparse targets (native_symlink=rel).
@@ -193,6 +195,7 @@ enum {
 	NV_ShowHiddenFiles,
 	NV_HideDotFiles,
 	NV_CheckWindowsNames,
+	NV_Hibernated,
 	NV_Discard,
 	NV_DisableSparse,
 	NV_NativeSymlinkRel,
@@ -231,6 +234,7 @@ DEFINE_NVOL_BIT_OPS(SysImmutable)
 DEFINE_NVOL_BIT_OPS(ShowHiddenFiles)
 DEFINE_NVOL_BIT_OPS(HideDotFiles)
 DEFINE_NVOL_BIT_OPS(CheckWindowsNames)
+DEFINE_NVOL_BIT_OPS(Hibernated)
 DEFINE_NVOL_BIT_OPS(Discard)
 DEFINE_NVOL_BIT_OPS(DisableSparse)
 DEFINE_NVOL_BIT_OPS(NativeSymlinkRel)
-- 
2.25.1

[PATCH v3 2/3] ntfs: sync the volume dirty bit with the recorded error state

[Hongling Zeng]

The runtime metadata-corruption paths in fs/ntfs only record the
in-memory NVolErrors() flag; whether VOLUME_IS_DIRTY ever reaches disk
depends on ntfs_set_volume_flags() being called by some other path,
which for most error sites never happens.  A volume can therefore
unmount with a clean on-disk flag despite recorded corruption, and
chkdsk will not run on the next mount.

Persisting the dirty bit from the error paths themselves does not work:
they run under a wide variety of ntfs locks, and the dirty-bit write
takes the $Volume mrec_lock and maps the $Volume mft record, which on
an $MFT page-cache miss takes the $MFT runlist lock for writing.  That
is enough to self-deadlock or form ABBA cycles from several of them:
the $MFT extend undo paths hold the $MFT runlist lock and then take
vol->lcnbmp_lock inside ntfs_cluster_free(); the cluster allocation and
free rollback paths hold vol->lcnbmp_lock; and the whole mft record
allocation tree is reachable from ntfs_write_volume_label()'s
attribute-list maintenance while it holds the $Volume mrec_lock itself.

Instead, make the persistence a property of the sync paths, which run
without ntfs locks held.  The new ntfs_sync_volume_dirty_state() sets
VOLUME_IS_DIRTY when NVolErrors() is recorded and clears it otherwise,
evaluating the error flag under the $Volume mrec_lock.  It is called
from ntfs_sync_fs(), from the remount-to-read-only path of
ntfs_reconfigure(), and from ntfs_put_super(), which previously
evaluated NVolErrors() outside the lock before clearing the dirty bit
unconditionally, and which now also persists the dirty bit for volumes
with recorded errors so they unmount with chkdsk scheduled.

The guarantee this provides is eventual, not instantaneous: the error
paths record NVolErrors() with a lock-free set_bit(), so a persistence
point that evaluates the flag just before an error is recorded can
still leave the on-disk bit clean until the next one.  This is sound
because NVolErrors() is sticky for the lifetime of the mount and every
persistence point re-derives the on-disk bit from it; the last one,
ntfs_put_super(), runs after evict_inodes() on a quiesced filesystem,
so a volume that is read-write at unmount time cannot unmount clean.
A volume that is already read-only when the error is recorded
(errors=remount-ro flips the superblock on the first error, as does an
earlier remount-ro) has no persistence point left and keeps whatever
on-disk bit it had; that behaviour is unchanged.  The residual window
is a crash between the error and the next persistence point.

The persistence paths never write a hibernated volume: resuming Windows
from a modified image corrupts it.  Record the mount-time hibernation
verdict in the new NV_Hibernated volume flag and make
ntfs_sync_volume_dirty_state() a no-op while it is set, so the dirty
bit is left exactly as it is on disk and only the in-memory error
state is kept.  Without this, an rw mount of a hibernated volume with
the default errors=continue would gain a filesystem-internal write on
the first sync, remount or unmount.  Other writes to such a mount,
like the mount-time logfile emptying, are pre-existing and unchanged.

Cc: stable@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
 fs/ntfs/super.c  | 113 ++++++++++++++++++++++++++++++++++++-----------
 fs/ntfs/volume.h |   4 ++
 2 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index a7977b95b967..920b1420a26f 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -262,6 +262,8 @@ static int ntfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	return 0;
 }
 
+static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol);
+
 static int ntfs_reconfigure(struct fs_context *fc)
 {
 	struct super_block *sb = fc->root->d_sb;
@@ -312,11 +314,19 @@ static int ntfs_reconfigure(struct fs_context *fc)
 		}
 	} else if (!sb_rdonly(sb) && (fc->sb_flags & SB_RDONLY)) {
 		/* Remounting read-only. */
-		if (!NVolErrors(vol)) {
-			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
-				ntfs_warning(sb,
-					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
-		}
+		/*
+		 * With errors recorded the dirty bit is set rather than
+		 * cleared, so it survives until the unmount.  Note that
+		 * once this remount succeeds no further persistence point
+		 * exists: ntfs_sync_fs() is only ever invoked for
+		 * read-write superblocks (all its VFS callers skip
+		 * read-only ones) and ntfs_put_super() skips them, so an
+		 * error recorded only after the remount is never
+		 * persisted.
+		 */
+		if (ntfs_sync_volume_dirty_state(vol))
+			ntfs_warning(sb,
+				"Failed to update dirty bit in volume information flags.  Run chkdsk.");
 	}
 
 	ntfs_debug("Done.");
@@ -357,9 +367,10 @@ void ntfs_handle_error(struct super_block *sb)
  * @vol:	ntfs volume on which to modify the flags
  * @set_bits:	bits to set in the volume information flags
  * @clear_bits:	bits to clear in the volume information flags
+ * @dirty_if_errors:	force VOLUME_IS_DIRTY on when NVolErrors() is set
  *
  * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
- * instead (see below).
+ * or ntfs_sync_volume_dirty_state() instead (see below).
  *
  * 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,
@@ -368,11 +379,18 @@ void ntfs_handle_error(struct super_block *sb)
  * All bit manipulation is done on CPU-endian values, and the result is
  * converted back to little-endian before storing it.
  *
+ * When @dirty_if_errors is true and errors have been recorded on @vol,
+ * VOLUME_IS_DIRTY is forced on after the requested changes.  NVolErrors() is
+ * evaluated under the same mrec_lock, which orders this against other
+ * locked flag updates; the runtime error paths themselves record the flag
+ * lock-free, so see ntfs_sync_volume_dirty_state() for the guarantee this
+ * provides against them.
+ *
  * Return 0 on success and -errno on error.
  */
 static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 		const __le16 set_bits, const __le16 clear_bits,
-		const bool skip_if_errors)
+		const bool dirty_if_errors)
 {
 	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
 	struct volume_information *vi;
@@ -382,12 +400,11 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol,
 
 	mutex_lock(&ni->mrec_lock);
 
-	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));
+	if (dirty_if_errors && NVolErrors(vol))
+		flags |= le16_to_cpu(VOLUME_IS_DIRTY);
 	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
 			le16_to_cpu(vol->vol_flags), flags);
 
@@ -455,15 +472,43 @@ int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
 }
 
 /*
- * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
- * @vol:	ntfs volume whose dirty bit should be cleared
+ * ntfs_sync_volume_dirty_state - persist the dirty bit per the error state
+ * @vol:	ntfs volume whose dirty bit to persist
+ *
+ * Set VOLUME_IS_DIRTY if errors have been recorded on @vol and clear it
+ * otherwise, under the $Volume mrec_lock.
+ *
+ * The guarantee this provides is eventual, not instantaneous: the runtime
+ * error paths record NVolErrors() with a lock-free set_bit(), so a
+ * persistence point that evaluates the flag just before an error is
+ * recorded can still leave the on-disk bit clean.  This is sound because
+ * NVolErrors() is sticky (nothing clears it for the lifetime of the mount)
+ * and every persistence point re-derives the on-disk bit from it; the
+ * last one, ntfs_put_super(), runs after evict_inodes() on a quiesced
+ * filesystem, so a volume that is read-write at unmount time cannot
+ * unmount clean.  A volume that is already read-only when the error is
+ * recorded (errors=remount-ro flips the superblock on the first error,
+ * as does an earlier remount-ro) has no persistence point left and
+ * keeps whatever on-disk bit it had; that behaviour is unchanged.  The
+ * residual window is a crash between the error and the next
+ * persistence point.
+ *
+ * This is the single point that persists the in-memory error state to disk.
+ * The runtime error paths only record NVolErrors() because they run under a
+ * variety of ntfs locks the dirty-bit write cannot be taken under (runlist
+ * locks, vol->lcnbmp_lock, vol->mftbmp_lock, mrec_locks); the first
+ * ntfs_sync_fs(), a remount, or the unmount then persists the flag here.
  *
- * 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.
+ * A hibernated volume is not written from these persistence paths:
+ * resuming Windows from a modified image corrupts it, so the dirty bit
+ * is left as it is on disk and only the in-memory error state is kept.
+ *
+ * Return 0 on success and -errno on error.
  */
-static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
+static int ntfs_sync_volume_dirty_state(struct ntfs_volume *vol)
 {
+	if (NVolHibernated(vol))
+		return 0;
 	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
 }
 
@@ -1621,6 +1666,11 @@ static bool load_system_files(struct ntfs_volume *vol)
 			ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
 		}
 		NVolSetErrors(vol);
+		/*
+		 * Remember it for the lifetime of the mount: see
+		 * ntfs_sync_volume_dirty_state().
+		 */
+		NVolSetHibernated(vol);
 	}
 
 	/* If (still) a read-write mount, empty the logfile. */
@@ -1776,22 +1826,31 @@ static void ntfs_put_super(struct super_block *sb)
 	ntfs_commit_inode(vol->mft_ino);
 
 	/*
-	 * If a read-write mount and no volume errors have occurred, mark the
-	 * volume clean.  Also, re-commit all affected inodes.
+	 * If a read-write mount, persist the error state in the volume flags:
+	 * mark the volume clean if no volume errors have occurred, and make
+	 * sure VOLUME_IS_DIRTY is on disk if any have, so chkdsk runs on the
+	 * next mount.  Also, re-commit all affected inodes.
 	 */
 	if (!sb_rdonly(sb)) {
+		if (ntfs_sync_volume_dirty_state(vol)) {
+			ntfs_warning(sb,
+				"Failed to sync dirty bit in volume information flags.  Run chkdsk.");
+		} else if (NVolErrors(vol)) {
+			/*
+			 * The dirty bit is on disk now; only warn when the
+			 * sync actually succeeded, or this message would
+			 * contradict the one above.
+			 */
+			ntfs_warning(sb,
+				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
+		}
+		/* Commits the updated volume flags if they were written. */
+		ntfs_commit_inode(vol->vol_ino);
 		if (!NVolErrors(vol)) {
-			if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))
-				ntfs_warning(sb,
-					"Failed to clear dirty bit in volume information flags.  Run chkdsk.");
-			ntfs_commit_inode(vol->vol_ino);
 			ntfs_commit_inode(vol->root_ino);
 			if (vol->mftmirr_ino)
 				ntfs_commit_inode(vol->mftmirr_ino);
 			ntfs_commit_inode(vol->mft_ino);
-		} else {
-			ntfs_warning(sb,
-				"Volume has errors.  Leaving volume marked dirty.  Run chkdsk.");
 		}
 	}
 
@@ -1890,8 +1949,8 @@ 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_dirty_if_no_errors(vol)) {
-		ntfs_warning(sb, "Failed to clear dirty bit in volume information flags.  Run chkdsk.");
+	if (ntfs_sync_volume_dirty_state(vol)) {
+		ntfs_warning(sb, "Failed to sync dirty bit in volume information flags.  Run chkdsk.");
 		err = -EIO;
 	}
 	sync_inodes_sb(sb);
diff --git a/fs/ntfs/volume.h b/fs/ntfs/volume.h
index 65fd3908af26..b946263153db 100644
--- a/fs/ntfs/volume.h
+++ b/fs/ntfs/volume.h
@@ -175,6 +175,8 @@ struct ntfs_volume {
  *				Windows-reserved names (CON, AUX, NUL, COM1,
  *				LPT1, etc.) or invalid characters.
  *
+ * NV_Hibernated		Windows is hibernated on the volume; the sync
+ *				paths must not write the volume flags.
  * NV_Discard			Issue discard/TRIM commands for freed clusters.
  * NV_DisableSparse		Disable creation of sparse regions.
  * NV_NativeSymlinkRel		Translate absolute Windows reparse targets (native_symlink=rel).
@@ -193,6 +195,7 @@ enum {
 	NV_ShowHiddenFiles,
 	NV_HideDotFiles,
 	NV_CheckWindowsNames,
+	NV_Hibernated,
 	NV_Discard,
 	NV_DisableSparse,
 	NV_NativeSymlinkRel,
@@ -231,6 +234,7 @@ DEFINE_NVOL_BIT_OPS(SysImmutable)
 DEFINE_NVOL_BIT_OPS(ShowHiddenFiles)
 DEFINE_NVOL_BIT_OPS(HideDotFiles)
 DEFINE_NVOL_BIT_OPS(CheckWindowsNames)
+DEFINE_NVOL_BIT_OPS(Hibernated)
 DEFINE_NVOL_BIT_OPS(Discard)
 DEFINE_NVOL_BIT_OPS(DisableSparse)
 DEFINE_NVOL_BIT_OPS(NativeSymlinkRel)
-- 
2.25.1