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

[PATCH 1/2] 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>
---
- Also fix the ntfs_sync_fs() race by checking NVolErrors() and clearing
  VOLUME_IS_DIRTY under ni->mrec_lock.
- Keep ntfs_set_volume_flags() and ntfs_clear_volume_flags() semantics
  unchanged.
- Do not tie setting VOLUME_IS_DIRTY to NVolSetErrors() in the generic
  set helper.
---
 fs/ntfs/super.c | 62 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index a1813093222b..a7977b95b967 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -353,31 +353,45 @@ void ntfs_handle_error(struct super_block *sb)
 }
 
 /*
- * ntfs_write_volume_flags - write new flags to the volume information flags
+ * ntfs_write_volume_flags - apply flag changes to the volume information flags
  * @vol:	ntfs volume on which to modify the flags
- * @flags:	new flags value for the volume information flags
+ * @set_bits:	bits to set in the volume information flags
+ * @clear_bits:	bits to clear in the volume information flags
  *
  * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
  * instead (see below).
  *
- * Replace the volume information flags on the volume @vol with the value
- * supplied in @flags.  Note, this overwrites the volume information flags, so
- * make sure to combine the flags you want to modify with the old flags and use
- * the result when calling ntfs_write_volume_flags().
+ * Combine @set_bits and @clear_bits with the current in-memory flag state and
+ * write the result back.  The set/clear helpers pass only the bits to modify,
+ * not the complete flag state.  The read-modify-write happens under
+ * ni->mrec_lock so that concurrent set/clear operations cannot lose updates.
+ * All bit manipulation is done on CPU-endian values, and the result is
+ * converted back to little-endian before storing it.
  *
  * Return 0 on success and -errno on error.
  */
-static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
+static int ntfs_write_volume_flags(struct ntfs_volume *vol,
+		const __le16 set_bits, const __le16 clear_bits,
+		const bool skip_if_errors)
 {
 	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
 	struct volume_information *vi;
 	struct ntfs_attr_search_ctx *ctx;
+	u16 flags;
 	int err;
 
-	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
-			le16_to_cpu(vol->vol_flags), le16_to_cpu(flags));
 	mutex_lock(&ni->mrec_lock);
-	if (vol->vol_flags == flags)
+
+	if (skip_if_errors && NVolErrors(vol))
+		goto done;
+
+	flags = le16_to_cpu(vol->vol_flags);
+	flags |= le16_to_cpu(set_bits) & le16_to_cpu(VOLUME_FLAGS_MASK);
+	flags &= ~(le16_to_cpu(clear_bits) & le16_to_cpu(VOLUME_FLAGS_MASK));
+	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
+			le16_to_cpu(vol->vol_flags), flags);
+
+	if (le16_to_cpu(vol->vol_flags) == flags)
 		goto done;
 
 	ctx = ntfs_attr_get_search_ctx(ni, NULL);
@@ -393,7 +407,7 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
 
 	vi = (struct volume_information *)((u8 *)ctx->attr +
 			le16_to_cpu(ctx->attr->data.resident.value_offset));
-	vol->vol_flags = vi->flags = flags;
+	vol->vol_flags = vi->flags = cpu_to_le16(flags);
 	mark_mft_record_dirty(ctx->ntfs_ino);
 	ntfs_attr_put_search_ctx(ctx);
 done:
@@ -414,13 +428,14 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
  * @flags:	flags to set on the volume
  *
  * Set the bits in @flags in the volume information flags on the volume @vol.
