Re: [PATCH v3 09/33] fat: don't set SB_RDONLY after filesystem errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] fat: fat_free() zeroes i_start on disk when
  fat_free_clusters() returns -EROFS
- [Medium] fat: directory modifications proceed after fat__get_entry()
  sets emergency RO
--

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

fat: don't set SB_RDONLY after filesystem errors

This commit switches __fat_fs_error() from setting SB_RDONLY on a mounted
superblock to calling sb_set_emergency_ro(), and adds sb_emergency_ro()
checks across FAT write, allocation, and state paths.

> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index 35bdb62944a2e..553034bcbed40 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -1329,6 +1329,9 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
>  	loff_t pos;
>  	bool saw_eod;
>  
> +	if (unlikely(sb_emergency_ro(sb)))
> +		return -EROFS;
> +
>  	sinfo->nr_slots = nr_slots;
>  
>  	/* First stage: search free directory entries */

[Severity: Medium]
Can fat_add_entries() and fat_dir_empty() still modify directory buffers
after a FAT error in fat__get_entry() sets SB_STATE_EMERGENCY_RO?

In fat__get_entry(), a fat_bmap() error calls fat_fs_error(), sets
sb_emergency_ro(sb), and returns -1 just like normal EOF.  In
fat_add_entries(), if the fat_get_entry() loop hits a FAT error on a later
cluster after collecting some free slots (0 < free_slots < nr_slots), it
falls through to found:, writes the partial slots into bhs[], fails in
fat_add_new_entries() -> fat_alloc_clusters() with -EROFS, and then
overwrites the original slots (including 0x00 end-of-directory markers)
with DELETED_FLAG in __fat_remove_entries().

Similarly, in fat_dir_empty(), a -1 return from fat_get_short_entry() after
a FAT error makes a corrupted non-empty directory appear empty (return 0),
and because neither fat_dir_empty() nor fat_remove_entries() checks
sb_emergency_ro(sb), rmdir and rename proceed to delete the directory entry
on disk after the filesystem has entered emergency read-only mode.

> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index 2d10a815a5797..9e4bb0140320f 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c

[ ... ]

> @@ -620,6 +626,9 @@ int fat_free_clusters(struct inode *inode, int cluster)
>  	int i, err, nr_bhs;
>  	int first_cl = cluster, dirty_fsinfo = 0;
>  
> +	if (unlikely(sb_emergency_ro(sb)))
> +		return -EROFS;
> +
>  	nr_bhs = 0;
>  	fatent_init(&fatent);
>  	lock_fat(sbi);

[Severity: Medium]
Does returning -EROFS from fat_free_clusters() without guarding fat_free()
cause fat_free() to zero i_start on disk while leaving the cluster chain
allocated in the FAT?

When cont_write_begin() fails due to a FAT error that sets
sb_emergency_ro(sb) (for example on a 0-byte file with fallocated clusters,
or after allocating the first cluster of a multi-cluster write),
fat_write_begin() calls fat_write_failed() -> fat_truncate_blocks(inode, 0)
-> fat_free(inode, 0):

fs/fat/file.c:fat_free() {
    ...
	if (!skip) {
		MSDOS_I(inode)->i_start = 0;
		MSDOS_I(inode)->i_logstart = 0;
	}
	MSDOS_I(inode)->i_attrs |= ATTR_ARCH;
	fat_truncate_time(inode, NULL, FAT_UPDATE_CMTIME);
	mark_inode_dirty(inode);
    ...
	inode->i_blocks = skip << (MSDOS_SB(sb)->cluster_bits - 9);

	/* Freeing the remained cluster chain */
	return fat_free_clusters(inode, free_start);
}

Because fat_free() clears i_start and marks the inode dirty before
fat_free_clusters() returns -EROFS, and fat_write_inode() stays active in
emergency read-only mode, writeback writes start = 0 to the directory entry
on disk and orphans the allocated clusters in the FAT.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-work-fs-sb_rdonly-v3-0-0be92bb73709@kernel.org?part=9