[PATCH 1/6] hfsplus: prevent btree data loss on root split

[PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

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;
+		}
 		fd->bnode = hfs_bnode_find(tree, new_node->parent);
 		/* create index key and entry */
 		hfs_bnode_read_key(new_node, fd->search_key, 14);
-- 
2.11.0

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

[Ernesto A. Fernández]

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;
 	}
 
 	if (!rec && node->parent)
-- 
2.11.0

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

[Viacheslav Dubeyko]

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. I like to assign the NULL
value to the pointer. But are you sure that it's proper place? Maybe it
makes sense to place this assignment in the beginning of the function?

Thanks,
Vyacheslav Dubeyko.


>  	}
>  
>  	if (!rec && node->parent)

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

[Ernesto A. Fernández]

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

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

[Ernesto A. Fernández]

On Wed, Oct 24, 2018 at 02:39:47PM -0700, Andrew Morton wrote:
> On Tue, 23 Oct 2018 23:48:46 -0300 Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> wrote:
> 
> > > 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.
> 
> I think it would be clearer to do it this way:
> 
> --- a/fs/hfs/brec.c~a
> +++ a/fs/hfs/brec.c
> @@ -359,11 +359,11 @@ static int hfs_brec_update_parent(struct
>  
>  	tree = fd->tree;
>  	node = fd->bnode;
> -	new_node = NULL;
>  	if (!node->parent)
>  		return 0;
>  
>  again:
> +	new_node = NULL;
>  	parent = hfs_bnode_find(tree, node->parent);
>  	if (IS_ERR(parent))
>  		return PTR_ERR(parent);
> 
> But it doesn't matter much...

Right, that looks better.  I don't remember why I did it this way.  If
you want me to resend I'd rather run some tests again, just in case.

[PATCH 3/6] hfsplus: prevent btree data loss on ENOSPC

[Ernesto A. Fernández]

Inserting or deleting a record in a btree may require splitting several
of its nodes.  If we hit ENOSPC halfway through, the new nodes will be
left orphaned and their records will be lost.  This could mean lost
inodes, extents or xattrs.

>From now on, check the available disk space before making any changes.
This still leaves the potential problem of corruption on ENOMEM.

The patch can be tested with xfstests generic/027.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfsplus/attributes.c | 10 ++++++++++
 fs/hfsplus/btree.c      | 42 +++++++++++++++++++++++++++---------------
 fs/hfsplus/catalog.c    | 24 ++++++++++++++++++++++++
 fs/hfsplus/extents.c    |  4 ++++
 fs/hfsplus/hfsplus_fs.h |  2 ++
 5 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c
index 2bab6b3cdba4..e6d554476db4 100644
--- a/fs/hfsplus/attributes.c
+++ b/fs/hfsplus/attributes.c
@@ -217,6 +217,11 @@ int hfsplus_create_attr(struct inode *inode,
 	if (err)
 		goto failed_init_create_attr;
 
+	/* Fail early and avoid ENOSPC during the btree operation */
+	err = hfs_bmap_reserve(fd.tree, fd.tree->depth + 1);
+	if (err)
+		goto failed_create_attr;
+
 	if (name) {
 		err = hfsplus_attr_build_key(sb, fd.search_key,
 						inode->i_ino, name);
@@ -313,6 +318,11 @@ int hfsplus_delete_attr(struct inode *inode, const char *name)
 	if (err)
 		return err;
 
+	/* Fail early and avoid ENOSPC during the btree operation */
+	err = hfs_bmap_reserve(fd.tree, fd.tree->depth);
+	if (err)
+		goto out;
+
 	if (name) {
 		err = hfsplus_attr_build_key(sb, fd.search_key,
 						inode->i_ino, name);
diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index de14b2b6881b..d20902f3e015 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -342,26 +342,21 @@ static struct hfs_bnode *hfs_bmap_new_bmap(struct hfs_bnode *prev, u32 idx)
 	return node;
 }
 
-struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
+/* Make sure @tree has enough space for the @rsvd_nodes */
+int hfs_bmap_reserve(struct hfs_btree *tree, int rsvd_nodes)
 {
-	struct hfs_bnode *node, *next_node;
-	struct page **pagep;
-	u32 nidx, idx;
-	unsigned off;
-	u16 off16;
-	u16 len;
-	u8 *data, byte, m;
-	int i;
+	struct inode *inode = tree->inode;
+	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
+	u32 count;
+	int res;
 
-	while (!tree->free_nodes) {
-		struct inode *inode = tree->inode;
-		struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
-		u32 count;
-		int res;
+	if (rsvd_nodes <= 0)
+		return 0;
 
+	while (tree->free_nodes < rsvd_nodes) {
 		res = hfsplus_file_extend(inode, hfs_bnode_need_zeroout(tree));
 		if (res)
-			return ERR_PTR(res);
+			return res;
 		hip->phys_size = inode->i_size =
 			(loff_t)hip->alloc_blocks <<
 				HFSPLUS_SB(tree->sb)->alloc_blksz_shift;
@@ -372,6 +367,23 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 		tree->free_nodes = count - tree->node_count;
 		tree->node_count = count;
 	}
+	return 0;
+}
+
+struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
+{
+	struct hfs_bnode *node, *next_node;
+	struct page **pagep;
+	u32 nidx, idx;
+	unsigned off;
+	u16 off16;
+	u16 len;
+	u8 *data, byte, m;
+	int i, res;
+
+	res = hfs_bmap_reserve(tree, 1);
+	if (res)
+		return ERR_PTR(res);
 
 	nidx = 0;
 	node = hfs_bnode_find(tree, nidx);
diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index a196369ba779..35472cba750e 100644
--- a/fs/hfsplus/catalog.c
+++ b/fs/hfsplus/catalog.c
@@ -265,6 +265,14 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
 	if (err)
 		return err;
 
+	/*
+	 * Fail early and avoid ENOSPC during the btree operations. We may
+	 * have to split the root node at most once.
+	 */
+	err = hfs_bmap_reserve(fd.tree, 2 * fd.tree->depth);
+	if (err)
+		goto err2;
+
 	hfsplus_cat_build_key_with_cnid(sb, fd.search_key, cnid);
 	entry_size = hfsplus_fill_cat_thread(sb, &entry,
 		S_ISDIR(inode->i_mode) ?
@@ -333,6 +341,14 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, const struct qstr *str)
 	if (err)
 		return err;
 
+	/*
+	 * Fail early and avoid ENOSPC during the btree operations. We may
+	 * have to split the root node at most once.
+	 */
+	err = hfs_bmap_reserve(fd.tree, 2 * (int)fd.tree->depth - 2);
+	if (err)
+		goto out;
+
 	if (!str) {
 		int len;
 
@@ -433,6 +449,14 @@ int hfsplus_rename_cat(u32 cnid,
 		return err;
 	dst_fd = src_fd;
 
+	/*
+	 * Fail early and avoid ENOSPC during the btree operations. We may
+	 * have to split the root node at most twice.
+	 */
+	err = hfs_bmap_reserve(src_fd.tree, 4 * (int)src_fd.tree->depth - 1);
+	if (err)
+		goto out;
+
 	/* find the old dir entry and read the data */
 	err = hfsplus_cat_build_key(sb, src_fd.search_key,
 			src_dir->i_ino, src_name);
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index 8e0f59767694..8a8893d522ef 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -100,6 +100,10 @@ static int __hfsplus_ext_write_extent(struct inode *inode,
 	if (hip->extent_state & HFSPLUS_EXT_NEW) {
 		if (res != -ENOENT)
 			return res;
+		/* Fail early and avoid ENOSPC during the btree operation */
+		res = hfs_bmap_reserve(fd->tree, fd->tree->depth + 1);
+		if (res)
+			return res;
 		hfs_brec_insert(fd, hip->cached_extents,
 				sizeof(hfsplus_extent_rec));
 		hip->extent_state &= ~(HFSPLUS_EXT_DIRTY | HFSPLUS_EXT_NEW);
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 8e039435958a..dd7ad9f13e3a 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -311,6 +311,7 @@ static inline unsigned short hfsplus_min_io_size(struct super_block *sb)
 #define hfs_btree_open hfsplus_btree_open
 #define hfs_btree_close hfsplus_btree_close
 #define hfs_btree_write hfsplus_btree_write
+#define hfs_bmap_reserve hfsplus_bmap_reserve
 #define hfs_bmap_alloc hfsplus_bmap_alloc
 #define hfs_bmap_free hfsplus_bmap_free
 #define hfs_bnode_read hfsplus_bnode_read
@@ -395,6 +396,7 @@ u32 hfsplus_calc_btree_clump_size(u32 block_size, u32 node_size, u64 sectors,
 struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id);
 void hfs_btree_close(struct hfs_btree *tree);
 int hfs_btree_write(struct hfs_btree *tree);
+int hfs_bmap_reserve(struct hfs_btree *tree, int rsvd_nodes);
 struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree);
 void hfs_bmap_free(struct hfs_bnode *node);
 
-- 
2.11.0

[PATCH 4/6] hfs: prevent btree data loss on root split

[Ernesto A. Fernández]

This 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.  It is not possible for this
to happen on a valid hfs filesystem because the index nodes have fixed
length keys.

For reasons I ignore, the hfs module does have support for a number of
hfsplus features.  A corrupt btree header may report variable length
keys and trigger this bug, so it's better to fix it.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfs/brec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/hfs/brec.c b/fs/hfs/brec.c
index 9a8772465a90..da25c49203cc 100644
--- a/fs/hfs/brec.c
+++ b/fs/hfs/brec.c
@@ -425,6 +425,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;
+		}
 		fd->bnode = hfs_bnode_find(tree, new_node->parent);
 		/* create index key and entry */
 		hfs_bnode_read_key(new_node, fd->search_key, 14);
-- 
2.11.0

[PATCH 5/6] hfs: fix BUG on bnode parent update

[Ernesto A. Fernández]

hfs_brec_update_parent() 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.  It is not possible for this to happen on a valid hfs
filesystem because the index nodes have fixed length keys.

For reasons I ignore, the hfs module does have support for a number of
hfsplus features.  A corrupt btree header may report variable length
keys and trigger this BUG, so it's better to fix it.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfs/brec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfs/brec.c b/fs/hfs/brec.c
index da25c49203cc..896396554bcc 100644
--- a/fs/hfs/brec.c
+++ b/fs/hfs/brec.c
@@ -445,6 +445,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;
 	}
 
 	if (!rec && node->parent)
-- 
2.11.0

[PATCH 6/6] hfs: prevent btree data loss on ENOSPC

[Ernesto A. Fernández]

Inserting a new record in a btree may require splitting several of its
nodes.  If we hit ENOSPC halfway through, the new nodes will be left
orphaned and their records will be lost.  This could mean lost inodes
or extents.

>From now on, check the available disk space before making any changes.
This still leaves the potential problem of corruption on ENOMEM.

There is no need to reserve space before deleting a catalog record, as
we do for hfsplus.  This difference is because hfs index nodes have
fixed length keys.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfs/btree.c   | 39 ++++++++++++++++++++++++---------------
 fs/hfs/btree.h   |  1 +
 fs/hfs/catalog.c | 16 ++++++++++++++++
 fs/hfs/extent.c  |  4 ++++
 4 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/fs/hfs/btree.c b/fs/hfs/btree.c
index 374b5688e29e..66c9e7f7170c 100644
--- a/fs/hfs/btree.c
+++ b/fs/hfs/btree.c
@@ -220,25 +220,17 @@ static struct hfs_bnode *hfs_bmap_new_bmap(struct hfs_bnode *prev, u32 idx)
 	return node;
 }
 
-struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
+/* Make sure @tree has enough space for the @rsvd_nodes */
+int hfs_bmap_reserve(struct hfs_btree *tree, int rsvd_nodes)
 {
-	struct hfs_bnode *node, *next_node;
-	struct page **pagep;
-	u32 nidx, idx;
-	unsigned off;
-	u16 off16;
-	u16 len;
-	u8 *data, byte, m;
-	int i;
-
-	while (!tree->free_nodes) {
-		struct inode *inode = tree->inode;
-		u32 count;
-		int res;
+	struct inode *inode = tree->inode;
+	u32 count;
+	int res;
 
+	while (tree->free_nodes < rsvd_nodes) {
 		res = hfs_extend_file(inode);
 		if (res)
-			return ERR_PTR(res);
+			return res;
 		HFS_I(inode)->phys_size = inode->i_size =
 				(loff_t)HFS_I(inode)->alloc_blocks *
 				HFS_SB(tree->sb)->alloc_blksz;
@@ -249,6 +241,23 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 		tree->free_nodes = count - tree->node_count;
 		tree->node_count = count;
 	}
+	return 0;
+}
+
+struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
+{
+	struct hfs_bnode *node, *next_node;
+	struct page **pagep;
+	u32 nidx, idx;
+	unsigned off;
+	u16 off16;
+	u16 len;
+	u8 *data, byte, m;
+	int i, res;
+
+	res = hfs_bmap_reserve(tree, 1);
+	if (res)
+		return ERR_PTR(res);
 
 	nidx = 0;
 	node = hfs_bnode_find(tree, nidx);
diff --git a/fs/hfs/btree.h b/fs/hfs/btree.h
index c8b252dbb26c..dcc2aab1b2c4 100644
--- a/fs/hfs/btree.h
+++ b/fs/hfs/btree.h
@@ -82,6 +82,7 @@ struct hfs_find_data {
 extern struct hfs_btree *hfs_btree_open(struct super_block *, u32, btree_keycmp);
 extern void hfs_btree_close(struct hfs_btree *);
 extern void hfs_btree_write(struct hfs_btree *);
+extern int hfs_bmap_reserve(struct hfs_btree *, int);
 extern struct hfs_bnode * hfs_bmap_alloc(struct hfs_btree *);
 extern void hfs_bmap_free(struct hfs_bnode *node);
 
diff --git a/fs/hfs/catalog.c b/fs/hfs/catalog.c
index 8a66405b0f8b..d365bf0b8c77 100644
--- a/fs/hfs/catalog.c
+++ b/fs/hfs/catalog.c
@@ -97,6 +97,14 @@ int hfs_cat_create(u32 cnid, struct inode *dir, const struct qstr *str, struct i
 	if (err)
 		return err;
 
+	/*
+	 * Fail early and avoid ENOSPC during the btree operations. We may
+	 * have to split the root node at most once.
+	 */
+	err = hfs_bmap_reserve(fd.tree, 2 * fd.tree->depth);
+	if (err)
+		goto err2;
+
 	hfs_cat_build_key(sb, fd.search_key, cnid, NULL);
 	entry_size = hfs_cat_build_thread(sb, &entry, S_ISDIR(inode->i_mode) ?
 			HFS_CDR_THD : HFS_CDR_FTH,
@@ -295,6 +303,14 @@ int hfs_cat_move(u32 cnid, struct inode *src_dir, const struct qstr *src_name,
 		return err;
 	dst_fd = src_fd;
 
+	/*
+	 * Fail early and avoid ENOSPC during the btree operations. We may
+	 * have to split the root node at most once.
+	 */
+	err = hfs_bmap_reserve(src_fd.tree, 2 * src_fd.tree->depth);
+	if (err)
+		goto out;
+
 	/* find the old dir entry and read the data */
 	hfs_cat_build_key(sb, src_fd.search_key, src_dir->i_ino, src_name);
 	err = hfs_brec_find(&src_fd);
diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index 5d0182654580..0c638c612152 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -117,6 +117,10 @@ static int __hfs_ext_write_extent(struct inode *inode, struct hfs_find_data *fd)
 	if (HFS_I(inode)->flags & HFS_FLG_EXT_NEW) {
 		if (res != -ENOENT)
 			return res;
+		/* Fail early and avoid ENOSPC during the btree operation */
+		res = hfs_bmap_reserve(fd->tree, fd->tree->depth + 1);
+		if (res)
+			return res;
 		hfs_brec_insert(fd, HFS_I(inode)->cached_extents, sizeof(hfs_extent_rec));
 		HFS_I(inode)->flags &= ~(HFS_FLG_EXT_DIRTY|HFS_FLG_EXT_NEW);
 	} else {
-- 
2.11.0

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Christoph Hellwig]

On Fri, Aug 31, 2018 at 12:58:19AM -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

It would be good to wire up this short reproducer as well for xfstests.

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

On Thu, Aug 30, 2018 at 10:36:42PM -0700, Christoph Hellwig wrote:
> On Fri, Aug 31, 2018 at 12:58:19AM -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
> 
> It would be good to wire up this short reproducer as well for xfstests.

Yes, that's my intention. The problem is that mkfs.hfsplus does not allow
setting the size of the filesystem for scratch_mkfs_sized(); you need a
workaround with the device mapper. I think I should submit that patch first
and see if there is a problem with it.

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Dave Chinner]

On Fri, Aug 31, 2018 at 11:55:54AM -0300, Ernesto A. Fern�ndez wrote:
> On Thu, Aug 30, 2018 at 10:36:42PM -0700, Christoph Hellwig wrote:
> > On Fri, Aug 31, 2018 at 12:58:19AM -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
> > 
> > It would be good to wire up this short reproducer as well for xfstests.
> 
> Yes, that's my intention. The problem is that mkfs.hfsplus does not allow
> setting the size of the filesystem for scratch_mkfs_sized(); you need a
> workaround with the device mapper. I think I should submit that patch first
> and see if there is a problem with it.

You don't need to do that. We use loop devices like this w/ mkfs_dev
quite a lot in fstests. For example, generic/361 has pretty much the
exact code pattern you need....

Cheers,

Dave.


-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

On Sat, Sep 01, 2018 at 02:49:26PM +1000, Dave Chinner wrote:
> On Fri, Aug 31, 2018 at 11:55:54AM -0300, Ernesto A. Fernández wrote:
> > On Thu, Aug 30, 2018 at 10:36:42PM -0700, Christoph Hellwig wrote:
> > > On Fri, Aug 31, 2018 at 12:58:19AM -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
> > > 
> > > It would be good to wire up this short reproducer as well for xfstests.
> > 
> > Yes, that's my intention. The problem is that mkfs.hfsplus does not allow
> > setting the size of the filesystem for scratch_mkfs_sized(); you need a
> > workaround with the device mapper. I think I should submit that patch first
> > and see if there is a problem with it.
> 
> You don't need to do that. We use loop devices like this w/ mkfs_dev
> quite a lot in fstests. For example, generic/361 has pretty much the
> exact code pattern you need....

I see what you mean in this case, but I really want to run every test that
uses _scratch_mkfs_sized(); generic/027 in particular, since it's the one
that found these bugs for me.

> 
> Cheers,
> 
> Dave.
> 
> 
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Dave Chinner]

On Sun, Sep 02, 2018 at 01:33:09AM -0300, Ernesto A. Fern�ndez wrote:
> On Sat, Sep 01, 2018 at 02:49:26PM +1000, Dave Chinner wrote:
> > On Fri, Aug 31, 2018 at 11:55:54AM -0300, Ernesto A. Fern�ndez wrote:
> > > On Thu, Aug 30, 2018 at 10:36:42PM -0700, Christoph Hellwig wrote:
> > > > On Fri, Aug 31, 2018 at 12:58:19AM -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
> > > > 
> > > > It would be good to wire up this short reproducer as well for xfstests.
> > > 
> > > Yes, that's my intention. The problem is that mkfs.hfsplus does not allow
> > > setting the size of the filesystem for scratch_mkfs_sized(); you need a
> > > workaround with the device mapper. I think I should submit that patch first
> > > and see if there is a problem with it.
> > 
> > You don't need to do that. We use loop devices like this w/ mkfs_dev
> > quite a lot in fstests. For example, generic/361 has pretty much the
> > exact code pattern you need....
> 
> I see what you mean in this case, but I really want to run every test that
> uses _scratch_mkfs_sized(); generic/027 in particular, since it's the one
> that found these bugs for me.

Trying to hack around SCRATCH_DEV to be some other device set up by
_scratch_mkfs_sized is likely to break things in unexpected ways.
Lots of library routines directly access SCRATCH_DEV or manipulate
it in interesting ways (e.g. build their own dm constructs on it).

IMO the right thing to do is implement the size parameter in
mkfs.hfsplus. I note that it has a dry-run capability (-N <size>)
already allows it to simulate making a filesystem of any size, so it
looks to me that most of what you need it to do is already in the
mkfs code. Then you can test for the mkfs parameter in
_scratch_mkfs_sized....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

On Mon, Sep 03, 2018 at 09:32:36AM +1000, Dave Chinner wrote:
> On Sun, Sep 02, 2018 at 01:33:09AM -0300, Ernesto A. Fernández wrote:
> > On Sat, Sep 01, 2018 at 02:49:26PM +1000, Dave Chinner wrote:
> > > On Fri, Aug 31, 2018 at 11:55:54AM -0300, Ernesto A. Fernández wrote:
> > > > On Thu, Aug 30, 2018 at 10:36:42PM -0700, Christoph Hellwig wrote:
> > > > > On Fri, Aug 31, 2018 at 12:58:19AM -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
> > > > > 
> > > > > It would be good to wire up this short reproducer as well for xfstests.
> > > > 
> > > > Yes, that's my intention. The problem is that mkfs.hfsplus does not allow
> > > > setting the size of the filesystem for scratch_mkfs_sized(); you need a
> > > > workaround with the device mapper. I think I should submit that patch first
> > > > and see if there is a problem with it.
> > > 
> > > You don't need to do that. We use loop devices like this w/ mkfs_dev
> > > quite a lot in fstests. For example, generic/361 has pretty much the
> > > exact code pattern you need....
> > 
> > I see what you mean in this case, but I really want to run every test that
> > uses _scratch_mkfs_sized(); generic/027 in particular, since it's the one
> > that found these bugs for me.
> 
> Trying to hack around SCRATCH_DEV to be some other device set up by
> _scratch_mkfs_sized is likely to break things in unexpected ways.
> Lots of library routines directly access SCRATCH_DEV or manipulate
> it in interesting ways (e.g. build their own dm constructs on it).
> 

I wasn't changing SCRATCH_DEV, the device mapper was only used briefly
on _scratch_mkfs_sized, and then again on _check_scratch_fs. Still your
point stands, Eryu already found problems with my patch.

> IMO the right thing to do is implement the size parameter in
> mkfs.hfsplus. I note that it has a dry-run capability (-N <size>)
> already allows it to simulate making a filesystem of any size, so it
> looks to me that most of what you need it to do is already in the
> mkfs code. Then you can test for the mkfs parameter in
> _scratch_mkfs_sized....

I'll look into it. I will also need to change fsck.hfsplus, since it
claims it found corruption when the filesystem doesn't fill the device.

Thanks.
 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

I'm sorry Andrew, could you please drop patches 3 and 6 of this series?
There is a regression: the free node count is not always updated correctly
when reserving space.  I'll send the new versions as soon as I'm done
testing them.

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Viacheslav Dubeyko]

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

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

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

If 'new_node' had been the result of splitting a node other than root,
then it would have a parent.

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

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Viacheslav Dubeyko]

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 <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?
> 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. I simply mean here that as implementation
as concept of the HFS+ b-trees is not trivial. But everybody should
easy understand the patch.

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

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 <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?
> > 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.
> 

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Viacheslav Dubeyko]

On Thu, 2018-10-25 at 16:42 -0300, Ernesto A. Fernández wrote:
> 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 <ernesto.mnd.fernandez@gm
> > > > > ail.
> > > > > 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...
> 

If you've found an issue or a bug then you did see some error message
and/or callstack. It's not big deal to share it. Moreover, it's
important to include such error message, callstack and issue's details
in the comment section of the patch. Let's imagine that somebody will
use the same test-case and this guy would find another issue. So, how
will it be possible to distinguish your issue with another one for the
same test-case? Does your fix resolve another issue or not? It will be
impossible to realize this without error message or something. I didn't
ask to share some "secret" knowledge. But to share more details about
the issue is really important. Could you please share more details
(error message, callstack and so on) about the found issue? Even if we
are talking about silent corruption then the fsck tool complains about
something. And, finally, you've found some path in the code (callstack)
that is the reason of such corruption. Again, it's not big deal to
share these details.

Thanks,
Vyacheslav Dubeyko. 

Re: [PATCH 1/6] hfsplus: prevent btree data loss on root split

[Ernesto A. Fernández]

On Fri, Oct 26, 2018 at 09:58:49AM -0700, Viacheslav Dubeyko wrote:
> On Thu, 2018-10-25 at 16:42 -0300, Ernesto A. Fernández wrote:
> > 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 <ernesto.mnd.fernandez@gm
> > > > > > ail.
> > > > > > 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...
> > 
> 
> If you've found an issue or a bug then you did see some error message
> and/or callstack. It's not big deal to share it. Moreover, it's
> important to include such error message, callstack and issue's details
> in the comment section of the patch. Let's imagine that somebody will
> use the same test-case and this guy would find another issue. So, how
> will it be possible to distinguish your issue with another one for the
> same test-case? Does your fix resolve another issue or not? It will be
> impossible to realize this without error message or something. I didn't
> ask to share some "secret" knowledge. But to share more details about
> the issue is really important. Could you please share more details
> (error message, callstack and so on) about the found issue? Even if we
> are talking about silent corruption then the fsck tool complains about
> something. And, finally, you've found some path in the code (callstack)
> that is the reason of such corruption. Again, it's not big deal to
> share these details.

I didn't remember the fsck error, so I rebuilt without the patch and ran
the reproducer again.  No idea why you couldn't do that yourself.

The error is "Invalid sibling link (4, 3)".  Have fun with that...
 
> Thanks,
> Vyacheslav Dubeyko. 
>