Re: [PATCH v2 07/34] fat: 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] fat: fat_write_inode() refusal on emergency ro corrupts pending dir entries
--

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

fat: don't set SB_RDONLY after filesystem errors

This commit replaces setting SB_RDONLY in __fat_fs_error() after SB_BORN with
sb_set_emergency_ro() so that thaw_super() does not leave s_writers locked if
an error occurs while the filesystem is frozen. It also adds
sb_emergency_ro() checks across FAT operations to reject further
modifications.

> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index d85c8fbdcca46..afd73dc4ba9a8 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c

[ ... ]

> @@ -954,6 +963,9 @@ static int fat_write_inode(struct inode *inode, struct writeback_control *wbc)
>  {
>  	int err;
>  
> +	if (unlikely(sb_emergency_ro(inode->i_sb)))
> +		return -EROFS;
> +
>  	if (inode->i_ino == MSDOS_FSINFO_INO) {
>  		struct super_block *sb = inode->i_sb;
>  
> @@ -1949,6 +1961,9 @@ int fat_flush_inodes(struct super_block *sb, struct inode *i1, struct inode *i2)
>  		ret = writeback_inode(i2);
>  	if (!ret)
>  		ret = sync_blockdev_nowait(sb->s_bdev);
> +	/* The change is done, an error made us read-only in the meantime. */
> +	if (ret == -EROFS && sb_emergency_ro(sb))
> +		ret = 0;
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(fat_flush_inodes);

[Severity: High]
Can rejecting fat_write_inode() when sb_emergency_ro() is set corrupt
on-disk directory entries when dirty block device buffer_heads are still
flushed to disk?

In FAT, an inode's starting cluster (i_logstart) and size (i_size) are stored
inside its msdos_dir_entry in sb->s_bdev buffer_heads and are only written to
that buffer_head by fat_write_inode() -> __fat_write_inode():

    raw_entry = &((struct msdos_dir_entry *) (bh->b_data))[offset];
    if (S_ISDIR(inode->i_mode))
        raw_entry->size = 0;
    else
        raw_entry->size = cpu_to_le32(inode->i_size);
    raw_entry->attr = fat_make_attrs(inode);
    fat_set_start(raw_entry, MSDOS_I(inode)->i_logstart);

During vfat_rename() and do_msdos_rename(), vfat_add_entry() or
msdos_add_entry() creates the target directory entry in new_dir's buffer_head
with cluster = 0 and size = 0 and marks that buffer_head dirty:

    err = msdos_add_entry(new_dir, new_name, is_dir, is_hid, 0,
                  &ts, &sinfo);
    if (err)
        goto out;
    new_i_pos = sinfo.i_pos;
    ...
    fat_detach(old_inode);
    fat_attach(old_inode, new_i_pos);
    ...
    mark_inode_dirty(old_inode);
    ...
    err = fat_remove_entries(old_dir, &old_sinfo);

Similarly, when a newly created file allocates its first cluster in
fat_chain_add() and updates i_size in fat_write_end(), the FAT and directory
buffer_heads are dirtied in sb->s_bdev while the directory entry still has
start = 0 and size = 0 until __fat_write_inode() updates it.

If fat_fs_error() sets sb_emergency_ro(sb) either during the operation (for
example, fat_add_entries() hitting "Odd directory size" or fat_chain_add()
hitting "clusters badly computed", both of which call fat_fs_error() and
still return 0) or before background inode writeback runs, fat_write_inode()
will now return -EROFS without updating the directory entry's start cluster
and size.

However, the dirty directory and FAT buffer_heads in sb->s_bdev are still
written out by bdev writeback, mmb_sync() in fat_evict_inode(), and
sync_blockdev() on unmount. Wouldn't that flush the deleted old directory
entry along with a new directory entry containing start = 0 and size = 0,
losing the file or directory contents and orphaning its cluster chain?

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