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@xxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx
> Cc: Joe Perches <joe@xxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Vyacheslav Dubeyko <slava@xxxxxxxxxxx>
> Cc: Hin-Tak Leung <htl10@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Anton Altaparmakov <aia21@xxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Signed-off-by: Sergei Antonov <saproj@xxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html