On Wed, Jun 26, 2013 at 12:02:51PM +0800, Liu Bo wrote: > Several users reported this crash of NULL pointer or general protection, > the story is that we add a rbtree for speedup ulist iteration, and we > use krealloc() to address ulist growth, and krealloc() use memcpy to copy > old data to new memory area, so it's OK for an array as it doesn't use > pointers while it's not OK for a rbtree as it uses pointers. > > So krealloc() will mess up our rbtree and it ends up with crash.
It's not just krealloc(), the initial memcpy() out of the int_nodes array is also buggy. > + /* > + * krealloc actually uses memcpy, which does not copy rb_node > + * pointers, so we have to do it ourselves. Otherwise we may > + * be bitten by crashes. > + */ > + for (i = 0; i < ulist->nnodes; i++) { > + rb_erase(&ulist->nodes[i].rb_node, &ulist->root); > + ret = ulist_rbtree_insert(ulist, &new_nodes[i]); > + BUG_ON(ret); > + } This'll work for the int_nodes case where you still have access to the old pointers for rb_erase() to reference. But in the krealloc() case the rb_erase() will be trying to reference freed memmory because krealloc() frees the old pointer on success. And all the tree balancing in the deletion and re-insertion dance is totally unneeded. And another f-ing BUG_ON()! Just fixup all the pointers: ptrdiff_t diff = new_nodes - old; ulist->root.rb_node += diff; for (i = 0; i < ulist->nnodes; i++) { ulist->nodes[i].rb_node.rb_left += diff; ulist->nodes[i].rb_node.rb_left += diff; } Yeah, it's insane, but no more so than using krealloc() for an array with internal pointers in the first place. - z -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html