[Patch net-next 1/2] net_sched: fix errno in tcindex_set_parms()

[Patch net-next 1/2] net_sched: fix errno in tcindex_set_parms()

[Cong Wang]

When kmemdup() fails, we should return -ENOMEM.

Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 5054fae..9d78fd7 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -237,15 +237,14 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	if (err < 0)
 		return err;
 
+	err = -ENOMEM;
 	/* tcindex_data attributes must look atomic to classifier/lookup so
 	 * allocate new tcindex data and RCU assign it onto root. Keeping
 	 * perfect hash and hash pointers from old data.
 	 */
 	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
-	if (!cp) {
-		err = -ENOMEM;
+	if (!cp)
 		goto errout;
-	}
 
 	cp->mask = p->mask;
 	cp->shift = p->shift;
-- 
1.8.3.1

[Patch net-next 2/2] net_sched: fix another regression in cls_tcindex

[Cong Wang]

Clearly the following change is not expected:

	-       if (!cp.perfect && !cp.h)
	-               cp.alloc_hash = cp.hash;
	+       if (!cp->perfect && cp->h)
	+               cp->alloc_hash = cp->hash;

Fixes: commit 331b72922c5f58d48fd ("net: sched: RCU cls_tcindex")
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 9d78fd7..a18eb1b 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -303,7 +303,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 			cp->hash = DEFAULT_HASH_SIZE;
 	}
 
-	if (!cp->perfect && cp->h)
+	if (!cp->perfect && !cp->h)
 		cp->alloc_hash = cp->hash;
 
 	/* Note: this could be as restrictive as if (handle & ~(mask >> shift))
-- 
1.8.3.1

Re: [Patch net-next 2/2] net_sched: fix another regression in cls_tcindex

[John Fastabend]

On 09/25/2014 12:06 PM, Cong Wang wrote:
> Clearly the following change is not expected:
> 
> 	-       if (!cp.perfect && !cp.h)
> 	-               cp.alloc_hash = cp.hash;
> 	+       if (!cp->perfect && cp->h)
> 	+               cp->alloc_hash = cp->hash;
> 
> Fixes: commit 331b72922c5f58d48fd ("net: sched: RCU cls_tcindex")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/cls_tcindex.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index 9d78fd7..a18eb1b 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -303,7 +303,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  			cp->hash = DEFAULT_HASH_SIZE;
>  	}
>  
> -	if (!cp->perfect && cp->h)
> +	if (!cp->perfect && !cp->h)
>  		cp->alloc_hash = cp->hash;
>  
>  	/* Note: this could be as restrictive as if (handle & ~(mask >> shift))
> 

Dang thanks. I added a test to my scripts that doesn't set the
hash explicitly with TCA_TCINDEX_HASH. It looks like changes to
the filter after the initial setup could fail without this fix.

Acked-by: John Fastabend <john.r.fastabend@intel.com>

Re: [Patch net-next 2/2] net_sched: fix another regression in cls_tcindex

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 25 Sep 2014 12:06:05 -0700

> Clearly the following change is not expected:
> 
> 	-       if (!cp.perfect && !cp.h)
> 	-               cp.alloc_hash = cp.hash;
> 	+       if (!cp->perfect && cp->h)
> 	+               cp->alloc_hash = cp->hash;
> 
> Fixes: commit 331b72922c5f58d48fd ("net: sched: RCU cls_tcindex")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied.

Re: [Patch net-next 1/2] net_sched: fix errno in tcindex_set_parms()

[John Fastabend]

On 09/25/2014 12:06 PM, Cong Wang wrote:
> When kmemdup() fails, we should return -ENOMEM.
> 
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---

Acked-by: John Fastabend <john.r.fastabend@intel.com>

Thanks Cong.

>  net/sched/cls_tcindex.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index 5054fae..9d78fd7 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -237,15 +237,14 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	if (err < 0)
>  		return err;
>  
> +	err = -ENOMEM;
>  	/* tcindex_data attributes must look atomic to classifier/lookup so
>  	 * allocate new tcindex data and RCU assign it onto root. Keeping
>  	 * perfect hash and hash pointers from old data.
>  	 */
>  	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
> -	if (!cp) {
> -		err = -ENOMEM;
> +	if (!cp)
>  		goto errout;
> -	}
>  
>  	cp->mask = p->mask;
>  	cp->shift = p->shift;
> 

Re: [Patch net-next 1/2] net_sched: fix errno in tcindex_set_parms()

[John Fastabend]

On 09/25/2014 02:07 PM, John Fastabend wrote:
> On 09/25/2014 12:06 PM, Cong Wang wrote:
>> When kmemdup() fails, we should return -ENOMEM.
>>
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> ---
>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>
>
> Thanks Cong.
>

Related to this we should probably push error messages
from the dump routines correctly. If you see u32_dump()
returns a -EINVAL even though it was due to a memory
allocation failure. This is not necessarily due to my
patches though I think the error returning from dump
has always been a simple -1 value.

I'll take a look at it, unless you beat me to it, after
I finish up the percpu stats which I'm doing now.

.John

-- 
John Fastabend         Intel Corporation

Re: [Patch net-next 1/2] net_sched: fix errno in tcindex_set_parms()

[John Fastabend]

On 09/25/2014 03:57 PM, John Fastabend wrote:
> On 09/25/2014 02:07 PM, John Fastabend wrote:
>> On 09/25/2014 12:06 PM, Cong Wang wrote:
>>> When kmemdup() fails, we should return -ENOMEM.
>>>
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> ---
>>
>> Acked-by: John Fastabend <john.r.fastabend@intel.com>
>>
>> Thanks Cong.
>>
> 
> Related to this we should probably push error messages
> from the dump routines correctly. If you see u32_dump()
> returns a -EINVAL even though it was due to a memory
> allocation failure. This is not necessarily due to my
> patches though I think the error returning from dump
> has always been a simple -1 value.
> 
> I'll take a look at it, unless you beat me to it, after
> I finish up the percpu stats which I'm doing now.
> 
> .John
> 

Also just in case you hit them, there is an suspicious
rcu usage from qdisc_watchdog and another from task_csl_state()
call in cls_cgroup.

The qdisc_watchdog is due to the annotation on the qdisc
and I'm not entirely convinced the cls_cgroup hasn't been
there all along.

I'm looking into them but most likely wont figure it out
tonight as I have to take off for a bit.

Thanks,
John

Re: [Patch net-next 1/2] net_sched: fix errno in tcindex_set_parms()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 25 Sep 2014 12:06:04 -0700

> When kmemdup() fails, we should return -ENOMEM.
> 
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied.