Message-ID: <22cddf1f1db9a6c9efdf21f8b3197f858d37ec70.camel@ibm.com>
Date: Mon, 14 Jul 2025 23:30:41 +0000
From: Viacheslav Dubeyko <Slava.Dubeyko@....com>
To: "frank.li@...o.com" <frank.li@...o.com>,
        "glaubitz@...sik.fu-berlin.de"
	<glaubitz@...sik.fu-berlin.de>,
        "penguin-kernel@...ove.SAKURA.ne.jp"
	<penguin-kernel@...ove.SAKURA.ne.jp>,
        "slava@...eyko.com"
	<slava@...eyko.com>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
CC: "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] hfsplus: don't use BUG_ON() in hfsplus_create_attributes_file()

On Sat, 2025-07-12 at 20:22 +0900, Tetsuo Handa wrote:
> On 2025/07/12 2:21, Viacheslav Dubeyko wrote:
> > Frankly speaking, I still don't see the whole picture here. If we have created
> > the Attribute File during mount operation, then why should we try to create the
> > Attributes File during __hfsplus_setxattr() call? If we didn't create the
> > Attributes File during the mount time and HFSPLUS_SB(inode->i_sb)->attr_tree is
> > NULL, then how i_size_read(attr_file) != 0? Even if we are checking vhdr-
> > > attr_file.total_blocks, then it doesn't provide guarantee that
> > i_size_read(attr_file) is zero too. Something is wrong in this situation and
> > more stricter mount time validation cannot guarantee against the situation that
> > you are trying to solve in the issue. We are missing something here.
> 
> I still don't see what you are missing.
> 
> When hfsplus_iget(sb, HFSPLUS_ATTR_CNID) is called from hfsplus_create_attributes_file(sb),
> hfsplus_system_read_inode(inode) from hfsplus_iget(HFSPLUS_ATTR_CNID) calls
> hfsplus_inode_read_fork(inode, &vhdr->attr_file). Since hfsplus_inode_read_fork() calls
> inode_set_bytes(), it is natural that i_size_read(attr_file) != 0 when returning from
> hfsplus_iget(sb, HFSPLUS_ATTR_CNID).
> 
> At this point, the only question should be why hfsplus_inode_read_fork() from
> hfsplus_system_read_inode(inode) from hfsplus_iget() is not called from hfsplus_fill_super()
> when the Attributes File already exists and its size is not 0. And the reason is that
> hfsplus_iget(sb, HFSPLUS_ATTR_CNID) from hfs_btree_open(sb, HFSPLUS_ATTR_CNID) is called
> only when vhdr->attr_file.total_blocks != 0.
> 
> That is, when "vhdr" contains erroneous values (in the reproducer, vhdr->attr_file.total_blocks
> is 0) that do not reflect the actual state of the filesystem (in the reproducer, inode_set_bytes()
> sets non-zero value despite vhdr->attr_file.total_blocks is 0), hfsplus_fill_super() fails to call
> hfs_btree_open(sb, HFSPLUS_ATTR_CNID) at mount time.
> 

Yes, you are right here. I was missing that we have corrupted state of the
volume.

Related to reworking the mount-time validation logic... I think it's not very
good idea because hfs_btree_open() will:
(1) allocate memory: tree = kzalloc(sizeof(*tree), GFP_KERNEL);
(2) get inode: inode = hfsplus_iget(sb, id);
(3) try to read memory page: page = read_mapping_page(mapping, 0, NULL);
And, then, we need to free memory and to make all necessary cleanup. It's too
much activity for the really empty tree.

But we can make some improvement of the initial patch. We definitely know that
it's the volume corruption. So, from my point of view, it makes sense to add
comment that explains the whole situation and change the pr_err() message with
recommendation to run the FSCK tool:

if (i_size_read(attr_file) != 0) {
    err = -EIO;
    /* explain here how we detected that volume is corrupted */
    pr_err("HFS+ superblock contains incorrect Attributes File details. "
           "Probably, volume is corrupted!!! Please, run FSCK tool!!!\n");
    goto end_attr_file_creation;
}

You can modify my comments as you like. :)

Thanks,
Slava.

> You can easily reproduce this problem by compiling and running the reproducer
> at https://syzkaller.appspot.com/text?tag=ReproC&x=15f6b9d4580000   after you run
> "losetup -f" which creates /dev/loop0 needed by the reproducer.
> 
> I noticed that the reason fsck.hfsplus could not detect errors is that the filesystem
> image in the reproducer was compressed. If I run fsck.hfsplus on uncompressed image,
> fsck.hfsplus generated the following messages.
> 
> # fsck.hfsplus hfsplus.img
> ** hfsplus.img
>    Executing fsck_hfs (version 540.1-Linux).
> ** Checking non-journaled HFS Plus Volume.
>    The volume name is untitled
> ** Checking extents overflow file.
> ** Checking catalog file.
>    Invalid extent entry
> (4, 1)
> ** Checking multi-linked files.
> ** Checking catalog hierarchy.
> ** Checking extended attributes file.
> ** Checking volume bitmap.
> ** Checking volume information.
> ** Repairing volume.
>    Look for links to corrupt files in DamagedFiles directory.
> ** Rechecking volume.
> ** Checking non-journaled HFS Plus Volume.
>    The volume name is untitled
> ** Checking extents overflow file.
> ** Checking catalog file.
> ** Checking multi-linked files.
> ** Checking catalog hierarchy.
> ** Checking extended attributes file.
> ** Checking volume bitmap.
>    Volume bitmap needs minor repair for under-allocation
> ** Checking volume information.
>    Invalid volume free block count
>    (It should be 179 instead of 180)
> ** Repairing volume.
> ** Rechecking volume.
> ** Checking non-journaled HFS Plus Volume.
>    The volume name is untitled
> ** Checking extents overflow file.
> ** Checking catalog file.
> ** Checking multi-linked files.
> ** Checking catalog hierarchy.
> ** Checking extended attributes file.
> ** Checking volume bitmap.
> ** Checking volume information.
> ** The volume untitled was repaired successfully.

Powered by blists - more mailing lists