Re: [PATCH v2] hfsplus: fix ignored error return in hfsplus_delete_cat

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Tue, 2026-04-14 at 07:26 +0530, Deepanshu Kartikey wrote:
> hfsplus_delete_cat() calls hfsplus_delete_all_attrs() to remove all
> extended attributes associated with a catalog entry, but silently
> discards its return value.
> 
> When the xattr deletion fails on a corrupt filesystem image (as
> triggered by syzbot), hfsplus_delete_cat() incorrectly returns 0
> (success) to its callers, allowing execution to continue with the
> filesystem in an inconsistent state and eventually triggering a
> general protection fault in hfsplus_cat_write_inode().
> 
> Fix this by capturing the return value of hfsplus_delete_all_attrs()
> and propagating genuine errors back to the caller. -ENOENT is
> excluded since it signals normal loop termination (no more xattrs
> left to delete) and is not an error condition.
> 

Frankly speaking, I don't quite follow how your fix relates to the initial
problem?

vfs_unlink+0x272/0x6d0 fs/namei.c:5476

error = dir->i_op->unlink(dir, dentry);

hfsplus_unlink+0x57a/0x930 fs/hfsplus/dir.c:435

res = hfsplus_cat_write_inode(sbi->hidden_dir);

RIP: 0010:hfsplus_cat_write_inode+0xbe/0x8f0 fs/hfsplus/inode.c:637

struct hfs_btree *tree = HFSPLUS_SB(inode->i_sb)->cat_tree;

It looks like that sbi->hidden_dir is NULL and this is why crash here.

But how have it happened at first place?


> Changes in v2:
>   - Fixed commit message: hfsplus_delete_cat() is called from multiple
>     callers (hfsplus_unlink, hfsplus_rmdir, hfsplus_rename,
>     hfsplus_evict_inode, hfsplus_fill_super), not just hfsplus_unlink()
>     as incorrectly stated in v1.
> 
> Fixes: 324ef39a8a4f ("hfsplus: add support of manipulation by attributes file")
> Reported-by: syzbot+c0ba772a362e70937dfb@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c0ba772a362e70937dfb
> Tested-by: syzbot+c0ba772a362e70937dfb@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  fs/hfsplus/catalog.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 02c1eee4a4b8..adbaeabc06ab 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -421,8 +421,15 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, const struct qstr *str)
>  	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>  
>  	if (type == HFSPLUS_FILE || type == HFSPLUS_FOLDER) {
> -		if (HFSPLUS_SB(sb)->attr_tree)
> -			hfsplus_delete_all_attrs(dir, cnid);
> +		if (HFSPLUS_SB(sb)->attr_tree) {
> +			int attr_err = hfsplus_delete_all_attrs(dir, cnid);

It looks like that this code is generated by Claude AI or something like this.

Thanks,
Slava.

> +
> +			if (attr_err && attr_err != -ENOENT) {
> +				pr_err("hfsplus: failed to delete xattrs for cnid %u: %d\n",
> +				       cnid, attr_err);
> +				err = attr_err;
> +			}
> +		}
>  	}
>  
>  out: