Skip to contentSkip to conversation

Fix slab corruption with netem

PATCH In review Guillaume Chazarain 2 participants 4 messages Jul 15, 2006

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix slab corruption with netem

Hello,

CONFIG_DEBUG_SLAB found the following bug:
netem_enqueue() in sch_netem.c gets a pointer inside a slab object: struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb; But then, the slab object may be freed: skb = skb_unshare(skb, GFP_ATOMIC)
cb is still pointing inside the freed skb, so here is a patch to initialize cb later, and make it clear that initializing it sooner is a bad idea.

Thanks.

Signature · 1 line
Guillaume
net/sched/sch_netem.c +3 -1
@@ -148,7 +148,8 @@ static int netem_enqueue(struct sk_buff 
 static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
-	struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
+	/* We don't fill cb now as skb_unshare() may invalidate it */
+	struct netem_skb_cb *cb = NULL;
 	struct sk_buff *skb2;
 	int ret;
 	int count = 1;
@@ -200,6 +201,7 @@ static int netem_enqueue(struct sk_buff 
 		skb->data[net_random() % skb_headlen(skb)] ^= 1<<(net_random() % 8);
 	}
 
+	cb = (struct netem_skb_cb *)skb->cb;
 	if (q->gap == 0 		/* not doing reordering */
 	    || q->counter < q->gap 	/* inside last reordering gap */
 	    || q->reorder < get_crandom(&q->reorder_cor)) {
Signature · 1 line
Signed-off-by: Guillaume Chazarain <[email protected]>
#1 lore raw
Quoted 15 lines · Guillaume Chazarain

Hello,

CONFIG_DEBUG_SLAB found the following bug:
netem_enqueue() in sch_netem.c gets a pointer inside a slab object: struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb; But then, the slab object may be freed: skb = skb_unshare(skb, GFP_ATOMIC)
cb is still pointing inside the freed skb, so here is a patch to initialize cb later, and make it clear that initializing it sooner is a bad idea.

Thanks.

------------------------------------------------------------------------

net/sched/sch_netem.c static int netem_enqueue(struct sk_buff · line 153

Quoted 7 lines
net/sched/sch_netem.c +2 -1
@@ -148,7 +148,8 @@ static int netem_enqueue(struct sk_buff 
 static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
-	struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
+	/* We don't fill cb now as skb_unshare() may invalidate it */
+	struct netem_skb_cb *cb = NULL;
 

Would rather leave it unitialized, rather than setting to NULL.

Quoted 13 lines
net/sched/sch_netem.c +0 -0
 	struct sk_buff *skb2;
 	int ret;
net/sched/sch_netem.c +1 -0
 	int count = 1;
@@ -200,6 +201,7 @@ static int netem_enqueue(struct sk_buff 
 		skb->data[net_random() % skb_headlen(skb)] ^= 1<<(net_random() % 8);
 	}
 
+	cb = (struct netem_skb_cb *)skb->cb;
 	if (q->gap == 0 		/* not doing reordering */
 	    || q->counter < q->gap 	/* inside last reordering gap */
 	    || q->reorder < get_crandom(&q->reorder_cor)) {
#2 lore raw
Quoted 5 lines · Stephen Hemminger

net/sched/sch_netem.c static int netem_enqueue(struct sk_buff · line 152

Quoted 4 lines
net/sched/sch_netem.c +2 -1
-    struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
+    /* We don't fill cb now as skb_unshare() may invalidate it */
+    struct netem_skb_cb *cb = NULL;

Would rather leave it unitialized, rather than setting to NULL.

I find that strange. If someone mistakenly uses cb before it is initialized it could be hard to figure out why some memory is corrupted. But by initializing cb to NULL, it will always trigger an Oops, so it will be easier to debug in this case.

Cheers.

Signature · 1 line
Guillaume
#3 lore raw
Quoted 5 lines · Guillaume Chazarain
Quoted 4 lines · Stephen Hemminger
Quoted 4 lines
net/sched/sch_netem.c +2 -1
-    struct netem_skb_cb *cb = (struct netem_skb_cb *)skb->cb;
+    /* We don't fill cb now as skb_unshare() may invalidate it */
+    struct netem_skb_cb *cb = NULL;
Quoted 8 lines
Quoted 1 line

Would rather leave it unitialized, rather than setting to NULL.

I find that strange. If someone mistakenly uses cb before it is initialized it could be hard to figure out why some memory is corrupted. But by initializing cb to NULL, it will always trigger an Oops, so it will be easier to debug in this case.

Cheers.

Gcc will complain if variable is used before initialized, and I would rather see it at compile time than runtime.