| Message ID | 1426558248-7508-1-git-send-email-saproj@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <slava@dubeyko.com> > 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 <joe@perches.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Vyacheslav Dubeyko <slava@dubeyko.com> > Cc: Hin-Tak Leung <htl10@users.sourceforge.net> > Cc: Anton Altaparmakov <aia21@cam.ac.uk> > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Sergei Antonov <saproj@gmail.com> > --- > 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; > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(). 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. Cc: stable@vger.kernel.org Cc: Joe Perches <joe@perches.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Vyacheslav Dubeyko <slava@dubeyko.com> Cc: Hin-Tak Leung <htl10@users.sourceforge.net> Cc: Anton Altaparmakov <aia21@cam.ac.uk> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Christoph Hellwig <hch@infradead.org> Signed-off-by: Sergei Antonov <saproj@gmail.com> --- fs/hfsplus/brec.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)