Re: [PATCH v2 09/34] exfat: 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] exfat: __exfat_truncate() and exfat_evict_inode() modify fs in emergency RO
--

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

exfat: don't set SB_RDONLY after filesystem errors

This commit introduces EXFAT_FLAGS_EMERGENCY_RO and exfat_emergency_state() so
that filesystem errors after mount mark the filesystem emergency read-only
rather than setting SB_RDONLY without holding s_umount.

> diff --git a/fs/exfat/inode.c b/fs/exfat/inode.c
> index 5129df1e79b44..3f24e5bd3eb11 100644
> --- a/fs/exfat/inode.c
> +++ b/fs/exfat/inode.c
> @@ -114,8 +114,9 @@ int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
>  {
>  	int ret;
>  
> -	if (unlikely(exfat_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	ret = exfat_emergency_state(inode->i_sb);
> +	if (unlikely(ret))
> +		return ret;
>  
>  	mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
>  	ret = __exfat_write_inode(inode, wbc->sync_mode == WB_SYNC_ALL);

[Severity: High]
Should __exfat_truncate() or exfat_evict_inode() also check
exfat_emergency_state()?

When an unlinked file is closed (or during unmount) after a filesystem error
sets EXFAT_FLAGS_EMERGENCY_RO (or SB_STATE_EMERGENCY_RO in the follow-up
commit "exfat: use the generic emergency read-only state"),
exfat_evict_inode() calls __exfat_truncate():

fs/exfat/inode.c:exfat_evict_inode() {
	truncate_inode_pages_final(&inode->i_data);

	if (!inode->i_nlink) {
		i_size_write(inode, 0);
		mutex_lock(&EXFAT_SB(inode->i_sb)->s_lock);
		__exfat_truncate(inode);
		mutex_unlock(&EXFAT_SB(inode->i_sb)->s_lock);
	}
    ...
}

Similarly, if a write in exfat_file_write_iter() encounters a corrupted FAT
chain or cluster error that sets EXFAT_FLAGS_EMERGENCY_RO via
exfat_fs_error(), the error unwind in exfat_extend_valid_size() or
exfat_write_failed() calls exfat_truncate() -> __exfat_truncate().

Because __exfat_truncate() in fs/exfat/file.c calls __exfat_write_inode()
directly rather than exfat_write_inode(), it bypasses the
exfat_emergency_state() check above:

fs/exfat/file.c:__exfat_truncate() {
    ...
	if (__exfat_write_inode(inode, inode_needs_sync(inode)))
		return -EIO;

	/* cut off from the FAT chain */
	if (ei->flags == ALLOC_FAT_CHAIN && last_clu != EXFAT_FREE_CLUSTER &&
			last_clu != EXFAT_EOF_CLUSTER) {
		if (exfat_ent_set(sb, last_clu, EXFAT_EOF_CLUSTER))
			return -EIO;
	}
    ...
	/* free the clusters */
	if (exfat_free_cluster(inode, &clu))
		return -EIO;
    ...
}

This modifies directory entries, FAT entries, and allocation bitmap buffers via
exfat_free_cluster() -> __exfat_free_cluster() -> exfat_clear_bitmap() (which
also writes synchronously when IS_DIRSYNC(inode) is set), and if mounted with
discard (which __exfat_fs_error() does not clear, unlike
exfat_force_shutdown()), issues block device discard requests via
exfat_discard_cluster() while walking a potentially corrupted cluster chain.
Furthermore, because SB_RDONLY is no longer set on error, sync_filesystem() no
longer returns early at sb_rdonly(sb) and flushes those dirtied metadata
buffers to disk.

Can __exfat_truncate() and/or exfat_evict_inode() check
exfat_emergency_state() to prevent metadata modifications and discards after
the filesystem enters emergency read-only state?

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/