+ * The bits are combined with the current flag state under the lock in
+ * ntfs_write_volume_flags(), so concurrent updates are not lost.
  *
  * Return 0 on success and -errno on error.
  */
 int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
 {
-	flags &= VOLUME_FLAGS_MASK;
-	return ntfs_write_volume_flags(vol, vol->vol_flags | flags);
+	return ntfs_write_volume_flags(vol, flags, 0, false);
 }
 
 /*
@@ -429,14 +444,27 @@ int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
  * @flags:	flags to clear on the volume
  *
  * Clear the bits in @flags in the volume information flags on the volume @vol.
+ * The bits are combined with the current flag state under the lock in
+ * ntfs_write_volume_flags(), so concurrent updates are not lost.
  *
  * Return 0 on success and -errno on error.
  */
 int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
 {
-	flags &= VOLUME_FLAGS_MASK;
-	flags = vol->vol_flags & cpu_to_le16(~le16_to_cpu(flags));
-	return ntfs_write_volume_flags(vol, flags);
+	return ntfs_write_volume_flags(vol, 0, flags, false);
+}
+
+/*
+ * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
+ * @vol:	ntfs volume whose dirty bit should be cleared
+ *
+ * Check NVolErrors() and clear VOLUME_IS_DIRTY under the same mrec_lock so
+ * ntfs_sync_fs() cannot clear the dirty bit after a concurrent error has been
+ * recorded.
+ */
+static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
+{
+	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
 }
 
 int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
@@ -1862,7 +1890,7 @@ static int ntfs_sync_fs(struct super_block *sb, int wait)
 		return 0;
 
 	/* If there are some dirty buffers in the bdev inode */
-	if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY)) {
+	if (ntfs_clear_volume_dirty_if_no_errors(vol)) {
 		ntfs_warning(sb, "Failed to clear dirty bit in volume information flags.  Run chkdsk.");
 		err = -EIO;
 	}
-- 
2.25.1

[PATCH 2/2] ntfs: add helper to record error flag before setting volume dirty bit

[Hongling Zeng]

The runtime error paths in fs/ntfs/super.c record the error flag with a
bare NVolSetErrors() and separately rely on ntfs_set_volume_flags() or
nothing at all to persist VOLUME_IS_DIRTY.  Since the clear side
(ntfs_sync_fs() via ntfs_clear_volume_dirty_if_no_errors()) evaluates
NVolErrors() under ni->mrec_lock, a writer that records the error flag
after the lock has been checked leaves a window:

    Sync thread                          Error path
    -----------
    lock
    NVolErrors() == false
    clear VOLUME_IS_DIRTY
    unlock
                                         NVolSetErrors()

The on-disk volume ends up clean despite the recorded error, so chkdsk
will not run on the next mount.

Add ntfs_mark_volume_dirty_with_error() which records the error flag
before the locked dirty-bit write: the mutex acquire/release in
ntfs_write_volume_flags() then provides the ordering, as any clear path
running after us can only evaluate NVolErrors() under the lock once the
flag is set.  Convert the super.c error paths that run with $Volume
loaded and a writable bdev to use it:

 - the ntfs_reconfigure() failure to empty the LogFile on remount
   read-write.  This branch only runs when remounting read-only to
   read-write, and the VFS has already rejected the remount with
   -EACCES if the bdev is read-only (reconfigure_super()), so the
   dirty bit can be written here.  A hibernated volume never reaches
   this point because the earlier NVolErrors() check rejects the
   remount first;

 - the load_system_files() failure to empty the LogFile on a
   read-write mount.

The remaining super.c call sites keep calling NVolSetErrors() directly,
now with comments explaining why:

 - the $MFTMirr check failure runs before $Volume is loaded, so the
   dirty bit cannot be persisted yet;

 - the LogFile load failure is also reached on read-only mounts, which
   open the bdev read-only, so the dirty bit cannot be written;

 - the hibernation check must not write to the volume at all.

The helper is only for runtime error paths: it returns success without
doing anything if vol_ino is still NULL during mount, a state no
runtime caller can observe, rather than dereference a NULL inode;
mount-time error paths record the error flag with NVolSetErrors()
directly instead.  It stays static to this file; converting the runtime
metadata-corruption call sites in mft.c, lcnalloc.c, bitmap.c and
inode.c is left to a follow-up patch which will also move it to shared
code if needed.

This covers the writer-side ordering for the super.c error paths only;
it does not change ntfs_sync_fs(), which already clears the dirty bit
via the in-lock NVolErrors() check of
ntfs_clear_volume_dirty_if_no_errors().

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

diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index a7977b95b967..8c5681472002 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -262,6 +262,41 @@ static int ntfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	return 0;
 }
 
