[Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex

[Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex

[Cong Wang]

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Cong Wang (5):
  net_sched: fix an allocation bug in tcindex_set_parms()
  net_sched: fix memory leak in cls_tcindex
  net_sched: fix suspicious RCU usage in tcindex_classify()
  net_sched: use tcindex_filter_result_init()
  net_sched: clean up tcindex_set_parms()

 net/sched/cls_tcindex.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[Patch net-next 1/5] net_sched: fix an allocation bug in tcindex_set_parms()

[Cong Wang]

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 a9f4279..a02ca72 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -241,7 +241,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	 * 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);
+	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
 	if (!cp)
 		return -ENOMEM;
 
-- 
1.8.3.1

[Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex

[Cong Wang]

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 | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index a02ca72..16ec1ed 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -243,7 +243,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	 */
 	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
 	if (!cp)
-		return -ENOMEM;
+		goto errout;
 
 	cp->mask = p->mask;
 	cp->shift = p->shift;
@@ -257,6 +257,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 				      sizeof(*r) * cp->hash, GFP_KERNEL);
 		if (!cp->perfect)
 			goto errout;
+		balloc = 1;
 	}
 	cp->h = p->h;
 
@@ -285,9 +286,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	if (cp->perfect) {
 		if (!valid_perfect_hash(cp) ||
 		    cp->hash > cp->alloc_hash)
-			goto errout;
+			goto errout_alloc;
 	} else if (cp->h && cp->hash != cp->alloc_hash) {
-		goto errout;
+		goto errout_alloc;
 	}
 
 	err = -EINVAL;
@@ -314,7 +315,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	 */
 	if (cp->perfect || valid_perfect_hash(cp))
 		if (handle >= cp->alloc_hash)
-			goto errout;
+			goto errout_alloc;
 
 
 	err = -ENOMEM;
@@ -324,7 +325,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 
 			cp->perfect = kcalloc(cp->hash, sizeof(*r), GFP_KERNEL);
 			if (!cp->perfect)
-				goto errout;
+				goto errout_alloc;
 			for (i = 0; i < cp->hash; i++)
 				tcf_exts_init(&cp->perfect[i].exts,
 					      TCA_TCINDEX_ACT,
@@ -338,7 +339,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 				       GFP_KERNEL);
 
 			if (!hash)
-				goto errout;
+				goto errout_alloc;
 
 			cp->h = hash;
 			balloc = 2;
-- 
1.8.3.1

[Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify()

[Cong Wang]

This patch fixes the following kernel warning:

[   44.805900] [ INFO: suspicious RCU usage. ]
[   44.808946] 3.17.0-rc4+ #610 Not tainted
[   44.811831] -------------------------------
[   44.814873] net/sched/cls_tcindex.c:84 suspicious rcu_dereference_check() usage!

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 16ec1ed..f276de5 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -81,7 +81,7 @@ tcindex_lookup(struct tcindex_data *p, u16 key)
 static int tcindex_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 			    struct tcf_result *res)
 {
-	struct tcindex_data *p = rcu_dereference(tp->root);
+	struct tcindex_data *p = rcu_dereference_bh(tp->root);
 	struct tcindex_filter_result *f;
 	int key = (skb->tc_index & p->mask) >> p->shift;
 
-- 
1.8.3.1

[Patch net-next 4/5] net_sched: use tcindex_filter_result_init()

[Cong Wang]

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 | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index f276de5..4ec99a6 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -261,10 +261,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	}
 	cp->h = p->h;
 
