From mboxrd@z Thu Jan 1 00:00:00 1970 From: Viacheslav Dubeyko Subject: Re: [PATCH 2.6 to 4.0] hfsplus: fix B-tree corruption after insertion at position 0 Date: Tue, 17 Mar 2015 09:50:40 -0700 Message-ID: <1426611040.2901.3.camel@slavad-ubuntu-14.04> References: <1426558248-7508-1-git-send-email-saproj@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: linux-fsdevel@vger.kernel.org, stable@vger.kernel.org, Joe Perches , Andrew Morton , Hin-Tak Leung , Anton Altaparmakov , Al Viro , Christoph Hellwig To: Sergei Antonov Return-path: In-Reply-To: <1426558248-7508-1-git-send-email-saproj@gmail.com> Sender: stable-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Tue, 2015-03-17 at 03:10 +0100, Sergei Antonov wrote: > Fix B-tree corruption when a new record is inserted at position 0 in the node > in hfs_brec_insert(). In this case a hfs_brec_update_parent() is called to > update the parent index node (if exists) and it is passed hfs_find_data with > a search_key containing a newly inserted key instead of the key to be updated. > This results in an inconsistent index node. The bug reproduces on my machine > after an extents overflow record for the catalog file (CNID=4) is inserted into > the extents overflow B-tree. Because of a low (reserved) value of CNID=4, it > has to become the first record in the first leaf node. > The resulting first leaf node is correct: > ---------------------------------------------------- > | key0.CNID=4 | key1.CNID=123 | key2.CNID=456, ... | > ---------------------------------------------------- > But the parent index key0 still contains the previous key CNID=123: > ----------------------- > | key0.CNID=123 | ... | > ----------------------- > > A change in hfs_brec_insert() makes hfs_brec_update_parent() work correctly > by preventing it from getting fd->record=-1 value from __hfs_brec_find(). > The fix looks good for me. Thank you. Reviewed-by: Vyacheslav Dubeyko > Along the way, I removed duplicate code with unification of the if condition. > The resulting code is equivalent to the original code because node is never 0. > > Also hfs_brec_update_parent() will now return an error after getting a negative > fd->record value. However, the return value of hfs_brec_update_parent() is not > checked anywhere in the file and I'm leaving it unchanged by this patch. > brec.c lacks error checking after some other calls too, but this issue is of > less importance than the one being fixed by this patch. But I think that to check the returned value is important. So, how do you feel about adding checking of error codes and some error messages? Thanks, Vyacheslav Dubeyko. > > Cc: stable@vger.kernel.org > Cc: Joe Perches > Cc: Andrew Morton > Cc: Vyacheslav Dubeyko > Cc: Hin-Tak Leung > Cc: Anton Altaparmakov > Cc: Al Viro > Cc: Christoph Hellwig > Signed-off-by: Sergei Antonov > --- > fs/hfsplus/brec.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > index 6e560d5..754fdf8 100644 > --- a/fs/hfsplus/brec.c > +++ b/fs/hfsplus/brec.c > @@ -131,13 +131,16 @@ skip: > hfs_bnode_write(node, entry, data_off + key_len, entry_len); > hfs_bnode_dump(node); > > - if (new_node) { > - /* update parent key if we inserted a key > - * at the start of the first node > - */ > - if (!rec && new_node != node) > - hfs_brec_update_parent(fd); > + /* > + * update parent key if we inserted a key > + * at the start of the node and it is not the new node > + */ > + if (!rec && new_node != node) { > + hfs_bnode_read_key(node, fd->search_key, data_off + size); > + hfs_brec_update_parent(fd); > + } > > + if (new_node) { > hfs_bnode_put(fd->bnode); > if (!new_node->parent) { > hfs_btree_inc_height(tree); > @@ -168,9 +171,6 @@ skip: > goto again; > } > > - if (!rec) > - hfs_brec_update_parent(fd); > - > return 0; > } > > @@ -370,6 +370,8 @@ again: > if (IS_ERR(parent)) > return PTR_ERR(parent); > __hfs_brec_find(parent, fd, hfs_find_rec_by_key); > + if (fd->record < 0) > + return -ENOENT; > hfs_bnode_dump(parent); > rec = fd->record; >