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.

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);
+
+			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:
-- 
2.43.0

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:

On Wed, Apr 15, 2026 at 12:59 AM Viacheslav Dubeyko <vdubeyko@redhat.com> wrote:
>
> 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?
>

Hi Vyacheslav,

Thank you for the review and for pushing back on the commit message.

Let me explain my initial approach. When I saw "hfsplus: xattr search
failed" in the crash log, I traced it to hfsplus_delete_all_attrs()
in hfsplus_delete_cat(). I noticed the return value was completely
ignored and thought that was the root cause — that the ignored error
was allowing execution to continue into hfsplus_cat_write_inode()
with the filesystem in a bad state.

However after your feedback I looked more carefully at the actual
crash site:

    hfsplus_unlink+0x57a/0x930 fs/hfsplus/dir.c:435
    res = hfsplus_cat_write_inode(sbi->hidden_dir);

You are right — sbi->hidden_dir is NULL here. When a corrupt image
is mounted where the hidden directory is absent.

hfsplus_link() and hfsplus_unlink() then call
hfsplus_cat_write_inode(sbi->hidden_dir) unconditionally without
checking for NULL. Other call sites in dir.c already guard against
this; these two were simply missed.

I will send v3 with the correct fix in dir.c.

I also want to be transparent — I used Claude AI as a learning tool
while working through this bug, and used it to help generate the
commit message. I should have verified the root cause more carefully
before sending. I apologize for the noise.

Regards,
Deeanshu Kartikey

On Wed, 2026-04-15 at 07:10 +0530, Deepanshu Kartikey wrote:
> On Wed, Apr 15, 2026 at 12:59 AM Viacheslav Dubeyko <vdubeyko@redhat.com> wrote:
> > 
> > 

<skipped>

> 
> I also want to be transparent — I used Claude AI as a learning tool
> while working through this bug, and used it to help generate the
> commit message. I should have verified the root cause more carefully
> before sending. I apologize for the noise.
> 

Don't get me wrong. I am not against of using the Claude AI. It helps me to be
more efficient too. But we should check the Claude AI's output really carefully.
Because, it generates workarounds but not the fixes very frequently. It is
useful tool, anyway.

Thanks,
Slava.