Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Tue, 2018-10-23 at 22:32 -0300, Ernesto A. Fernández wrote:
> On Tue, Oct 23, 2018 at 06:23:53PM -0700, Viacheslav Dubeyko wrote:
> > 
> > On Fri, 2018-08-31 at 00:58 -0300, Ernesto A. Fernández wrote:
> > > 
> > > Creating, renaming or deleting a file may cause catalog
> > > corruption and
> > > data loss.  This bug is randomly triggered by xfstests
> > > generic/027, but
> > > here is a faster reproducer:
> > > 
> > >   truncate -s 50M fs.iso
> > >   mkfs.hfsplus fs.iso
> > >   mount fs.iso /mnt
> > >   i=100
> > >   while [ $i -le 150 ]; do
> > >     touch /mnt/$i &>/dev/null
> > >     ((++i))
> > >   done
> > >   i=100
> > >   while [ $i -le 150 ]; do
> > >     mv /mnt/$i /mnt/$(perl -e "print $i x82") &>/dev/null
> > >     ((++i))
> > >   done
> > >   umount /mnt
> > >   fsck.hfsplus -n fs.iso
> > > 
> > > The bug is triggered whenever hfs_brec_update_parent() needs to
> > > split
> > > the root node.  The height of the btree is not increased, which
> > > leaves
> > > the new node orphaned and its records lost.
> > > 
> > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.
> > > com>
> > > ---
> > >  fs/hfsplus/brec.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> > > index ed8eacb34452..aa17a392b414 100644
> > > --- a/fs/hfsplus/brec.c
> > > +++ b/fs/hfsplus/brec.c
> > > @@ -429,6 +429,10 @@ static int hfs_brec_update_parent(struct
> > > hfs_find_data *fd)
> > >  	if (new_node) {
> > >  		__be32 cnid;
> > >  
> > > +		if (!new_node->parent) {
> > > +			hfs_btree_inc_height(tree);
> > > +			new_node->parent = tree->root;
> > I worry about the case when we are adding the node on intermediate
> > (not
> > root) level. As far as I can see, we will be in trouble here
> > because I
> > don't see any processing of two possible cases: (1) root node; (2)
> > node
> > of intermediate level. Do I miss something here?
> If 'new_node' had been the result of splitting a node other than
> root,
> then it would have a parent.
> 

Could you please share the callstack or/and more detailed explanation
that would show the correctness of the fix? Currently, it's not enough
details in the comment for easy understanding the issue's environment
and correctness of the fix. I simply mean here that as implementation
as concept of the HFS+ b-trees is not trivial. But everybody should
easy understand the patch.

Thanks,
Vyacheslav Dubeyko.