cls_u32: refcounting fix for u32_delete()

PATCH In review Jarek Poplawski 4 participants 8 messages Apr 11, 2008 – Apr 13, 2008

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Jarek Poplawski

author

2008-04-11 12:43 UTC #0 lore raw

PATCH 1/1 cls_u32: refcounting fix for u32_delete()

[NET_SCHED] cls_u32: refcounting fix for u32_delete()

Deleting of nonroot hnodes mostly doesn't work in u32_delete():
refcnt == 1 is expected, but such hnodes' refcnts are initialized with 0 and charged only with "link" nodes. Now they'll start with 1 like usual.

Signed-off-by Jarek Poplawski <[email protected]>

Cc Jamal Hadi Salim <[email protected]>

---

 net/sched/cls_u32.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

net/sched/cls_u32.c +7 -3

index c5c16b4..4f150b9 100644
@@ -411,8 +411,10 @@ static void u32_destroy(struct tcf_proto *tp)
 			}
 		}
 
-		for (ht=tp_c->hlist; ht; ht = ht->next)
+		for (ht = tp_c->hlist; ht; ht = ht->next) {
+			ht->refcnt--;
 			u32_clear_hnode(tp, ht);
+		}
 
 		while ((ht = tp_c->hlist) != NULL) {
 			tp_c->hlist = ht->next;

@@ -441,8 +443,10 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 	if (tp->root == ht)
 		return -EINVAL;
 
-	if (--ht->refcnt == 0)
+	if (ht->refcnt == 1) {
+		ht->refcnt--;
 		u32_destroy_hnode(tp, ht);
+	}
 
 	return 0;
 }

@@ -568,7 +572,7 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
 		if (ht == NULL)
 			return -ENOBUFS;
 		ht->tp_c = tp_c;
-		ht->refcnt = 0;
+		ht->refcnt = 1;
 		ht->divisor = divisor;
 		ht->handle = handle;
 		ht->prio = tp->prio;

Patrick McHardy

2008-04-11 12:55 UTC #1 lore raw

net/sched/cls_u32.c static int u32_delete(struct tcf_proto *tp, unsigned long arg) · line 450

Quoted 12 lines · Jarek Poplawski

[NET_SCHED] cls_u32: refcounting fix for u32_delete()

net/sched/cls_u32.c +3 -1

@@ -441,8 +443,10 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 	if (tp->root == ht)
 		return -EINVAL;
 
-	if (--ht->refcnt == 0)
+	if (ht->refcnt == 1) {
+		ht->refcnt--;
 		u32_destroy_hnode(tp, ht);
+	}

Shouldn't the refcount be decremented unconditionally? Otherwise I'd suggest to reject the removal if refcnt > 1, but silently doing nothing doesn't seem right.

Jarek Poplawski

author

2008-04-11 13:20 UTC #2 lore raw

Quoted 16 lines · Patrick McHardy

net/sched/cls_u32.c static int u32_delete(struct tcf_proto *tp, unsigned long arg) · line 450
Quoted 11 lines · Jarek Poplawski
[NET_SCHED] cls_u32: refcounting fix for u32_delete()
net/sched/cls_u32.c +3 -0
@@ -441,8 +443,10 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 	if (tp->root == ht)
 		return -EINVAL;
 -	if (--ht->refcnt == 0)
+	if (ht->refcnt == 1) {
+		ht->refcnt--;
 		u32_destroy_hnode(tp, ht);
+	}
 
Shouldn't the refcount be decremented unconditionally? Otherwise I'd suggest to reject the removal if refcnt > 1, but silently doing nothing doesn't seem right.

If someone tries deleting and the refcnt is e.g. 3 (2 links) unconditional deleting could decrease refcnt without deleting and then unlinking would make it below 0. I think it's OK now:
we can't delete hnode until it's referenced, but I could add a warning if you like?

Thanks,
Jarek P.

Patrick McHardy

2008-04-11 13:22 UTC #3 lore raw

Quoted 17 lines · Jarek Poplawski

Quoted 15 lines · Patrick McHardy
net/sched/cls_u32.c static int u32_delete(struct tcf_proto *tp, unsigned long arg) · line 450
Quoted 11 lines · Jarek Poplawski
[NET_SCHED] cls_u32: refcounting fix for u32_delete()
net/sched/cls_u32.c +3 -0
@@ -441,8 +443,10 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 	if (tp->root == ht)
 		return -EINVAL;
 -	if (--ht->refcnt == 0)
+	if (ht->refcnt == 1) {
+		ht->refcnt--;
 		u32_destroy_hnode(tp, ht);
+	}
 
Shouldn't the refcount be decremented unconditionally? Otherwise I'd suggest to reject the removal if refcnt > 1, but silently doing nothing doesn't seem right.

Quoted 5 lines

