| Message ID | 1383615602-1784-1-git-send-email-cody@linux.vnet.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 11/04/2013 05:40 PM, Cody P Schafer wrote: > Provide a new helper called rb_next_postorder_entry() to perform NULL > checks and container_of() coversions and use it in > rbtree_for_each_entry_safe() to fix oopses that occur when rb_node is > not the first element in the entry. On second thought, it appears I was a bit to hasty with this, and this patch actually breaks things. On 11/04/2013 04:45 PM, Jan Kara wrote:> On Mon 04-11-13 15:26:38, Jan Kara wrote: >> On Fri 01-11-13 15:38:50, Cody P Schafer wrote: >>> Use rbtree_postorder_for_each_entry_safe() to destroy the rbtree instead >>> of opencoding an alternate postorder iteration that modifies the tree >> Thanks. I've merged the patch into my tree. > Hum, except that the kernel oopses with this patch. And I think the > problem is in rbtree_postorder_for_each_entry_safe(). How are those tests > for NULL supposed to work? For example if the tree is empty, 'pos' will be > NULL and you'll call rb_next_postorder(&NULL->field) which is pretty much > guaranteed to oops if 'field' doesn't have offset 0 in the structure... No, it shouldn't oops because pos won't be NULL, &pos->field will be. pos is only assigned via an rb_entry(rb_first_postorder()) or rb_entry(rb_next_postorder()). rb_next_postorder() and rb_first_postorder() can return NULL. That NULL then is munged by rb_entry to be (NULL - offset_of_field). Causing (&pos->field == NULL == (pos + offset_of_field)). That is, unless I've screwed something up (very possible, as this overly hurried patchset shows). I expect it's more likely that my adaptation of this to ext3's usage is buggy. Could you tell me what you did to cause the oops? And/Or post it? -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 05-11-13 02:05:44, Cody P Schafer wrote: > On 11/04/2013 05:40 PM, Cody P Schafer wrote: > > Provide a new helper called rb_next_postorder_entry() to perform NULL > > checks and container_of() coversions and use it in > > rbtree_for_each_entry_safe() to fix oopses that occur when rb_node is > > not the first element in the entry. > > On second thought, it appears I was a bit to hasty with this, and this patch actually breaks things. > > On 11/04/2013 04:45 PM, Jan Kara wrote:> On Mon 04-11-13 15:26:38, Jan Kara wrote: > >> On Fri 01-11-13 15:38:50, Cody P Schafer wrote: > >>> Use rbtree_postorder_for_each_entry_safe() to destroy the rbtree instead > >>> of opencoding an alternate postorder iteration that modifies the tree > >> Thanks. I've merged the patch into my tree. > > Hum, except that the kernel oopses with this patch. And I think the > > problem is in rbtree_postorder_for_each_entry_safe(). How are those tests > > for NULL supposed to work? For example if the tree is empty, 'pos' will be > > NULL and you'll call rb_next_postorder(&NULL->field) which is pretty much > > guaranteed to oops if 'field' doesn't have offset 0 in the structure... > > No, it shouldn't oops because pos won't be NULL, &pos->field will be. > > pos is only assigned via an rb_entry(rb_first_postorder()) or > rb_entry(rb_next_postorder()). rb_next_postorder() and > rb_first_postorder() can return NULL. That NULL then is munged by > rb_entry to be (NULL - offset_of_field). Causing (&pos->field == NULL == > (pos + offset_of_field)). OK, so I had a second look. And the compiler thinks differently than you :) The thing is that my gcc (4.3.4) apparently assumes pointer underflow is undefined and thus optimizes your test &pos->field to 1. I've asked our gcc guys for a definitive answer but clearly your code will need some way to avoid pointer underflows... Honza
diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h index aa870a4..630eedb 100644 --- a/include/linux/rbtree.h +++ b/include/linux/rbtree.h @@ -86,6 +86,18 @@ static inline void rb_link_node(struct rb_node * node, struct rb_node * parent, } /** + * rb_next_postorder_entry - a helper to check for a NULL entry and advance to + * the next element. + * + * @elem: a 'type *' which is contained in an rbtree + * @field: the field in 'type' which contains the struct rb_node. + */ +#define rb_next_postorder_entry(elem, field) \ + ((elem) ? rb_entry(rb_next_postorder(&(elem)->field), \ + typeof(*(elem)), field) \ + : NULL) + +/** * rbtree_postorder_for_each_entry_safe - iterate over rb_root in post order of * given type safe against removal of rb_node entry * @@ -96,11 +108,9 @@ static inline void rb_link_node(struct rb_node * node, struct rb_node * parent, */ #define rbtree_postorder_for_each_entry_safe(pos, n, root, field) \ for (pos = rb_entry(rb_first_postorder(root), typeof(*pos), field),\ - n = rb_entry(rb_next_postorder(&pos->field), \ - typeof(*pos), field); \ - &pos->field; \ + n = rb_next_postorder_entry(pos, field); \ + pos; \ pos = n, \ - n = rb_entry(rb_next_postorder(&pos->field), \ - typeof(*pos), field)) + n = rb_next_postorder_entry(pos, field)) #endif /* _LINUX_RBTREE_H */ diff --git a/lib/rbtree.c b/lib/rbtree.c index 65f4eff..08168d0 100644 --- a/lib/rbtree.c +++ b/lib/rbtree.c @@ -534,8 +534,6 @@ static struct rb_node *rb_left_deepest_node(const struct rb_node *node) struct rb_node *rb_next_postorder(const struct rb_node *node) { const struct rb_node *parent; - if (!node) - return NULL; parent = rb_parent(node); /* If we're sitting on node, we've already seen our children */
Provide a new helper called rb_next_postorder_entry() to perform NULL checks and container_of() coversions and use it in rbtree_for_each_entry_safe() to fix oopses that occur when rb_node is not the first element in the entry. Additionally, remove the missplaced NULL check from rb_next_postorder(). Signed-off-by: Cody P Schafer <cody@linux.vnet.ibm.com> --- include/linux/rbtree.h | 20 +++++++++++++++----- lib/rbtree.c | 2 -- 2 files changed, 15 insertions(+), 7 deletions(-)