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

Ernesto A. Fernández
2018-10-24T11:14:53Z

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)
>
>