-	memset(&new_filter_result, 0, sizeof(new_filter_result));
-	tcf_exts_init(&new_filter_result.exts,
-		      TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
-
+	tcindex_filter_result_init(&new_filter_result);
 	tcindex_filter_result_init(&cr);
 	if (old_r)
 		cr.res = r->res;
-- 
1.8.3.1

[Patch net-next 5/5] net_sched: clean up tcindex_set_parms()

[Cong Wang]

We can move the allocation down.

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/cls_tcindex.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 4ec99a6..688ab75 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -229,7 +229,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	struct tcindex_filter_result new_filter_result, *old_r = r;
 	struct tcindex_filter_result cr;
 	struct tcindex_data *cp, *oldp;
-	struct tcindex_filter *f = NULL; /* make gcc behave */
 	struct tcf_exts e;
 
 	tcf_exts_init(&e, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
@@ -348,12 +347,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	else
 		r = tcindex_lookup(cp, handle) ? : &new_filter_result;
 
-	if (r == &new_filter_result) {
-		f = kzalloc(sizeof(*f), GFP_KERNEL);
-		if (!f)
-			goto errout_alloc;
-	}
-
 	if (tb[TCA_TCINDEX_CLASSID]) {
 		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
 		tcf_bind_filter(tp, &cr.res, base);
@@ -374,6 +367,11 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
 	if (r == &new_filter_result) {
 		struct tcindex_filter *nfp;
 		struct tcindex_filter __rcu **fp;
+		struct tcindex_filter *f;
+
+		f = kzalloc(sizeof(*f), GFP_KERNEL);
+		if (!f)
+			goto errout_alloc;
 
 		f->key = handle;
 		f->result = new_filter_result;
-- 
1.8.3.1

Re: [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify()

[John Fastabend]

On 09/15/2014 02:06 PM, Cong Wang wrote:
> This patch fixes the following kernel warning:
> 
> [   44.805900] [ INFO: suspicious RCU usage. ]
> [   44.808946] 3.17.0-rc4+ #610 Not tainted
> [   44.811831] -------------------------------
> [   44.814873] net/sched/cls_tcindex.c:84 suspicious rcu_dereference_check() usage!
> 
> 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 16ec1ed..f276de5 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -81,7 +81,7 @@ tcindex_lookup(struct tcindex_data *p, u16 key)
>  static int tcindex_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>  			    struct tcf_result *res)
>  {
> -	struct tcindex_data *p = rcu_dereference(tp->root);
> +	struct tcindex_data *p = rcu_dereference_bh(tp->root);
>  	struct tcindex_filter_result *f;
>  	int key = (skb->tc_index & p->mask) >> p->shift;
>  
> 

Thanks for catching/fixing. I also missed cls_bpf.c case I'll send a fix
in a moment.


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

Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex

[John Fastabend]

On 09/15/2014 02:06 PM, Cong Wang wrote:
> 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 | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index a02ca72..16ec1ed 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -243,7 +243,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	 */
>  	cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>  	if (!cp)
> -		return -ENOMEM;
> +		goto errout;

but you need to set 'err = -ENOMEM' then.

>  
>  	cp->mask = p->mask;
>  	cp->shift = p->shift;
> @@ -257,6 +257,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  				      sizeof(*r) * cp->hash, GFP_KERNEL);
>  		if (!cp->perfect)
>  			goto errout;
> +		balloc = 1;

Actually can we just get rid of the balloc here altogether and
remove the checks in errout_alloc so that cp->perfect and cp->h
are freed unconditionally? They should be NULL if they are not
being used because of the kzalloc.

>  	}
>  	cp->h = p->h;
>  
> @@ -285,9 +286,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	if (cp->perfect) {
>  		if (!valid_perfect_hash(cp) ||
>  		    cp->hash > cp->alloc_hash)
> -			goto errout;
> +			goto errout_alloc;
>  	} else if (cp->h && cp->hash != cp->alloc_hash) {
> -		goto errout;
> +		goto errout_alloc;
>  	}
>  
>  	err = -EINVAL;
> @@ -314,7 +315,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	 */
>  	if (cp->perfect || valid_perfect_hash(cp))
>  		if (handle >= cp->alloc_hash)
> -			goto errout;
> +			goto errout_alloc;
>  
>  
>  	err = -ENOMEM;
> @@ -324,7 +325,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  
>  			cp->perfect = kcalloc(cp->hash, sizeof(*r), GFP_KERNEL);
>  			if (!cp->perfect)
> -				goto errout;
> +				goto errout_alloc;
>  			for (i = 0; i < cp->hash; i++)
>  				tcf_exts_init(&cp->perfect[i].exts,
>  					      TCA_TCINDEX_ACT,
> @@ -338,7 +339,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  				       GFP_KERNEL);
>  
>  			if (!hash)
> -				goto errout;
> +				goto errout_alloc;
>  
>  			cp->h = hash;
>  			balloc = 2;
> 

Re: [Patch net-next 1/5] net_sched: fix an allocation bug in tcindex_set_parms()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 15 Sep 2014 14:06:46 -0700

> Fixes: commit 331b72922c5f58d48fd ("net: sched: RCU cls_tcindex")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

This seems trivially correct, applied, thanks.

Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 15 Sep 2014 14:06:47 -0700

> Fixes: commit 331b72922c5f58d48fd ("net: sched: RCU cls_tcindex")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Please address John's feedback.

Re: [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 15 Sep 2014 14:06:48 -0700

> This patch fixes the following kernel warning:
> 
> [   44.805900] [ INFO: suspicious RCU usage. ]
> [   44.808946] 3.17.0-rc4+ #610 Not tainted
> [   44.811831] -------------------------------
> [   44.814873] net/sched/cls_tcindex.c:84 suspicious rcu_dereference_check() usage!
> 
> 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 4/5] net_sched: use tcindex_filter_result_init()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 15 Sep 2014 14:06:49 -0700

> Fixes: commit 331b72922c5f58d48fd ("net: sched: RCU cls_tcindex")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Clearly an equivalent transformation, applied, thanks.

Re: [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 15 Sep 2014 14:06:50 -0700

> We can move the allocation down.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

I'll give John time to review this.

Re: [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()

[John Fastabend]

On 09/15/2014 02:06 PM, Cong Wang wrote:
> We can move the allocation down.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/cls_tcindex.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index 4ec99a6..688ab75 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -229,7 +229,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	struct tcindex_filter_result new_filter_result, *old_r = r;
>  	struct tcindex_filter_result cr;
>  	struct tcindex_data *cp, *oldp;
> -	struct tcindex_filter *f = NULL; /* make gcc behave */
>  	struct tcf_exts e;
>  
>  	tcf_exts_init(&e, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
> @@ -348,12 +347,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	else
>  		r = tcindex_lookup(cp, handle) ? : &new_filter_result;
>  
> -	if (r == &new_filter_result) {
> -		f = kzalloc(sizeof(*f), GFP_KERNEL);
> -		if (!f)
> -			goto errout_alloc;
> -	}
> -

It was put here to avoid having logic to unwind the state when/if the
malloc fails.

>  	if (tb[TCA_TCINDEX_CLASSID]) {
>  		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
>  		tcf_bind_filter(tp, &cr.res, base);
> @@ -374,6 +367,11 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  	if (r == &new_filter_result) {
>  		struct tcindex_filter *nfp;
>  		struct tcindex_filter __rcu **fp;
> +		struct tcindex_filter *f;
> +
> +		f = kzalloc(sizeof(*f), GFP_KERNEL);
> +		if (!f)
> +			goto errout_alloc;

I don't think its safe to bail out here, you've already done
the tcf_exts_change and rcu_assign_pointer() then you free it
down below leaving tp->root corrupted?

Thanks,
John

>  
>  		f->key = handle;
>  		f->result = new_filter_result;
> 

Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex

[Cong Wang]

On Mon, Sep 15, 2014 at 2:41 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 09/15/2014 02:06 PM, Cong Wang wrote:
>> 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 | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
>> index a02ca72..16ec1ed 100644
>> --- a/net/sched/cls_tcindex.c
>> +++ b/net/sched/cls_tcindex.c
>> @@ -243,7 +243,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>>        */
>>       cp = kzalloc(sizeof(*cp), GFP_KERNEL);
>>       if (!cp)
>> -             return -ENOMEM;
>> +             goto errout;
>
> but you need to set 'err = -ENOMEM' then.

Yeah, I thought err is initialized to ENOMEM.

>
>>
>>       cp->mask = p->mask;
>>       cp->shift = p->shift;
>> @@ -257,6 +257,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>>                                     sizeof(*r) * cp->hash, GFP_KERNEL);
>>               if (!cp->perfect)
>>                       goto errout;
>> +             balloc = 1;
>
> Actually can we just get rid of the balloc here altogether and
> remove the checks in errout_alloc so that cp->perfect and cp->h
> are freed unconditionally? They should be NULL if they are not
> being used because of the kzalloc.
>

Hmm, but for cp->h which you don't duplicate if we free it we
free p->h too, which seems not expected. I think we probably
need to cp->h = kmemdup(p->h) as well.

Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex

[Cong Wang]

On Mon, Sep 15, 2014 at 3:15 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 2:41 PM, John Fastabend
> <john.r.fastabend@intel.com> wrote:
>> Actually can we just get rid of the balloc here altogether and
>> remove the checks in errout_alloc so that cp->perfect and cp->h
>> are freed unconditionally? They should be NULL if they are not
>> being used because of the kzalloc.
>>
>
> Hmm, but for cp->h which you don't duplicate if we free it we
> free p->h too, which seems not expected. I think we probably
> need to cp->h = kmemdup(p->h) as well.

I can see why you didn't use kmemdup() there, because p->h
is accessible for readers since we now use RCU read lock.
I think that means we probably have to duplicate the whole
hash table including the chained elements. But it deserves
another patch, to fix this memory leak I think we should just keep
the balloc logic.

I will just fix the missing err = -ENOMEM.

Thanks.

Re: [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()

[Cong Wang]

On Mon, Sep 15, 2014 at 3:09 PM, John Fastabend
<john.r.fastabend@intel.com> wrote:
> On 09/15/2014 02:06 PM, Cong Wang wrote:
>> -     if (r == &new_filter_result) {
>> -             f = kzalloc(sizeof(*f), GFP_KERNEL);
>> -             if (!f)
>> -                     goto errout_alloc;
>> -     }
>> -
>
> It was put here to avoid having logic to unwind the state when/if the
> malloc fails.
>

Right, some operation after that is not undoable so we have to keep it there.
So drop this one.