From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-f194.google.com ([209.85.160.194]:43938 "EHLO mail-qt1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725784AbeJZEQn (ORCPT ); Fri, 26 Oct 2018 00:16:43 -0400 Received: by mail-qt1-f194.google.com with SMTP id q41-v6so11340570qtq.10 for ; Thu, 25 Oct 2018 12:42:36 -0700 (PDT) Date: Thu, 25 Oct 2018 16:42:31 -0300 From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= To: Viacheslav Dubeyko Cc: linux-fsdevel@vger.kernel.org, Andrew Morton Subject: Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split Message-ID: <20181025194231.mbfwmt6fhmokrn52@eaf> References: <26d882184fc43043a810114258f45277752186c7.1535682461.git.ernesto.mnd.fernandez@gmail.com> <1540344233.28908.6.camel@slavad-ubuntu-14.04> <20181024013259.4gbxlqiqhmpopeqh@eaf> <1540486317.2669.6.camel@dubeyko.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1540486317.2669.6.camel@dubeyko.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Oct 25, 2018 at 09:51:57AM -0700, Viacheslav Dubeyko wrote: > 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 > > > 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. This patch is two months old now. Why did you wait until the merge window to give me your traditional "the patch is confusing" reply? > I simply mean here that as implementation > as concept of the HFS+ b-trees is not trivial. But everybody should > easy understand the patch. Those two sentences are not compatible. Reviewers need to have some understanding of the code they review. No royal road... > Thanks, > Vyacheslav Dubeyko. >