+/*
+ * ntfs_mark_volume_dirty_with_error - record an error and mark volume dirty
+ * @vol:	ntfs volume on which an error has been recorded
+ *
+ * To be called when metadata corruption is detected so that chkdsk runs on
+ * the next mount.  NVolSetErrors() is called before the dirty bit is
+ * written, and the mutex acquire/release in ntfs_write_volume_flags()
+ * provides the ordering: a concurrent ntfs_clear_volume_dirty_if_no_errors()
+ * can only run under the lock after the error flag is set and will
+ * therefore leave the dirty bit alone.
+ *
+ * Only for runtime error paths on a writable volume.  Do not call while
+ * holding the mrec_lock of the $Volume inode itself (the ntfs_attr_lookup()
+ * failure paths).  Error paths that run before $Volume is loaded, that may
+ * run with a read-only opened bdev, or on a hibernated volume, which we
+ * must not write to at all, keep calling NVolSetErrors() directly.
+ *
+ * Return 0 on success and -errno on error.
+ */
+static int ntfs_mark_volume_dirty_with_error(struct ntfs_volume *vol)
+{
+	/*
+	 * vol_ino is NULL while the volume is still being mounted.  This is a
+	 * runtime-only helper and no runtime caller can see that state, but if
+	 * one ever does, there is nothing on disk to update yet, so do nothing
+	 * rather than dereference a NULL inode.  Mount-time error paths record
+	 * the error flag with NVolSetErrors() directly instead.
+	 */
+	if (!vol->vol_ino)
+		return 0;
+
+	NVolSetErrors(vol);
+	return ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
+}
+
 static int ntfs_reconfigure(struct fs_context *fc)
 {
 	struct super_block *sb = fc->root->d_sb;
@@ -307,7 +342,7 @@ static int ntfs_reconfigure(struct fs_context *fc)
 		if (vol->logfile_ino && !ntfs_empty_logfile(vol->logfile_ino)) {
 			ntfs_error(sb, "Failed to empty journal LogFile%s",
 					es);
-			NVolSetErrors(vol);
+			ntfs_mark_volume_dirty_with_error(vol);
 			return -EROFS;
 		}
 	} else if (!sb_rdonly(sb) && (fc->sb_flags & SB_RDONLY)) {
@@ -1446,6 +1481,11 @@ static bool load_system_files(struct ntfs_volume *vol)
 			ntfs_error(sb, "%s.  Mounting read-only%s",
 					!vol->mftmirr_ino ? es1 : es2, es3);
 		}
+		/*
+		 * $Volume is not loaded yet, so the dirty bit cannot be
+		 * persisted; record the error flag only.  Mount is
+		 * serialized against sync anyway.
+		 */
 		NVolSetErrors(vol);
 	}
 	/* Get mft bitmap attribute inode. */
@@ -1588,6 +1628,11 @@ static bool load_system_files(struct ntfs_volume *vol)
 			sb->s_flags |= SB_RDONLY;
 			ntfs_error(sb, "Failed to load LogFile. Mounting read-only.");
 		}
+		/*
+		 * Read-only mounts reach this too with a read-only opened
+		 * bdev, so the dirty bit cannot be written; record the
+		 * error flag only.  Mount is serialized against sync anyway.
+		 */
 		NVolSetErrors(vol);
 	}
 
@@ -1633,7 +1678,7 @@ static bool load_system_files(struct ntfs_volume *vol)
 		/* Convert to a read-only mount. */
 		ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
 		sb->s_flags |= SB_RDONLY;
-		NVolSetErrors(vol);
+		ntfs_mark_volume_dirty_with_error(vol);
 	}
 	/* If on NTFS versions before 3.0, we are done. */
 	if (unlikely(vol->major_ver < 3))
-- 
2.25.1

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

[liubaolin]

