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 <[email protected]> > 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: [email protected] > Cc: Joe Perches <[email protected]> > Cc: Andrew Morton <[email protected]> > Cc: Vyacheslav Dubeyko <[email protected]> > Cc: Hin-Tak Leung <[email protected]> > Cc: Anton Altaparmakov <[email protected]> > Cc: Al Viro <[email protected]> > Cc: Christoph Hellwig <[email protected]> > Signed-off-by: Sergei Antonov <[email protected]> > --- > 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 stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
