Re: [PATCH 2/6] hfsplus: fix BUG on bnode parent update

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

On Tue, Oct 23, 2018 at 06:33:50PM -0700, Viacheslav Dubeyko wrote:
> On Fri, 2018-08-31 at 00:59 -0300, Ernesto A. Fernández wrote:
> > Creating, renaming or deleting a file may hit BUG_ON() if the first
> > record of both a leaf node and its parent are changed, and if this
> > forces the parent to be split.  This bug is triggered by xfstests
> > generic/027, somewhat rarely; here is a more reliable reproducer:
> > 
> >   truncate -s 50M fs.iso
> >   mkfs.hfsplus fs.iso
> >   mount fs.iso /mnt
> >   i=1000
> >   while [ $i -le 2400 ]; do
> >     touch /mnt/$i &>/dev/null
> >     ((++i))
> >   done
> >   i=2400
> >   while [ $i -ge 1000 ]; do
> >     mv /mnt/$i /mnt/$(perl -e "print $i x61") &>/dev/null
> >     ((--i))
> >   done
> > 
> > The issue is that a newly created bnode is being put twice.  Reset
> > new_node to NULL in hfs_brec_update_parent() before reaching goto again.
> > 
> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> > ---
> >  fs/hfsplus/brec.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> > index aa17a392b414..1918544a7871 100644
> > --- a/fs/hfsplus/brec.c
> > +++ b/fs/hfsplus/brec.c
> > @@ -449,6 +449,7 @@ static int hfs_brec_update_parent(struct hfs_find_data *fd)
> >  			/* restore search_key */
> >  			hfs_bnode_read_key(node, fd->search_key, 14);
> >  		}
> > +		new_node = NULL;
> 
> Sorry, I don't follow where the new_node is put twice. Could you explain
> in more details? Currently, it looks unclear.

There is a 'goto again', as I said in the commit message.  Follow the code
and you'll see that hfs_bnode_put() is called again on the same node.

> I like to assign the NULL value to the pointer.

This isn't a matter of taste.

> But are you sure that it's proper place?

Yes, but it's always better if somebody reviews the code...

> Maybe it
> makes sense to place this assignment in the beginning of the function?

Without knowing what you mean by "beginning of the function", I can't
tell if your idea would work or not.

> Thanks,
> Vyacheslav Dubeyko.
> 
> 
> >  	}
> >  
> >  	if (!rec && node->parent)
> 
>