[BUG] net_cls: Panic occured when net_cls subsystem use

On Tue, 2009-06-02 at 06:26 +0000, Jarek Poplawski wrote:

> The patch #2 is obviously worse and fixes less (of course it still
> needs testing for Minoru's case), but I'm 100% confident it can't
> introduce any regression (neither take 1 nor 2), which is much harder
> to say about patch #1, considering various "rude" configs we could
> miss (but we could give them some time to show off). But, as I've
> written before, I'm really (really) OK with your decision.

Thanks for the courtesy.
I note Dave already swallowed Minoru's patch; so lets move from there.
Yes, there's a possibility of a regression - I (and so are you) are only
recently evolved humans; we are not perfect... yet ;->
So i would agree with Minoru testing your patch as plan B in case the
applied one starts causing trouble.
BTW, ok - here's a quick untested, uncompiled fix to the u32 classifier
to fix the first rock (which you already worked around in your changes
to the included patch). No rush to submit for now..

On the second rock you threw so violently, after some reflection, I
think it is ok to send a replace twice with different priorities.
The second one will be added and the old not deleted, but if the user
has chosen the correct priority, then things will work out just fine.
And if they want they have to explicitly delete the one they dont want.
It is also not illegal to do a "replace" for installing instead of
"add".

So the only other things left to do from this exercise are (no rush in
any of them):
a) remove all "buckets" from underneath other classifiers
b) get consistency across all classifiers in usage of setup API

If you want to do this - go ahead; else i plan on tackling it probably
when stable 2.6.31 kicks in.

cheers,
jamal

On Tue, Jun 02, 2009 at 09:16:27AM -0400, jamal wrote:
...
> Thanks for the courtesy.
> I note Dave already swallowed Minoru's patch; so lets move from there.
> Yes, there's a possibility of a regression - I (and so are you) are only
> recently evolved humans; we are not perfect... yet ;->
> So i would agree with Minoru testing your patch as plan B in case the
> applied one starts causing trouble.
> BTW, ok - here's a quick untested, uncompiled fix to the u32 classifier
> to fix the first rock (which you already worked around in your changes
> to the included patch). No rush to submit for now..

Thanks for your courtesy as well. Alas, I'm not sure I can fully
understand the current patch. You seem to redefine the ->get() method
usage, so it looks for handle only for configured tp's. It might be
right but I need more time to check this.

> 
> On the second rock you threw so violently, after some reflection, I
> think it is ok to send a replace twice with different priorities.
> The second one will be added and the old not deleted, but if the user
> has chosen the correct priority, then things will work out just fine.
> And if they want they have to explicitly delete the one they dont want.
> It is also not illegal to do a "replace" for installing instead of
> "add".
> 
> So the only other things left to do from this exercise are (no rush in
> any of them):
> a) remove all "buckets" from underneath other classifiers
> b) get consistency across all classifiers in usage of setup API
> 
> If you want to do this - go ahead; else i plan on tackling it probably
> when stable 2.6.31 kicks in.

I definitely prefer you doing this and me asking "rude" questions.;-)

Cheers,
Jarek P.

> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 07372f6..5ad0b98 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -249,6 +249,9 @@ static unsigned long u32_get(struct tcf_proto *tp, u32 handle)
>  	struct tc_u_hnode *ht;
>  	struct tc_u_common *tp_c = tp->data;
>  
> +	if (!tp_c)
> +		return 0;
> +
>  	if (TC_U32_HTID(handle) == TC_U32_ROOT)
>  		ht = tp->root;
>  	else
> @@ -311,7 +314,6 @@ static int u32_init(struct tcf_proto *tp)
>  	root_ht->tp_c = tp_c;
>  
>  	tp->root = root_ht;
> -	tp->data = tp_c;
>  	return 0;
>  }
>  
> @@ -524,7 +526,7 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
>  		      struct nlattr **tca,
>  		      unsigned long *arg)
>  {
> -	struct tc_u_common *tp_c = tp->data;
> +	struct tc_u_common *tp_c = tp->root->tp_c;
>  	struct tc_u_hnode *ht;
>  	struct tc_u_knode *n;
>  	struct tc_u32_sel *s;
> @@ -540,6 +542,7 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
>  	if (err < 0)
>  		return err;
>  
> +	tp->data = tp_c;
>  	if ((n = (struct tc_u_knode*)*arg) != NULL) {
>  		if (TC_U32_KEY(n->handle) == 0)
>  			return -EINVAL;

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 02-06-2009 23:10, Jarek Poplawski wrote:
> On Tue, Jun 02, 2009 at 09:16:27AM -0400, jamal wrote:
...
> Thanks for your courtesy as well. Alas, I'm not sure I can fully
> understand the current patch. You seem to redefine the ->get() method
> usage, so it looks for handle only for configured tp's. It might be
> right but I need more time to check this.

After the second look I have some questions:
- if it's really aimed to skip checking by ->get() tp's before they're
configured in ->change(), maybe instead of using tp_c to check this it
would be simpler to generally skip calling ->get() for newly created
tp's?
- otherwise the current method probably needs adding a tp_c check for
NULL in u32_destroy()?
- it seems this method would also need adding a 'handle' lookup to
the u32_change(); otherwise its 'handle' parameter isn't controlled
for for uniqueness, unless I miss something?

Cheers,
Jarek P.

> 
>> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
>> index 07372f6..5ad0b98 100644
>> --- a/net/sched/cls_u32.c
>> +++ b/net/sched/cls_u32.c
>> @@ -249,6 +249,9 @@ static unsigned long u32_get(struct tcf_proto *tp, u32 handle)
>>  	struct tc_u_hnode *ht;
>>  	struct tc_u_common *tp_c = tp->data;
>>  
>> +	if (!tp_c)
>> +		return 0;
>> +
>>  	if (TC_U32_HTID(handle) == TC_U32_ROOT)
>>  		ht = tp->root;
>>  	else
>> @@ -311,7 +314,6 @@ static int u32_init(struct tcf_proto *tp)
>>  	root_ht->tp_c = tp_c;
>>  
>>  	tp->root = root_ht;
>> -	tp->data = tp_c;
>>  	return 0;
>>  }
>>  
>> @@ -524,7 +526,7 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
>>  		      struct nlattr **tca,
>>  		      unsigned long *arg)
>>  {
>> -	struct tc_u_common *tp_c = tp->data;
>> +	struct tc_u_common *tp_c = tp->root->tp_c;
>>  	struct tc_u_hnode *ht;
>>  	struct tc_u_knode *n;
>>  	struct tc_u32_sel *s;
>> @@ -540,6 +542,7 @@ static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
>>  	if (err < 0)
>>  		return err;
>>  
>> +	tp->data = tp_c;
>>  	if ((n = (struct tc_u_knode*)*arg) != NULL) {
>>  		if (TC_U32_KEY(n->handle) == 0)
>>  			return -EINVAL;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Apologies for the latency.

On Wed, 2009-06-03 at 11:27 +0000, Jarek Poplawski wrote:

> After the second look I have some questions:
> - if it's really aimed to skip checking by ->get() tp's before they're
> configured in ->change(), maybe instead of using tp_c to check this it
> would be simpler to generally skip calling ->get() for newly created
> tp's?
> - otherwise the current method probably needs adding a tp_c check for
> NULL in u32_destroy()?
> - it seems this method would also need adding a 'handle' lookup to
> the u32_change(); otherwise its 'handle' parameter isn't controlled
> for for uniqueness, unless I miss something?

Lets just ignore the need for these changes since the patch fixes them
for now. I would still like to make the changes i suggested but later
with more thought put into them.

cheers,
jamal

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html