[PATCH] hfs: fix not erasing deleted b-tree node issue

[PATCH] hfs: fix not erasing deleted b-tree node issue

[Viacheslav Dubeyko]

The generic/001 test of xfstests suite fails and corrupts
the HFS volume:

sudo ./check generic/001
FSTYP         -- hfs
PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.15.0-rc2+ #3 SMP PREEMPT_DYNAMIC Fri Apr 25 17:13:00 PDT 2>
MKFS_OPTIONS  -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch

generic/001 32s ... _check_generic_filesystem: filesystem on /dev/loop50 is inconsistent
(see /home/slavad/XFSTESTS-2/xfstests-dev/results//generic/001.full for details)

Ran: generic/001
Failures: generic/001
Failed 1 of 1 tests

fsck.hfs -d -n ./test-image.bin
** ./test-image.bin (NO WRITE)
	Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
   Executing fsck_hfs (version 540.1-Linux).
** Checking HFS volume.
   The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
   Unused node is not erased (node = 2)
   Unused node is not erased (node = 4)
<skipped>
   Unused node is not erased (node = 253)
   Unused node is not erased (node = 254)
   Unused node is not erased (node = 255)
   Unused node is not erased (node = 256)
** Checking catalog hierarchy.
** Checking volume bitmap.
** Checking volume information.
   Verify Status: VIStat = 0x0000, ABTStat = 0x0000 EBTStat = 0x0000
                  CBTStat = 0x0004 CatStat = 0x00000000
** The volume untitled was found corrupt and needs to be repaired.
	volume type is HFS
	primary MDB is at block 2 0x02
	alternate MDB is at block 20971518 0x13ffffe
	primary VHB is at block 0 0x00
	alternate VHB is at block 0 0x00
	sector size = 512 0x200
	VolumeObject flags = 0x19
	total sectors for volume = 20971520 0x1400000
	total sectors for embedded volume = 0 0x00

This patch adds logic of clearing the deleted b-tree node.

sudo ./check generic/001
FSTYP         -- hfs
PLATFORM      -- Linux/x86_64 hfsplus-testing-0001 6.15.0-rc2+ #3 SMP PREEMPT_DYNAMIC Fri Apr 25 17:13:00 PDT 2025
MKFS_OPTIONS  -- /dev/loop51
MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch

generic/001 9s ...  32s
Ran: generic/001
Passed all 1 tests

fsck.hfs -d -n ./test-image.bin
** ./test-image.bin (NO WRITE)
	Using cacheBlockSize=32K cacheTotalBlock=1024 cacheSize=32768K.
   Executing fsck_hfs (version 540.1-Linux).
** Checking HFS volume.
   The volume name is untitled
** Checking extents overflow file.
** Checking catalog file.
** Checking catalog hierarchy.
** Checking volume bitmap.
** Checking volume information.
** The volume untitled appears to be OK.

Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
---
 fs/hfs/bnode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
index cb823a8a6ba9..c5eae7c418a1 100644
--- a/fs/hfs/bnode.c
+++ b/fs/hfs/bnode.c
@@ -219,6 +219,8 @@ void hfs_bnode_unlink(struct hfs_bnode *node)
 		tree->root = 0;
 		tree->depth = 0;
 	}
+
+	hfs_bnode_clear(node, 0, tree->node_size);
 	set_bit(HFS_BNODE_DELETED, &node->flags);
 }
 
-- 
2.34.1

Re: [PATCH] hfs: fix not erasing deleted b-tree node issue

[Johannes Thumshirn]

On 29.04.25 03:16, Viacheslav Dubeyko wrote:
> Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
> ---
>   fs/hfs/bnode.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> index cb823a8a6ba9..c5eae7c418a1 100644
> --- a/fs/hfs/bnode.c
> +++ b/fs/hfs/bnode.c
> @@ -219,6 +219,8 @@ void hfs_bnode_unlink(struct hfs_bnode *node)
>   		tree->root = 0;
>   		tree->depth = 0;
>   	}
> +
> +	hfs_bnode_clear(node, 0, tree->node_size);
>   	set_bit(HFS_BNODE_DELETED, &node->flags);
>   }
>   

Hi Slava,

