Re: [PATCH v5] ntfs: mount hibernated volumes read-only regardless of errors=

[liubaolin <liubaolin12138@163.com>]

在 2026/9/20 10:59, Hongling Zeng 写道:
> The hibernation check in load_system_files() only converts the
> superblock to read-only under errors=remount-ro.  With the default
> errors=continue (and with errors=panic), a hibernated volume is
> mounted read-write and the mount-time $LogFile emptying writes to it,
> although a hibernated volume must not be written to at all.
> 
> Drop the on_errors term so that a hibernated volume, or a volume whose
> hibernation state cannot be determined, always mounts read-only.
> NVolErrors() is still recorded, so ntfs_reconfigure() keeps refusing
> remounts to read-write, and the $LogFile emptying is skipped by its
> !sb_rdonly() check.
> 
> The check itself must not apply the errors= policy either: it runs
> before SB_RDONLY is set, and ntfs_lookup_inode_by_name() and
> ntfs_iget() call ntfs_error() internally on corruption or I/O errors,
> which under errors=panic would panic the machine before the read-only
> fallback has made its decision.  Run the check with the super block
> temporarily marked read-only, which makes ntfs_handle_error() ignore
> any ntfs_error() issued on this path; the super block is not
> published yet at this point, so the temporary flag is not visible
> elsewhere.
> 
> Restore read-write access only if the check succeeded and no errors
> were recorded earlier during the mount, e.g. when loading the LogFile,
> so that a volume with recorded errors stays read-only, matching what
> ntfs_reconfigure() enforces for remounts, and a message is logged for
> that case.
> 
> Fixes: 6251f0b0de7d ("ntfs: update super block operations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> Change in v5:
>   -Rework the errors=panic handling per review feedback.
> ---
>   fs/ntfs/super.c | 36 +++++++++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index f4a73e45773d..ab91cd1a8515 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c
> @@ -1398,6 +1398,7 @@ static bool load_system_files(struct ntfs_volume *vol)
>   	struct ntfs_attr_search_ctx *ctx;
>   	struct restart_page_header *rp;
>   	int err;
> +	bool temporary_ro = false;
>   
>   	ntfs_debug("Entering.");
>   	/* Get mft mirror inode compare the contents of $MFT and $MFTMirr. */
> @@ -1572,8 +1573,20 @@ static bool load_system_files(struct ntfs_volume *vol)
>   	 * NVolErrors() without setting the dirty volume flag and mount
>   	 * read-only.  This will prevent read-write remounting and it will also
>   	 * prevent all writes.
> +	 *
> +	 * The check runs with the super block temporarily marked read-only, so
> +	 * that ntfs_error() calls issued internally by ntfs_lookup_inode_by_name()
> +	 * and ntfs_iget() cannot trigger errors=panic before the read-only
> +	 * fallback has run.  The super block is not published yet, so the flag
> +	 * is not visible elsewhere.
>   	 */
> +	if (!sb_rdonly(sb)) {
> +		sb->s_flags |= SB_RDONLY;
> +		temporary_ro = true;
> +	}
>   	err = check_windows_hibernation_status(vol);
> +	if (temporary_ro && !err && !NVolErrors(vol))
> +		sb->s_flags &= ~SB_RDONLY;

Hi Hongling,
   Thanks for sending the updated patch. I'm sorry, but while reviewing 
this version more closely, I found a problem with the temporary 
read-only approach I suggested during the v3 discussion.

   The condition above does not reliably establish whether read-write 
access can be restored. An ntfs_error() call does not necessarily set 
NVolErrors(), and some errors reported by nested helpers are not 
propagated back to the hibernation check.

   For example, ntfs_attr_readall() can report "Invalid attribute data 
size" and return -EIO without setting NVolErrors(). If this happens 
while reading the optional WSL extended attributes through 
ntfs_ea_get_wsl_inode(), the error can be ignored and inode loading can 
still succeed. If the hibernation header is otherwise valid and 
zero-filled, the check can then return zero.

   With errors=remount-ro, the original code would retain the read-only 
state set by that ntfs_error() call. With v5, ntfs_handle_error() 
returns immediately because SB_RDONLY is already set, and the condition 
above can subsequently restore read-write access because neither err nor 
NVolErrors() reflects that error.

   This makes it difficult to define a reliable restoration condition 
using only the check's return value and NVolErrors().

   I now think a better approach is to temporarily replace 
ON_ERRORS_PANIC with ON_ERRORS_REMOUNT_RO around 
check_windows_hibernation_status(), then restore the original policy
  immediately afterwards. This covers both direct and nested 
ntfs_error() calls while preserving any read-only transition they 
trigger. The temporary substitution leaves the existing errors=continue 
and errors=remount-ro policies unchanged.

   This deliberately falls back to read-only for any ntfs_error() that 
would otherwise trigger panic during the check, including recoverable 
errors. I think that is a reasonable conservative fallback: it avoids 
panic without treating every such error as a persistent volume error by 
automatically setting NVolErrors().

   If the check reports hibernation or fails, we would still 
unconditionally set SB_RDONLY and NVolErrors(). We would also retain 
your protection for volumes with errors recorded during the check or 
earlier in the mount.


   The following is a replacement patch against the same base as your v5:

   @@ -1398,6 +1398,7 @@ static bool load_system_files(struct 
ntfs_volume *vol)
    	struct ntfs_attr_search_ctx *ctx;
    	struct restart_page_header *rp;
    	int err;
   +	u8 saved_on_errors;

    	ntfs_debug("Entering.");
    	/* Get mft mirror inode compare the contents of $MFT and $MFTMirr. */
   @@ -1572,8 +1573,18 @@ static bool load_system_files(struct 
ntfs_volume *vol)
    	 * NVolErrors() without setting the dirty volume flag and mount
    	 * read-only.  This will prevent read-write remounting and it will also
    	 * prevent all writes.
   +	 *
   +	 * Nested lookup and inode-loading errors must not panic before the
   +	 * read-only fallback.  Temporarily use errors=remount-ro instead of
   +	 * errors=panic, preserving any read-only transition even if an error
   +	 * is not propagated or recorded in NVolErrors().  This is safe during
   +	 * initial mount, before the super block is published.
    	 */
   +	saved_on_errors = vol->on_errors;
   +	if (saved_on_errors == ON_ERRORS_PANIC)
   +		vol->on_errors = ON_ERRORS_REMOUNT_RO;
    	err = check_windows_hibernation_status(vol);
   +	vol->on_errors = saved_on_errors;
    	if (unlikely(err)) {
    		static const char *es1a = "Failed to determine if Windows is 
hibernated";
    		static const char *es1b = "Windows is hibernated";
   @@ -1581,12 +1592,15 @@ static bool load_system_files(struct 
ntfs_volume *vol)
    		const char *es1;

    		es1 = err < 0 ? es1a : es1b;
   -		/* If a read-write mount, convert it to a read-only mount. */
   -		if (!sb_rdonly(sb) && vol->on_errors == ON_ERRORS_REMOUNT_RO) {
   -			sb->s_flags |= SB_RDONLY;
   -			ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
   -		}
   +		/* Hibernation safety takes precedence over the errors= policy. */
   +		sb->s_flags |= SB_RDONLY;
    		NVolSetErrors(vol);
   +		ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
   +	} else if (!sb_rdonly(sb) && NVolErrors(vol)) {
   +		/* Match the read-write remount restriction for recorded errors. */
   +		sb->s_flags |= SB_RDONLY;
   +		ntfs_error(sb,
   +			   "Errors were recorded during mount.  Mounting read-only.  Run 
chkdsk.");
    	}

    	/* If (still) a read-write mount, empty the logfile. */


   Sorry again for overlooking this issue in my earlier suggestion. I 
believe this approach is more robust because it never clears SB_RDONLY, 
so it does not need to reconstruct whether a nested error should have 
made the volume read-only. The policy change is limited to the 
hibernation check during initial mount, before the superblock is published.

   If you agree with this approach, could you please incorporate it and 
send another revision? I'd appreciate feedback from you, Namjae, and 
Hyunchul.

feel free to add:
Suggested-by: Baolin Liu <liubaolin@kylinos.cn>

Thanks,
Baolin.







>   	if (unlikely(err)) {
>   		static const char *es1a = "Failed to determine if Windows is hibernated";
>   		static const char *es1b = "Windows is hibernated";
> @@ -1581,12 +1594,25 @@ static bool load_system_files(struct ntfs_volume *vol)
>   		const char *es1;
>   
>   		es1 = err < 0 ? es1a : es1b;
> -		/* If a read-write mount, convert it to a read-only mount. */
> -		if (!sb_rdonly(sb) && vol->on_errors == ON_ERRORS_REMOUNT_RO) {
> -			sb->s_flags |= SB_RDONLY;
> -			ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
> -		}
> +		/*
> +		 * A Windows hibernation image is not a filesystem error, so
> +		 * this is a safety interlock rather than something the
> +		 * errors= policy may downgrade.  The super block is already
> +		 * read-only here: the temporary flag taken for the check
> +		 * above is not restored when the check failed.
> +		 */
> +		ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
>   		NVolSetErrors(vol);
> +	} else if (unlikely(temporary_ro && sb_rdonly(sb))) {
> +		static const char *es1 = "Errors were recorded during mount";
> +		static const char *es2 = ".  Run chkdsk.";
> +
> +		/*
> +		 * Errors were recorded during the check or earlier, e.g. when
> +		 * loading the LogFile.  Stay read-only, like ntfs_reconfigure()
> +		 * does for volumes with recorded errors.
> +		 */
> +		ntfs_error(sb, "%s.  Mounting read-only%s", es1, es2);
>   	}
>   
>   	/* If (still) a read-write mount, empty the logfile. */