From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-pg1-f193.google.com ([209.85.215.193]:46541 "EHLO
        mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1725826AbeJXLeb (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 24 Oct 2018 07:34:31 -0400
Received: by mail-pg1-f193.google.com with SMTP id a5-v6so723177pgt.13
        for <linux-fsdevel@vger.kernel.org>; Tue, 23 Oct 2018 20:08:27 -0700 (PDT)
Message-ID: <1540344233.28908.6.camel@slavad-ubuntu-14.04>
Subject: Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split
From: Viacheslav Dubeyko <slava@dubeyko.com>
To: "Ernesto A." =?ISO-8859-1?Q?Fern=E1ndez?=
        <ernesto.mnd.fernandez@gmail.com>
Cc: linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 23 Oct 2018 18:23:53 -0700
In-Reply-To: <26d882184fc43043a810114258f45277752186c7.1535682461.git.ernesto.mnd.fernandez@gmail.com>
References: <26d882184fc43043a810114258f45277752186c7.1535682461.git.ernesto.mnd.fernandez@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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?

Thanks,
Vyacheslav Dubeyko.

> +		}
>  		fd->bnode = hfs_bnode_find(tree, new_node->parent);
>  		/* create index key and entry */
>  		hfs_bnode_read_key(new_node, fd->search_key, 14);