Re: [PATCH RFC 09/28] hfs: don't set SB_RDONLY after MDB write errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hfs: mmap writes bypass emergency read-only enforcement
- [High] hfs: b-tree corruption on mount due to mismatched MDB and b-tree header
--

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

    hfs: don't set SB_RDONLY after MDB write errors
    
    Since commit fdbb95ff89b2 ("hfs: don't re-dirty MDB buffers after a
    write failure") hfs_mdb_commit() sets SB_RDONLY when the MDB or the
    alternate MDB buffer lost BH_Uptodate after a failed write.

[ ... ]

> diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
> index f066a99a863bc..dab31f159b1aa 100644
> --- a/fs/hfs/extent.c
> +++ b/fs/hfs/extent.c
> @@ -350,6 +350,9 @@ int hfs_get_block(struct inode *inode, sector_t block,
>  		if (block > HFS_I(inode)->fs_blocks)
>  			return -EIO;
>  		if (ablock >= HFS_I(inode)->alloc_blocks) {
> +			/* No new allocations after an error. */
> +			if (unlikely(hfs_emergency_ro(sb)))
> +				return -EROFS;
>  			res = hfs_extend_file(inode);
>  			if (res)
>  				return res;

[Severity: High]
Does this code allow data modification on a filesystem in an emergency
read-only state?

If a user memory-maps a file with PROT_WRITE and modifies the pages after the
filesystem enters the emergency read-only state, the page fault succeeds
because hfs does not override page_mkwrite to check hfs_emergency_ro().

When the VM triggers writeback, hfs_writepages() calls mpage_writepages(),
which calls hfs_get_block(). For existing file blocks where
ablock < HFS_I(inode)->alloc_blocks, hfs_get_block() bypasses this new check,
maps the block, and successfully writes dirty pages to disk, violating the
read-only guarantee.

[ ... ]

> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index 2aef3c36a150a..42a2c2e390b23 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -472,6 +479,20 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  	int res;
>  
>  	hfs_dbg("ino %llu\n", inode->i_ino);
> +	if (unlikely(hfs_emergency_ro(inode->i_sb))) {
> +		/*
> +		 * Writeback still writes the b-tree nodes. Keep the headers in
> +		 * sync with them so the trees on disk stay intact.
> +		 */
> +		if (inode->i_ino == HFS_EXT_CNID)
> +			hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
> +		else if (inode->i_ino == HFS_CAT_CNID)
> +			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
> +		else
> +			return -EROFS;
> +		return 0;
> +	}
> +

[ ... ]

> diff --git a/fs/hfs/mdb.c b/fs/hfs/mdb.c
> index 277de712f9d47..ce20824bbdc84 100644
> --- a/fs/hfs/mdb.c
> +++ b/fs/hfs/mdb.c
> @@ -338,12 +338,18 @@ int hfs_mdb_commit(struct super_block *sb)
>  	struct hfs_mdb *mdb = HFS_SB(sb)->mdb;
>  	int ret = 0;
>  
> -	if (sb_rdonly(sb))
> +	if (sb_rdonly(sb) || hfs_emergency_ro(sb))
>  		return 0;

[Severity: High]
Can writing the updated b-tree header in hfs_write_inode() but dropping
the MDB update here cause b-tree corruption on the next mount?

The commit message notes that b-tree growth through hfs_bmap_reserve() isn't
refused. If a read operation triggers a cache flush of an extent record,
allocating new physical blocks, the b-tree header is updated and written to
disk with an increased node_count.

However, since hfs_mdb_commit() now returns early when hfs_emergency_ro() is
true, the MDB is not updated on disk. This causes the on-disk MDB size fields
(such as drXTFlSize and drXTExtRec) to desync from the b-tree header.

On the next mount, the b-tree file size is initialized from the stale MDB and
is smaller than node_count * node_size. Accessing the newly allocated nodes
reads past EOF, which the VFS zero-fills, resulting in -EIO during tree
traversal. Furthermore, the newly allocated blocks are not marked as used in
the MDB bitmap, leading to potential data overwrites.

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