I've just checked HFS+ code and hfs_bnode_unlink() in fs/hfsplus/bnode.c 
is a copy of the fs/hfs/bnode.c one (maybe most of the file is so 
there's room for unification?). So I think the fix is needed there as well.

Byte,
	Johannes

Re: [PATCH] hfs: fix not erasing deleted b-tree node issue

[John Paul Adrian Glaubitz]

Hi Johannes,

On Tue, 2025-04-29 at 06:05 +0000, Johannes Thumshirn wrote:
> On 29.04.25 03:16, Viacheslav Dubeyko wrote:
> > Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
> > ---
> >   fs/hfs/bnode.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> > index cb823a8a6ba9..c5eae7c418a1 100644
> > --- a/fs/hfs/bnode.c
> > +++ b/fs/hfs/bnode.c
> > @@ -219,6 +219,8 @@ void hfs_bnode_unlink(struct hfs_bnode *node)
> >   		tree->root = 0;
> >   		tree->depth = 0;
> >   	}
> > +
> > +	hfs_bnode_clear(node, 0, tree->node_size);
> >   	set_bit(HFS_BNODE_DELETED, &node->flags);
> >   }
> >   
> 
> I've just checked HFS+ code and hfs_bnode_unlink() in fs/hfsplus/bnode.c 
> is a copy of the fs/hfs/bnode.c one (maybe most of the file is so 
> there's room for unification?). So I think the fix is needed there as well.

Good catch. I assume we should be able to reproduce the above error with xfstests
with HFS+ as well and the same fix should address the bug accordingly.

I haven't created my xfstests setup yet, so hopefully someone will beat me to this.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] hfs: fix not erasing deleted b-tree node issue

[Viacheslav Dubeyko]

On Tue, 2025-04-29 at 06:05 +0000, Johannes Thumshirn wrote:
> On 29.04.25 03:16, Viacheslav Dubeyko wrote:
> > Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
> > ---
> >   fs/hfs/bnode.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> > index cb823a8a6ba9..c5eae7c418a1 100644
> > --- a/fs/hfs/bnode.c
> > +++ b/fs/hfs/bnode.c
> > @@ -219,6 +219,8 @@ void hfs_bnode_unlink(struct hfs_bnode *node)
> >   		tree->root = 0;
> >   		tree->depth = 0;
> >   	}
> > +
> > +	hfs_bnode_clear(node, 0, tree->node_size);
> >   	set_bit(HFS_BNODE_DELETED, &node->flags);
> >   }
> >   
> 
> Hi Slava,
> 
> I've just checked HFS+ code and hfs_bnode_unlink() in
> fs/hfsplus/bnode.c 
> is a copy of the fs/hfs/bnode.c one (maybe most of the file is so 
> there's room for unification?). So I think the fix is needed there as
> well.
> 

Yeah, makes sense. This fix should be there too. I simply started from
HFS and I didn't take a look into the HFS+ code yet. Let me prepare the
patch for HFS+ too.

Unification makes sense, but it requires more efforts. I think we need
to fix at first the bugs that are failing xfstests run. However, the
unification could be not so simple taking into account the difference
in on-disk layout and current implementation of drivers. So, let me
think about it more.

Thanks,
Slava.

Re: [PATCH] hfs: fix not erasing deleted b-tree node issue

[Viacheslav Dubeyko]

On Tue, 2025-04-29 at 10:19 -0700, Viacheslav Dubeyko wrote:
> On Tue, 2025-04-29 at 06:05 +0000, Johannes Thumshirn wrote:
> > On 29.04.25 03:16, Viacheslav Dubeyko wrote:
> > > Signed-off-by: Viacheslav Dubeyko <slava@dubeyko.com>
> > > ---
> > >   fs/hfs/bnode.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/fs/hfs/bnode.c b/fs/hfs/bnode.c
> > > index cb823a8a6ba9..c5eae7c418a1 100644
> > > --- a/fs/hfs/bnode.c
> > > +++ b/fs/hfs/bnode.c
> > > @@ -219,6 +219,8 @@ void hfs_bnode_unlink(struct hfs_bnode *node)
> > >   		tree->root = 0;
> > >   		tree->depth = 0;
> > >   	}
> > > +
> > > +	hfs_bnode_clear(node, 0, tree->node_size);
> > >   	set_bit(HFS_BNODE_DELETED, &node->flags);
> > >   }
> > >   
> > 
> > Hi Slava,
> > 
> > I've just checked HFS+ code and hfs_bnode_unlink() in
> > fs/hfsplus/bnode.c 
> > is a copy of the fs/hfs/bnode.c one (maybe most of the file is so 
> > there's room for unification?). So I think the fix is needed there
> > as
> > well.
> > 
> 
> Yeah, makes sense. This fix should be there too. I simply started
> from
> HFS and I didn't take a look into the HFS+ code yet. Let me prepare
> the
> patch for HFS+ too.
> 

I double checked the HFS+ code. The deleted node's clearing logic is
already there. The hfs_bnode_unlink() set HFS_BNODE_DELETED flag. Then,
hfs_bnode_put() check this flag. And if it is set, then
hfs_need_zeroout() is called. If it is the catalog file and volume
attributes contain HFSPLUS_VOL_UNUSED_NODE_FIX flag, then
hfs_bnode_clear() is called. Also, I don't see the corruption of the
HFS+ volume for the case of generic/001 test-case.

Technically speaking, I could follow to the same logic in HFS (I mean
of placing hfs_bnode_clear() into hfs_bnode_put()). But, as far as I
can see, HFS hasn't similar volume attribute like HFS+ has.

Thanks,
Slava.