Patchwork [1/2] rbtree: fix postorder iteration when the rb_node is not the first element in an entry

login
register
mail settings
Submitter Cody P Schafer
Date Nov. 5, 2013, 1:40 a.m.
Message ID <1383615602-1784-1-git-send-email-cody@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/288354/
State Superseded
Headers show

Comments

Cody P Schafer - Nov. 5, 2013, 1:40 a.m.
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(-)
Cody P Schafer - Nov. 5, 2013, 10:05 a.m.
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
Jan Kara - Nov. 5, 2013, 9:57 p.m.
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

Patch

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 */