[PATCH][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[PATCH][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[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.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
Cc: Jamal Hadi Salim <hadi@cyberus.ca>

---

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

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index c5c16b4..4f150b9 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -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;

Re: [PATCH][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[Patrick McHardy]

Jarek Poplawski wrote:
> [NET_SCHED] cls_u32: refcounting fix for u32_delete()
> 
> @@ -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.

Re: [PATCH][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[Jarek Poplawski]

On Fri, Apr 11, 2008 at 02:55:27PM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> [NET_SCHED] cls_u32: refcounting fix for u32_delete()
>>
>> @@ -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.

Re: [PATCH][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[Patrick McHardy]

Jarek Poplawski wrote:
> On Fri, Apr 11, 2008 at 02:55:27PM +0200, Patrick McHardy wrote:
>> Jarek Poplawski wrote:
>>> [NET_SCHED] cls_u32: refcounting fix for u32_delete()
>>>
>>> @@ -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?


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

[PATCH take2][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[Jarek Poplawski]

On Fri, Apr 11, 2008 at 03:22:51PM +0200, Patrick McHardy wrote:
...
> 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.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
Cc: Jamal Hadi Salim <hadi@cyberus.ca>

---

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index c5c16b4..4d75544 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -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;

Re: [PATCH take2][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[jamal]

On Fri, 2008-11-04 at 13:48 +0000, Jarek Poplawski wrote:
> On Fri, Apr 11, 2008 at 03:22:51PM +0200, Patrick McHardy wrote:
> ...
> > 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.
> 
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> Cc: Jamal Hadi Salim <hadi@cyberus.ca>

Thanks as always Jarek.

Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

cheers,
jamal

Re: [PATCH take2][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Fri, 11 Apr 2008 09:55:27 -0400

> On Fri, 2008-11-04 at 13:48 +0000, Jarek Poplawski wrote:
> > On Fri, Apr 11, 2008 at 03:22:51PM +0200, Patrick McHardy wrote:
> > ...
> > > 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.
> > 
> > 
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> > Cc: Jamal Hadi Salim <hadi@cyberus.ca>
> 
> Thanks as always Jarek.
> 
> Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

Applied, thanks everyone.

Re: [PATCH take2][NET_SCHED] cls_u32: refcounting fix for u32_delete()

[Patrick McHardy]

Jarek Poplawski wrote:
> [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.