Re: [PATCH v2 11/34] hfs: don't set SB_RDONLY after MDB write errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hfs: hfs_file_truncate() early return leaves in-memory inode sizes inconsistent
--

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

hfs: don't set SB_RDONLY after MDB write errors

Instead of setting SB_RDONLY without holding s_umount when an MDB write fails,
this commit records the error in a new HFS_FLG_EMERGENCY_RO flag and rejects
subsequent filesystem modifications with EROFS.

> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
> index 26357086517fc..dacf7ce1ffe4a 100644
> --- a/fs/hfs/extent.c
> +++ b/fs/hfs/extent.c

[ ... ]

> @@ -491,6 +494,8 @@ void hfs_file_truncate(struct inode *inode)
>  	hfs_dbg("ino %llu, phys_size %llu -> i_size %llu\n",
>  		inode->i_ino, (long long)HFS_I(inode)->phys_size,
>  		inode->i_size);
> +	if (unlikely(hfs_emergency_ro(sb)))
> +		return;
>  	if (inode->i_size > HFS_I(inode)->phys_size) {
>  		struct address_space *mapping = inode->i_mapping;
>  		void *fsdata = NULL;

[Severity: Medium]
Can returning at the top of hfs_file_truncate() in fs/hfs/extent.c leave
in-memory inode size metadata inconsistent when HFS_FLG_EMERGENCY_RO (or
SB_STATE_EMERGENCY_RO in the follow-up commit "hfs: use the generic
emergency read-only state") is set concurrently by the mdb_work worker or
sync_fs()?

First, in the expanding truncate path via hfs_inode_setattr() in
fs/hfs/inode.c (or notify_change() in the follow-up commit):

    if ((attr->ia_valid & ATTR_SIZE) &&
        attr->ia_size != i_size_read(inode)) {
            inode_dio_wait(inode);

            error = inode_newsize_ok(inode, attr->ia_size);
            if (error)
                    return error;

            truncate_setsize(inode, attr->ia_size);
            hfs_file_truncate(inode);

If HFS_FLG_EMERGENCY_RO is set after the initial read-only check (for
example, while waiting in inode_dio_wait()), truncate_setsize() updates
inode->i_size and then hfs_file_truncate() returns early before reaching
the expansion block:

    if (inode->i_size > HFS_I(inode)->phys_size) {
            ...
            res = hfs_write_begin(NULL, mapping, size + 1, 0, &folio,
                            &fsdata);
            ...
            if (res)
                    inode->i_size = HFS_I(inode)->phys_size;
            return;
    }

Without the early return, hfs_write_begin() fails with -EROFS and reverts
inode->i_size back to HFS_I(inode)->phys_size. With the early return,
hfs_inode_setattr() succeeds with inode->i_size larger than
HFS_I(inode)->phys_size and HFS_I(inode)->fs_blocks, causing subsequent
buffered reads via hfs_read_folio() -> block_read_full_folio() ->
hfs_get_block() to see unallocated blocks and return zero-filled pages up
to the new inode->i_size.

Second, when an extending write in hfs_write_begin() or hfs_direct_IO()
allocates some blocks via hfs_get_block() (incrementing
HFS_I(inode)->phys_size, HFS_I(inode)->fs_blocks, and inode->i_bytes) and
then hits -EROFS on a subsequent block after HFS_FLG_EMERGENCY_RO is set,
error cleanup calls hfs_write_failed():

    static void hfs_write_failed(struct address_space *mapping, loff_t to)
    {
            struct inode *inode = mapping->host;

            if (to > inode->i_size) {
                    truncate_pagecache(inode, inode->i_size);
                    hfs_file_truncate(inode);
            }
    }

Returning early at the top of hfs_file_truncate() skips the out: label
adjustments as well, leaving HFS_I(inode)->phys_size,
HFS_I(inode)->fs_blocks, and the inode byte count inflated past
inode->i_size.

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