在 2026/9/3 09:21, Hongling Zeng 写道:
> ntfs_set_volume_flags() and ntfs_clear_volume_flags() both read
> vol->vol_flags outside any lock to compute the new value before handing
> it to ntfs_write_volume_flags(), which only takes ni->mrec_lock around
> the actual write. The read-modify-write is therefore not atomic, and two
> concurrent callers can lose an update: ntfs_sync_fs() may derive a
> "clean" value from vol->vol_flags while a writer concurrently records an
> error and sets VOLUME_IS_DIRTY; the locked write then silently
> overwrites the freshly-set dirty bit. The on-disk volume looks clean
> despite the recorded errors, so chkdsk will not run on the next mount
> and corrupted metadata can persist.
> 
> Fix by moving the read-modify-write inside the mrec_lock: pass the bits
> to set and to clear separately, and combine them with the current flag
> state under the lock inside ntfs_write_volume_flags(). The set/clear
> helpers pass only the bits to modify, not the complete flag state. The
> bit manipulation is done on CPU-endian values, and the result is
> converted back to little-endian before storing it. The wrappers keep
> their signatures so callers are unchanged.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> - Also fix the ntfs_sync_fs() race by checking NVolErrors() and clearing
>    VOLUME_IS_DIRTY under ni->mrec_lock.
> - Keep ntfs_set_volume_flags() and ntfs_clear_volume_flags() semantics
>    unchanged.
> - Do not tie setting VOLUME_IS_DIRTY to NVolSetErrors() in the generic
>    set helper.
> ---
>   fs/ntfs/super.c | 62 +++++++++++++++++++++++++++++++++++--------------
>   1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index a1813093222b..a7977b95b967 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -353,31 +353,45 @@ void ntfs_handle_error(struct super_block *sb)
>   }
>   
>   /*
> - * ntfs_write_volume_flags - write new flags to the volume information flags
> + * ntfs_write_volume_flags - apply flag changes to the volume information flags
>    * @vol:	ntfs volume on which to modify the flags
> - * @flags:	new flags value for the volume information flags
> + * @set_bits:	bits to set in the volume information flags
> + * @clear_bits:	bits to clear in the volume information flags
>    *
>    * Internal function.  You probably want to use ntfs_{set,clear}_volume_flags()
>    * instead (see below).
>    *
> - * Replace the volume information flags on the volume @vol with the value
> - * supplied in @flags.  Note, this overwrites the volume information flags, so
> - * make sure to combine the flags you want to modify with the old flags and use
> - * the result when calling ntfs_write_volume_flags().
> + * Combine @set_bits and @clear_bits with the current in-memory flag state and
> + * write the result back.  The set/clear helpers pass only the bits to modify,
> + * not the complete flag state.  The read-modify-write happens under
> + * ni->mrec_lock so that concurrent set/clear operations cannot lose updates.
> + * All bit manipulation is done on CPU-endian values, and the result is
> + * converted back to little-endian before storing it.
>    *
>    * Return 0 on success and -errno on error.
>    */
> -static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
> +static int ntfs_write_volume_flags(struct ntfs_volume *vol,
> +		const __le16 set_bits, const __le16 clear_bits,
> +		const bool skip_if_errors)
>   {
>   	struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
>   	struct volume_information *vi;
>   	struct ntfs_attr_search_ctx *ctx;
> +	u16 flags;
>   	int err;
>   
> -	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
> -			le16_to_cpu(vol->vol_flags), le16_to_cpu(flags));
>   	mutex_lock(&ni->mrec_lock);
> -	if (vol->vol_flags == flags)
> +
> +	if (skip_if_errors && NVolErrors(vol))
> +		goto done;
> +
> +	flags = le16_to_cpu(vol->vol_flags);
> +	flags |= le16_to_cpu(set_bits) & le16_to_cpu(VOLUME_FLAGS_MASK);
> +	flags &= ~(le16_to_cpu(clear_bits) & le16_to_cpu(VOLUME_FLAGS_MASK));
> +	ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
> +			le16_to_cpu(vol->vol_flags), flags);
> +
> +	if (le16_to_cpu(vol->vol_flags) == flags)
>   		goto done;
>   
>   	ctx = ntfs_attr_get_search_ctx(ni, NULL);
> @@ -393,7 +407,7 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
>   
>   	vi = (struct volume_information *)((u8 *)ctx->attr +
>   			le16_to_cpu(ctx->attr->data.resident.value_offset));
> -	vol->vol_flags = vi->flags = flags;
> +	vol->vol_flags = vi->flags = cpu_to_le16(flags);
>   	mark_mft_record_dirty(ctx->ntfs_ino);
>   	ntfs_attr_put_search_ctx(ctx);
>   done:
> @@ -414,13 +428,14 @@ static int ntfs_write_volume_flags(struct ntfs_volume *vol, const __le16 flags)
>    * @flags:	flags to set on the volume
>    *
>    * Set the bits in @flags in the volume information flags on the volume @vol.
> + * The bits are combined with the current flag state under the lock in
> + * ntfs_write_volume_flags(), so concurrent updates are not lost.
>    *
>    * Return 0 on success and -errno on error.
>    */
>   int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
>   {
> -	flags &= VOLUME_FLAGS_MASK;
> -	return ntfs_write_volume_flags(vol, vol->vol_flags | flags);
> +	return ntfs_write_volume_flags(vol, flags, 0, false);
>   }
>   
>   /*
> @@ -429,14 +444,27 @@ int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
>    * @flags:	flags to clear on the volume
>    *
>    * Clear the bits in @flags in the volume information flags on the volume @vol.
> + * The bits are combined with the current flag state under the lock in
> + * ntfs_write_volume_flags(), so concurrent updates are not lost.
>    *
>    * Return 0 on success and -errno on error.
>    */
>   int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
>   {
> -	flags &= VOLUME_FLAGS_MASK;
> -	flags = vol->vol_flags & cpu_to_le16(~le16_to_cpu(flags));
> -	return ntfs_write_volume_flags(vol, flags);
> +	return ntfs_write_volume_flags(vol, 0, flags, false);
> +}
> +
> +/*
> + * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no errors exist
> + * @vol:	ntfs volume whose dirty bit should be cleared
> + *
> + * Check NVolErrors() and clear VOLUME_IS_DIRTY under the same mrec_lock so
> + * ntfs_sync_fs() cannot clear the dirty bit after a concurrent error has been
> + * recorded.
> + */
> +static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume *vol)
> +{
> +	return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
>   }
>   
>   int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
> @@ -1862,7 +1890,7 @@ static int ntfs_sync_fs(struct super_block *sb, int wait)
>   		return 0;
>   
>   	/* If there are some dirty buffers in the bdev inode */
> -	if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY)) {
> +	if (ntfs_clear_volume_dirty_if_no_errors(vol)) {
>   		ntfs_warning(sb, "Failed to clear dirty bit in volume information flags.  Run chkdsk.");
>   		err = -EIO;
>   	}

Hi Hongling,
   Patch 1 fixes the race in ntfs_sync_fs(), but there are two other 
sites with the same check-outside-lock-then-clear-inside-lock pattern:

   1. super.c:315 (ntfs_reconfigure, remount read-only path)
   2. super.c:1755 (ntfs_put_super, umount path)

   Both use:
       if (!NVolErrors(vol)) {   // check outside lock
           if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))  // clear 
