Re: [PATCH RFC 14/28] ntfs: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vfs: ABBA deadlock between umount and thaw_super on frozen NTFS
--

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

ntfs: don't set SB_RDONLY after filesystem errors

This commit updates NTFS to handle errors by setting NV_EmergencyRO instead
of SB_RDONLY to avoid leaving s_writers locked after thawing. It introduces
an asynchronous work item to mark the volume dirty on disk and flushes/syncs
this work during unmount.

> diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
> index 4066bacabe377..12ad2ce938ba9 100644
> --- a/fs/ntfs/super.c
> +++ b/fs/ntfs/super.c

[ ... ]

> @@ -423,6 +446,29 @@ int ntfs_set_volume_flags(struct ntfs_volume *vol, __le16 flags)
>  	return ntfs_write_volume_flags(vol, vol->vol_flags | flags);
>  }
>  
> +/*
> + * ntfs_error_work - mark the volume dirty on disk after an error
> + * @work:	the error_work of the ntfs volume
> + *
> + * VOLUME_IS_DIRTY makes chkdsk check the volume. It is set lazily on the first
> + * modification and ntfs_sync_fs() clears it again, so it may not be on disk
> + * when an error makes the volume read-only, and the inode writeback that would
> + * write it is refused from then on. __ntfs_write_inode() still writes $Volume
> + * in that state. This runs from a work item because ntfs_error() may be called
> + * with mft records locked. sb_start_intwrite() waits for a frozen volume to be
> + * thawed.
> + */
> +static void ntfs_error_work(struct work_struct *work)
> +{
> +	struct ntfs_volume *vol = container_of(work, struct ntfs_volume,
> +					       error_work);
> +
> +	sb_start_intwrite(vol->sb);

[Severity: High]
Could this cause an ABBA deadlock if an error occurs while the filesystem
is frozen? 

If the filesystem is frozen via freeze_super(), the VFS holds a write lock
on sb->s_writers.rw_sem. The worker executing ntfs_error_work() blocks here
indefinitely waiting for a thaw operation to release that lock.

> +	if (!ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY))
> +		ntfs_commit_inode(vol->vol_ino);
> +	sb_end_intwrite(vol->sb);
> +}

[ ... ]

> @@ -1716,6 +1762,15 @@ static void ntfs_put_super(struct super_block *sb)
>  
>  	cancel_work_sync(&vol->precalc_work);
>  
> +	/*
> +	 * If an error made the volume read-only, make sure it is marked dirty
> +	 * even if the error work didn't get to run. The commit of $Volume
> +	 * below writes it.
> +	 */
> +	disable_work_sync(&vol->error_work);

[Severity: High]
Does this sequence lead to an unresolvable 3-way deadlock between the error 
worker, umount, and thaw_super()?

If the error worker is blocked on sb_start_intwrite() as noted above, and
the user attempts to unmount the filesystem, deactivate_super() acquires
down_write(&sb->s_umount) before calling ntfs_put_super().

The disable_work_sync() call here will block forever waiting for the worker
to finish, keeping sb->s_umount held by the unmount process. 

To recover the filesystem, the user must call thaw_super(), but that
requires acquiring down_write(&sb->s_umount), which is now stuck behind
the blocked unmount.

> +	if (NVolEmergencyRO(vol))
> +		ntfs_set_volume_flags(vol, VOLUME_IS_DIRTY);
> +
>  	/*
>  	 * Commit all inodes while they are still open in case some of them
>  	 * cause others to be dirtied.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-work-fs-sb_rdonly-v1-0-60743a3dc326@kernel.org?part=14