On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote: > On 2025/08/01 3:03, Viacheslav Dubeyko wrote: > > On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote: > > > On 2025/07/31 4:24, Viacheslav Dubeyko wrote: > > > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it > > > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that > > > > HFS_POR_CNID could be a problem in hfs_write_inode()? > > > > > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG() > > > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode() > > > shall have to also reject 1, 5 and 15 (and as a result only accept 2). > > > > The fix should be in hfs_read_inode(). Currently, suggested solution hides the > > issue but not fix the problem. > > Not fixing this problem might be hiding other issues, by hitting BUG() before > other issues shows up. > I am not going to start a philosophical discussion. We simply need to fix the bug. The suggested patch doesn't fix the issue. > > Because b-tree nodes could contain multiple > > corrupted records. Now, this patch checks only record for root folder. Let's > > imagine that root folder record will be OK but another record(s) will be > > corrupted in such way. > > Can the inode number of the record retrieved as a result of > hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ? > > If the inode number of the record retrieved as a result of > hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be > a complete fix for this problem. > You are working with corrupted volume. In this case, you can extract any state of the Catalog File's record. > > Finally, we will have successful mount but operation with > > corrupted record(s) will trigger this issue. So, I cannot consider this patch as > > a complete fix of the problem. > > Did you try what you think as a fix of this problem (I guess something like > shown below will be needed for avoid hitting BUG()) using > https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@xxxxxxxxxxxxxxxxxxx ? > If you believe that you have another version of the patch, then simply send it and I will review it. Sorry, I haven't enough time to discuss every movement of your thoughts. > diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c > index a81ce7a740b9..d60395111ed5 100644 > --- a/fs/hfs/inode.c > +++ b/fs/hfs/inode.c > @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask) > tree = HFS_SB(sb)->cat_tree; > break; > default: > - BUG(); > + pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n", > + inode->i_ino); > return false; > } > > @@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data) > case HFS_CDR_FIL: > return inode->i_ino == be32_to_cpu(rec->file.FlNum); > default: > - BUG(); > + pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type); > return 1; > } > } > > +static bool is_bad_id(unsigned long ino) > +{ > + switch (ino) { > + case 0: > + case 3: > + case 4: > + case 6: > + case 7: > + case 8: > + case 9: > + case 10: > + case 11: > + case 12: > + case 13: > + case 14: > + return true; > + } > + return false; > +} Please, don't use hardcoded value. I already shared the point that we must use the declared constants. This function is incorrect and it cannot work for folders and files at the same time. Thanks, Slava. > + > /* > * hfs_read_inode > */ > @@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data) > } > > inode->i_ino = be32_to_cpu(rec->file.FlNum); > + if (is_bad_id(inode->i_ino)) { > + make_bad_inode(inode); > + break; > + } > inode->i_mode = S_IRUGO | S_IXUGO; > if (!(rec->file.Flags & HFS_FIL_LOCK)) > inode->i_mode |= S_IWUGO; > @@ -358,9 +383,15 @@ static int hfs_read_inode(struct inode *inode, void *data) > inode->i_op = &hfs_file_inode_operations; > inode->i_fop = &hfs_file_operations; > inode->i_mapping->a_ops = &hfs_aops; > + if (inode->i_ino < 16) > + pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino); > break; > case HFS_CDR_DIR: > inode->i_ino = be32_to_cpu(rec->dir.DirID); > + if (is_bad_id(inode->i_ino)) { > + make_bad_inode(inode); > + break; > + } > inode->i_size = be16_to_cpu(rec->dir.Val) + 2; > HFS_I(inode)->fs_blocks = 0; > inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask); > @@ -368,6 +399,8 @@ static int hfs_read_inode(struct inode *inode, void *data) > inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat)))); > inode->i_op = &hfs_dir_inode_operations; > inode->i_fop = &hfs_dir_operations; > + if (inode->i_ino < 16) > + pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino); > break; > default: > make_bad_inode(inode); > @@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc) > hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree); > return 0; > default: > - BUG(); > + pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n", > + inode->i_ino); > return -EIO; > } > } > > > # for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done > # dmesg | grep fsck > [ 52.563547] [ T479] hfs: detected unknown inode 1, running fsck.hfs is recommended. > [ 56.606238] [ T255] hfs: detected unknown inode 5, running fsck.hfs is recommended. > [ 66.694795] [ T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.