inside lock

   The same race can occur: if an error thread sets NVolErrors() and the
   dirty bit after the check but before the lock acquisition, the clear
   operation will overwrite the freshly-set dirty bit.

   Although the race window is much narrower for remount/umount, should
   these two sites also be converted to use
   ntfs_clear_volume_dirty_if_no_errors() for consistency?

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


Thanks,
Baolin.

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

[Hongling Zeng]

在 2026年09月03日 11:01, liubaolin 写道:
>
>
> 在 2026/9/3 09:21, Hongling Zeng 写道:
>> ntfs_set_volume_flags() and ntfs_clear_volume_flags() both read
>> vol->vol_flags outside any lock to compute the new value before handing
>> it to ntfs_write_volume_flags(), which only takes ni->mrec_lock around
>> the actual write. The read-modify-write is therefore not atomic, and two
>> concurrent callers can lose an update: ntfs_sync_fs() may derive a
>> "clean" value from vol->vol_flags while a writer concurrently records an
>> error and sets VOLUME_IS_DIRTY; the locked write then silently
>> overwrites the freshly-set dirty bit. The on-disk volume looks clean
>> despite the recorded errors, so chkdsk will not run on the next mount
>> and corrupted metadata can persist.
>>
>> Fix by moving the read-modify-write inside the mrec_lock: pass the bits
>> to set and to clear separately, and combine them with the current flag
>> state under the lock inside ntfs_write_volume_flags(). The set/clear
>> helpers pass only the bits to modify, not the complete flag state. The
>> bit manipulation is done on CPU-endian values, and the result is
>> converted back to little-endian before storing it. The wrappers keep
>> their signatures so callers are unchanged.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>> ---
>> - Also fix the ntfs_sync_fs() race by checking NVolErrors() and clearing
>>    VOLUME_IS_DIRTY under ni->mrec_lock.
>> - Keep ntfs_set_volume_flags() and ntfs_clear_volume_flags() semantics
>>    unchanged.
>> - Do not tie setting VOLUME_IS_DIRTY to NVolSetErrors() in the generic
>>    set helper.
>> ---
>>   fs/ntfs/super.c | 62 +++++++++++++++++++++++++++++++++++--------------
>>   1 file changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
>> index a1813093222b..a7977b95b967 100644
>> --- a/fs/ntfs/super.c
>> +++ b/fs/ntfs/super.c
>> @@ -353,31 +353,45 @@ void ntfs_handle_error(struct super_block *sb)
>>   }
>>     /*
>> - * ntfs_write_volume_flags - write new flags to the volume 
>> information flags
>> + * ntfs_write_volume_flags - apply flag changes to the volume 
>> information flags
>>    * @vol:    ntfs volume on which to modify the flags
>> - * @flags:    new flags value for the volume information flags
>> + * @set_bits:    bits to set in the volume information flags
>> + * @clear_bits:    bits to clear in the volume information flags
>>    *
>>    * Internal function.  You probably want to use 
>> ntfs_{set,clear}_volume_flags()
>>    * instead (see below).
>>    *
>> - * Replace the volume information flags on the volume @vol with the 
>> value
>> - * supplied in @flags.  Note, this overwrites the volume information 
>> flags, so
>> - * make sure to combine the flags you want to modify with the old 
>> flags and use
>> - * the result when calling ntfs_write_volume_flags().
>> + * Combine @set_bits and @clear_bits with the current in-memory flag 
>> state and
>> + * write the result back.  The set/clear helpers pass only the bits 
>> to modify,
>> + * not the complete flag state.  The read-modify-write happens under
>> + * ni->mrec_lock so that concurrent set/clear operations cannot lose 
>> updates.
>> + * All bit manipulation is done on CPU-endian values, and the result is
>> + * converted back to little-endian before storing it.
>>    *
>>    * Return 0 on success and -errno on error.
>>    */
>> -static int ntfs_write_volume_flags(struct ntfs_volume *vol, const 
>> __le16 flags)
>> +static int ntfs_write_volume_flags(struct ntfs_volume *vol,
>> +        const __le16 set_bits, const __le16 clear_bits,
>> +        const bool skip_if_errors)
>>   {
>>       struct ntfs_inode *ni = NTFS_I(vol->vol_ino);
>>       struct volume_information *vi;
>>       struct ntfs_attr_search_ctx *ctx;
>> +    u16 flags;
>>       int err;
>>   -    ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
>> -            le16_to_cpu(vol->vol_flags), le16_to_cpu(flags));
>>       mutex_lock(&ni->mrec_lock);
>> -    if (vol->vol_flags == flags)
>> +
>> +    if (skip_if_errors && NVolErrors(vol))
>> +        goto done;
>> +
>> +    flags = le16_to_cpu(vol->vol_flags);
>> +    flags |= le16_to_cpu(set_bits) & le16_to_cpu(VOLUME_FLAGS_MASK);
>> +    flags &= ~(le16_to_cpu(clear_bits) & 
>> le16_to_cpu(VOLUME_FLAGS_MASK));
>> +    ntfs_debug("Entering, old flags = 0x%x, new flags = 0x%x.",
>> +            le16_to_cpu(vol->vol_flags), flags);
>> +
>> +    if (le16_to_cpu(vol->vol_flags) == flags)
>>           goto done;
>>         ctx = ntfs_attr_get_search_ctx(ni, NULL);
>> @@ -393,7 +407,7 @@ static int ntfs_write_volume_flags(struct 
>> ntfs_volume *vol, const __le16 flags)
>>         vi = (struct volume_information *)((u8 *)ctx->attr +
>> le16_to_cpu(ctx->attr->data.resident.value_offset));
>> -    vol->vol_flags = vi->flags = flags;
>> +    vol->vol_flags = vi->flags = cpu_to_le16(flags);
>>       mark_mft_record_dirty(ctx->ntfs_ino);
>>       ntfs_attr_put_search_ctx(ctx);
>>   done:
>> @@ -414,13 +428,14 @@ static int ntfs_write_volume_flags(struct 
>> ntfs_volume *vol, const __le16 flags)
>>    * @flags:    flags to set on the volume
>>    *
>>    * Set the bits in @flags in the volume information flags on the 
>> volume @vol.
>> + * The bits are combined with the current flag state under the lock in
>> + * ntfs_write_volume_flags(), so concurrent updates are not lost.
>>    *
>>    * Return 0 on success and -errno on error.
>>    */
>>   int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
>>   {
>> -    flags &= VOLUME_FLAGS_MASK;
>> -    return ntfs_write_volume_flags(vol, vol->vol_flags | flags);
>> +    return ntfs_write_volume_flags(vol, flags, 0, false);
>>   }
>>     /*
>> @@ -429,14 +444,27 @@ int ntfs_set_volume_flags(struct ntfs_volume 
>> *vol, __le16 flags)
>>    * @flags:    flags to clear on the volume
>>    *
>>    * Clear the bits in @flags in the volume information flags on the 
>> volume @vol.
>> + * The bits are combined with the current flag state under the lock in
>> + * ntfs_write_volume_flags(), so concurrent updates are not lost.
>>    *
>>    * Return 0 on success and -errno on error.
>>    */
>>   int ntfs_clear_volume_flags(struct ntfs_volume *vol, __le16 flags)
>>   {
>> -    flags &= VOLUME_FLAGS_MASK;
>> -    flags = vol->vol_flags & cpu_to_le16(~le16_to_cpu(flags));
>> -    return ntfs_write_volume_flags(vol, flags);
>> +    return ntfs_write_volume_flags(vol, 0, flags, false);
>> +}
>> +
>> +/*
>> + * ntfs_clear_volume_dirty_if_no_errors - clear dirty bit if no 
>> errors exist
>> + * @vol:    ntfs volume whose dirty bit should be cleared
>> + *
>> + * Check NVolErrors() and clear VOLUME_IS_DIRTY under the same 
>> mrec_lock so
>> + * ntfs_sync_fs() cannot clear the dirty bit after a concurrent 
>> error has been
>> + * recorded.
>> + */
>> +static int ntfs_clear_volume_dirty_if_no_errors(struct ntfs_volume 
>> *vol)
>> +{
>> +    return ntfs_write_volume_flags(vol, 0, VOLUME_IS_DIRTY, true);
>>   }
>>     int ntfs_write_volume_label(struct ntfs_volume *vol, char *label)
>> @@ -1862,7 +1890,7 @@ static int ntfs_sync_fs(struct super_block *sb, 
>> int wait)
>>           return 0;
>>         /* If there are some dirty buffers in the bdev inode */
>> -    if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY)) {
>> +    if (ntfs_clear_volume_dirty_if_no_errors(vol)) {
>>           ntfs_warning(sb, "Failed to clear dirty bit in volume 
>> information flags.  Run chkdsk.");
>>           err = -EIO;
>>       }
>
> Hi Hongling,
>   Patch 1 fixes the race in ntfs_sync_fs(), but there are two other 
> sites with the same check-outside-lock-then-clear-inside-lock pattern:
>
>   1. super.c:315 (ntfs_reconfigure, remount read-only path)
>   2. super.c:1755 (ntfs_put_super, umount path)
>
>   Both use:
>       if (!NVolErrors(vol)) {   // check outside lock
>           if (ntfs_clear_volume_flags(vol, VOLUME_IS_DIRTY))  // clear 
> inside lock
>
>   The same race can occur: if an error thread sets NVolErrors() and the
>   dirty bit after the check but before the lock acquisition, the clear
>   operation will overwrite the freshly-set dirty bit.
>
>   Although the race window is much narrower for remount/umount, should
>   these two sites also be converted to use
>   ntfs_clear_volume_dirty_if_no_errors() for consistency?
>
> Reviewed-by: Baolin Liu <liubaolin@kylinos.cn>
>
>
> Thanks,
> Baolin.
Thanks for the careful reading

These two sites cannot actually race ntfs_sync_fs(): ntfs_reconfigure() 
(via do_remount()) and ntfs_put_super() (via generic_shutdown_super()) 
both run with sb->s_umount
held for write, while the sync(2)/syncfs(2) paths only take it for read 
through iterate_supers(). So the check and the clear inside 
ntfs_clear_volume_flags() are
effectively a single non-interleavable pair with respect to sync — the 
window is not merely narrow but structurally closed.

We deliberately keep ntfs_clear_volume_dirty_if_no_errors() for the 
paths that can genuinely race sync (currently only ntfs_sync_fs() 
itself), so converting these two would
blur that line rather than improve consistency.

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

[liubaolin]

在 2026/9/3 14:37, Hongling Zeng 写道:
> Thanks for the careful reading
> 
> These two sites cannot actually race ntfs_sync_fs(): ntfs_reconfigure() 
> (via do_remount()) and ntfs_put_super() (via generic_shutdown_super()) 
> both run with sb->s_umount
> held for write, while the sync(2)/syncfs(2) paths only take it for read 
> through iterate_supers(). So the check and the clear inside 
> ntfs_clear_volume_flags() are
> effectively a single non-interleavable pair with respect to sync — the 
> window is not merely narrow but structurally closed.
> 
> We deliberately keep ntfs_clear_volume_dirty_if_no_errors() for the 
> paths that can genuinely race sync (currently only ntfs_sync_fs() 
> itself), so converting these two would
> blur that line rather than improve consistency.

Hi Hongling,

    Thank you for the explanation.
    However, I believe we may be discussing different races.
    Your explanation addresses why ntfs_reconfigure()/ntfs_put_super() 
cannot run concurrently with ntfs_sync_fs().
    My concern is whether these paths can race with an NTFS writer/error 
path while checking NVolErrors(vol) and clearing VOLUME_IS_DIRTY.

   For ntfs_put_super(), I agree that no change is needed.
   It runs only during the final filesystem shutdown, when ordinary 
userspace writers and related I/O can no longer run concurrently with it.

   For a normal read-only remount, reconfigure_super() calls 
sb_prepare_remount_readonly(). This blocks new mount writers and checks 
for existing writers; if writers are present, the remount does not 
proceed. Therefore, the normal remount-ro path does not have the 
concurrency issue I was concerned about.

   However, an emergency/forced remount uses SB_FORCE and skips 
sb_prepare_remount_readonly(), calling only sb_start_ro_state_change(). 
This does not wait for writers that have already entered the filesystem. 
Such writers use sb->s_writers and do not need to acquire s_umount.
The following sequence is therefore possible:
NTFS writer                         forced remount
   -----------                         -------------
   file_start_write()
   Enter the NTFS write path
   Set or prepare to set VOLUME_IS_DIRTY
   Pause
                                       Acquire s_umount for writing
                                       SB_FORCE
                                       sb_start_ro_state_change()
                                       ntfs_reconfigure()
                                         sync_filesystem()
                                         NVolErrors() == false
                                         Clear VOLUME_IS_DIRTY
                                       Set SB_RDONLY
   Resume the writer
   Continue modifying metadata
   An error occurs
   NVolSetErrors(vol)

The final state may be:
   NVolErrors(vol) == true
   VOLUME_IS_DIRTY == false

   Therefore, the s_umount locking relationship only rules out 
concurrency with ntfs_sync_fs(); it does not rule out concurrency 
between a forced/emergency remount and an NTFS writer that is already in 
progress.

   Please consider whether this case also needs to be handled. If you 
agree with my analysis, I suggest adding a follow-up patch to address 
this race.
   This is my current understanding of the issue. I welcome further 
discussion, and please feel free to correct me if any part of my 
analysis is inaccurate.

Thanks,
Baolin.