Re: [PATCH v2 17/34] ntfs: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ntfs: missing ntfs_emergency_state() check in ntfs_drop_big_inode()
- [Medium] ntfs: ntfs_trim_prealloc() mutates MFT records in emergency RO state
--

commit f2f6f70fa52c8d2d01384c57dde6822ea706a5e3
Author: Christian Brauner <brauner@kernel.org>

ntfs: don't set SB_RDONLY after filesystem errors

This commit replaces setting SB_RDONLY on filesystem errors when mounted with
errors=remount-ro with a new NV_EmergencyRO volume flag and an
ntfs_emergency_state() helper so that freezing and thawing the superblock
does not leave s_writers locked. It also adds an error_work work item to
persist the dirty volume flag and updates write paths to check
ntfs_emergency_state().

> diff --git a/fs/ntfs/file.c b/fs/ntfs/file.c
> index b2f5882d9990d..fbfbd94c60924 100644
> --- a/fs/ntfs/file.c
> +++ b/fs/ntfs/file.c
> @@ -55,9 +55,14 @@
>  int ntfs_file_open(struct inode *vi, struct file *filp)
>  {
>  	struct ntfs_inode *ni = NTFS_I(vi);
> +	int err;
>  
> -	if (NVolShutdown(ni->vol))
> -		return -EIO;
> +	if (filp->f_mode & FMODE_WRITE)
> +		err = ntfs_emergency_state(ni->vol);
> +	else
> +		err = NVolShutdown(ni->vol) ? -EIO : 0;
> +	if (err)
> +		return err;

[Severity: Medium]
Should ntfs_trim_prealloc() (called from ntfs_file_release() and
ntfs_stream_file_release()) also check ntfs_emergency_state(vol) before
rolling back preallocated blocks?

When small writes in ntfs_file_write_iter() -> ntfs_expand_for_write() leave
ni->allocated_size larger than ni->data_size using trailing LCN_HOLE runlist
entries, and the volume subsequently enters NV_EmergencyRO or NV_Shutdown,
closing the open file descriptor invokes:

  ntfs_file_release()
    ntfs_trim_prealloc()
      ntfs_rl_truncate_nolock()
      ntfs_attr_update_mapping_pairs_locked()

Because neither function checks ntfs_emergency_state(vol), closing the file
mutates the runlist and MFT record mapping pairs and dirties the MFT record
and VFS inode even though __ntfs_write_inode() and ntfs_mft_writepages() now
refuse writeback with -EROFS or -EIO.

If the trailing hole run spanned an extent MFT record,
__ntfs_attr_update_mapping_pairs() -> ntfs_attr_record_rm() removes the
attribute and $ATTRIBUTE_LIST entry and then calls ntfs_mft_record_free(),
which now fails with -EROFS due to the new ntfs_emergency_state(vol) check,
leaving the extent partially removed and triggering secondary ntfs_error()
messages.

[ ... ]

> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index 206db4e99ef13..6219aed945dfc 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -2434,7 +2434,11 @@ void ntfs_evict_big_inode(struct inode *vi)
>  
>  	truncate_inode_pages_final(&vi->i_data);
>  
> -	if (!vi->i_nlink) {
> +	/*
> +	 * After an error the freed mft record would go out right away while
> +	 * the rest of the deletion can't, leave the inode to chkdsk.
> +	 */
> +	if (!vi->i_nlink && !ntfs_emergency_state(ni->vol)) {
>  		if (!NInoAttr(ni)) {
>  			/* Never called with extent inodes */
>  			WARN_ON(ni->nr_extents == -1);

[Severity: Medium]
Does ntfs_drop_big_inode() need the same !ntfs_emergency_state(ni->vol) guard
when truncating unlinked inodes with I_SYNC set?

In fs/ntfs/inode.c:ntfs_drop_big_inode():

	if (!inode_unhashed(inode) && inode_state_read_once(inode) & I_SYNC) {
		if (ni->type == AT_DATA || ni->type == AT_INDEX_ALLOCATION) {
			if (!inode->i_nlink) {
				...
				truncate_setsize(VFS_I(ni), 0);
				ntfs_truncate_vfs(VFS_I(ni), 0, 1);

If an unlinked inode is undergoing writeback with I_SYNC set in
ntfs_writepages() or __ntfs_write_inode() when an error triggers
ntfs_handle_error() and sets NV_EmergencyRO (or in commit "ntfs: use the
generic emergency read-only state", SB_STATE_EMERGENCY_RO), dropping the last
inode reference via iput() while I_SYNC is still held enters
ntfs_drop_big_inode().

Without checking !ntfs_emergency_state(ni->vol), ntfs_drop_big_inode() still
calls ntfs_truncate_vfs() -> __ntfs_attr_truncate_vfs() ->
ntfs_non_resident_attr_shrink() -> ntfs_cluster_free(), which frees clusters
in $Bitmap, fails partway through extent removal when ntfs_mft_record_free()
returns -EROFS, and on mounts with NVolDiscard issues blkdev_issue_discard()
on the file's clusters even though the on-disk MFT record and bitmap updates
cannot be written back for chkdsk.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-work-fs-sb_rdonly-v2-0-2f44f64f0b66@kernel.org?part=17