If someone tries deleting and the refcnt is e.g. 3 (2 links) unconditional deleting could decrease refcnt without deleting and then unlinking would make it below 0. I think it's OK now:
we can't delete hnode until it's referenced, but I could add a warning if you like?

I'd suggest to behave similar to qdiscs when trying to removing classes that are still referenced, return -EBUSY.

Jarek Poplawski

author

2008-04-11 13:45 UTC #4 lore raw

PATCH 1/1 cls_u32: refcounting fix for u32_delete()

On Fri, Apr 11, 2008 at 03:22:51PM +0200, Patrick McHardy wrote:
...

Quoted 2 lines

I'd suggest to behave similar to qdiscs when trying to removing classes that are still referenced, return -EBUSY.

Of course you are right!

Jarek P.

-------------------> (take 2)

[NET_SCHED] cls_u32: refcounting fix for u32_delete()

Signed-off-by Jarek Poplawski <[email protected]>

Cc Jamal Hadi Salim <[email protected]>

---

net/sched/cls_u32.c +9 -3

index c5c16b4..4d75544 100644
@@ -411,8 +411,10 @@ static void u32_destroy(struct tcf_proto *tp)
 			}
 		}
 
-		for (ht=tp_c->hlist; ht; ht = ht->next)
+		for (ht = tp_c->hlist; ht; ht = ht->next) {
+			ht->refcnt--;
 			u32_clear_hnode(tp, ht);
+		}
 
 		while ((ht = tp_c->hlist) != NULL) {
 			tp_c->hlist = ht->next;

@@ -441,8 +443,12 @@ static int u32_delete(struct tcf_proto *tp, unsigned long arg)
 	if (tp->root == ht)
 		return -EINVAL;
 
-	if (--ht->refcnt == 0)
+	if (ht->refcnt == 1) {
+		ht->refcnt--;
 		u32_destroy_hnode(tp, ht);
+	} else {
+		return -EBUSY;
+	}
 
 	return 0;
 }

@@ -568,7 +574,7 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
 		if (ht == NULL)
 			return -ENOBUFS;
 		ht->tp_c = tp_c;
-		ht->refcnt = 0;
+		ht->refcnt = 1;
 		ht->divisor = divisor;
 		ht->handle = handle;
 		ht->prio = tp->prio;

jamal

Acked-by on v1 1/1

2008-04-11 13:55 UTC #5 lore raw

Quoted 13 lines · Jarek Poplawski

On Fri, Apr 11, 2008 at 03:22:51PM +0200, Patrick McHardy wrote:
...
Quoted 2 lines
I'd suggest to behave similar to qdiscs when trying to removing classes that are still referenced, return -EBUSY.
Of course you are right!
Jarek P.
-------------------> (take 2)
[NET_SCHED] cls_u32: refcounting fix for u32_delete()

Quoted 8 lines

Deleting of nonroot hnodes mostly doesn't work in u32_delete():
refcnt == 1 is expected, but such hnodes' refcnts are initialized with 0 and charged only with "link" nodes. Now they'll start with 1 like usual. Thanks to Patrick McHardy for an improving suggestion.
Signed-off-by Jarek Poplawski <[email protected]>
Cc Jamal Hadi Salim <[email protected]>

Thanks as always Jarek.

Acked-by Jamal Hadi Salim <[email protected]>

cheers,
jamal

David Miller

maintainer Applied

2008-04-13 01:37 UTC #6 lore raw

From: jamal <[email protected]>
Date: Fri, 11 Apr 2008 09:55:27 -0400

Quoted 20 lines · Jarek Poplawski

Quoted 19 lines · Patrick McHardy
...
Quoted 2 lines
I'd suggest to behave similar to qdiscs when trying to removing classes that are still referenced, return -EBUSY.
Of course you are right!
Jarek P.
-------------------> (take 2)
[NET_SCHED] cls_u32: refcounting fix for u32_delete()
Deleting of nonroot hnodes mostly doesn't work in u32_delete():
refcnt == 1 is expected, but such hnodes' refcnts are initialized with 0 and charged only with "link" nodes. Now they'll start with 1 like usual. Thanks to Patrick McHardy for an improving suggestion.

Quoted 6 lines

Quoted 2 lines
Signed-off-by Jarek Poplawski <[email protected]>
Cc Jamal Hadi Salim <[email protected]>
Thanks as always Jarek.
Acked-by Jamal Hadi Salim <[email protected]>

Applied, thanks everyone.

Patrick McHardy

2008-04-11 14:00 UTC #7 lore raw

In reply to Jarek Poplawski

Quoted 6 lines · Jarek Poplawski

[NET_SCHED] cls_u32: refcounting fix for u32_delete()
Deleting of nonroot hnodes mostly doesn't work in u32_delete():
refcnt == 1 is expected, but such hnodes' refcnts are initialized with 0 and charged only with "link" nodes. Now they'll start with 1 like usual. Thanks to Patrick McHardy for an improving suggestion.

Looks good, thanks Jarek.