From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-qt1-f193.google.com ([209.85.160.193]:42816 "EHLO
        mail-qt1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726080AbeJXLOx (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 24 Oct 2018 07:14:53 -0400
Received: by mail-qt1-f193.google.com with SMTP id j46-v6so4087102qtc.9
        for <linux-fsdevel@vger.kernel.org>; Tue, 23 Oct 2018 19:48:52 -0700 (PDT)
Date: Tue, 23 Oct 2018 23:48:46 -0300
From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?=
        <ernesto.mnd.fernandez@gmail.com>
To: Viacheslav Dubeyko <slava@dubeyko.com>
Cc: linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 2/6] hfsplus: fix BUG on bnode parent update
Message-ID: <20181024024846.hbq6oftilihwumpt@eaf>
References: <26d882184fc43043a810114258f45277752186c7.1535682461.git.ernesto.mnd.fernandez@gmail.com>
 <5ee1db09b60373a15890f6a7c835d00e76bf601d.1535682461.git.ernesto.mnd.fernandez@gmail.com>
 <1540344830.28908.12.camel@slavad-ubuntu-14.04>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1540344830.28908.12.camel@slavad-ubuntu-14.04>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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