| Message ID | 5ee1db09b60373a15890f6a7c835d00e76bf601d.1535682461.git.ernesto.mnd.fernandez@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] hfsplus: prevent btree data loss on root split | expand |
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)
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) > >
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.
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)
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(+)