[net-next PATCH v2 1/4] net: sched: fix unsued cpu variable

[net-next PATCH v2 1/4] net: sched: fix unsued cpu variable

[John Fastabend]

kbuild test robot reported an unused variable cpu in cls_u32.c
after the patch below. This happens when PERF and MARK config
variables are disabled

Fix this is to use separate variables for perf and mark
and define the cpu variable inside the ifdef logic.

'commit 459d5f626da7 ("net: sched: make cls_u32 per cpu")'

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
 net/sched/cls_u32.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 5ed5ac4..8cffe5a 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -788,8 +788,8 @@ static int u32_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	} else {
 #ifdef CONFIG_CLS_U32_PERF
 		struct tc_u32_pcnt *gpf;
-#endif
 		int cpu;
+#endif
 
 		if (nla_put(skb, TCA_U32_SEL,
 			    sizeof(n->sel) + n->sel.nkeys*sizeof(struct tc_u32_key),
@@ -816,9 +816,10 @@ static int u32_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 			struct tc_u32_mark mark = {.val = n->val,
 						   .mask = n->mask,
 						   .success = 0};
+			int cpum;
 
-			for_each_possible_cpu(cpu) {
-				__u32 cnt = *per_cpu_ptr(n->pcpu_success, cpu);
+			for_each_possible_cpu(cpum) {
+				__u32 cnt = *per_cpu_ptr(n->pcpu_success, cpum);
 
 				mark.success += cnt;
 			}

[net-next PATCH v2 2/4] net: sched: cls_u32 add missing rcu_assign_pointer and annotation

[John Fastabend]

Add missing rcu_assign_pointer and missing  annotation for ht_up
in cls_u32.c

Caught by kbuild bot,

>> net/sched/cls_u32.c:378:36: sparse: incorrect type in initializer (different address spaces)
   net/sched/cls_u32.c:378:36:    expected struct tc_u_hnode *ht
   net/sched/cls_u32.c:378:36:    got struct tc_u_hnode [noderef] <asn:4>*ht_up
>> net/sched/cls_u32.c:610:54: sparse: incorrect type in argument 4 (different address spaces)
   net/sched/cls_u32.c:610:54:    expected struct tc_u_hnode *ht
   net/sched/cls_u32.c:610:54:    got struct tc_u_hnode [noderef] <asn:4>*ht_up
>> net/sched/cls_u32.c:684:18: sparse: incorrect type in assignment (different address spaces)
   net/sched/cls_u32.c:684:18:    expected struct tc_u_hnode [noderef] <asn:4>*ht_up
   net/sched/cls_u32.c:684:18:    got struct tc_u_hnode *[assigned] ht
>> net/sched/cls_u32.c:359:18: sparse: dereference of noderef expression

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
 net/sched/cls_u32.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 8cffe5a..eceeb04 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -375,7 +375,7 @@ static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
 {
 	struct tc_u_knode __rcu **kp;
 	struct tc_u_knode *pkp;
-	struct tc_u_hnode *ht = key->ht_up;
+	struct tc_u_hnode *ht = rtnl_dereference(key->ht_up);
 
 	if (ht) {
 		kp = &ht->ht[TC_U32_HASH(key->handle)];
@@ -607,7 +607,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		if (TC_U32_KEY(n->handle) == 0)
 			return -EINVAL;
 
-		return u32_set_parms(net, tp, base, n->ht_up, n, tb,
+		return u32_set_parms(net, tp, base,
+				     rtnl_dereference(n->ht_up), n, tb,
 				     tca[TCA_RATE], ovr);
 	}
 
@@ -681,7 +682,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 #endif
 
 	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
-	n->ht_up = ht;
+	RCU_INIT_POINTER(n->ht_up, ht);
 	n->handle = handle;
 	n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
 	tcf_exts_init(&n->exts, TCA_U32_ACT, TCA_U32_POLICE);

[net-next PATCH v2 3/4] net: sched: cls_cgroup fix possible memory leak of 'new'

[John Fastabend]

tree:   git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head:   54996b529ab70ca1d6f40677cd2698c4f7127e87
commit: c7953ef23042b7c4fc2be5ecdd216aacff6df5eb [625/646] net: sched: cls_cgroup use RCU

net/sched/cls_cgroup.c:130 cls_cgroup_change() warn: possible memory leak of 'new'
net/sched/cls_cgroup.c:135 cls_cgroup_change() warn: possible memory leak of 'new'
net/sched/cls_cgroup.c:139 cls_cgroup_change() warn: possible memory leak of 'new'

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
 net/sched/cls_cgroup.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 3b75487..10c7ffd 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -127,16 +127,18 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 	err = nla_parse_nested(tb, TCA_CGROUP_MAX, tca[TCA_OPTIONS],
 			       cgroup_policy);
 	if (err < 0)
-		return err;
+		goto errout;
 
 	tcf_exts_init(&e, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
 	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, ovr);
 	if (err < 0)
-		return err;
+		goto errout;
 
 	err = tcf_em_tree_validate(tp, tb[TCA_CGROUP_EMATCHES], &t);
-	if (err < 0)
-		return err;
+	if (err < 0) {
+		tcf_exts_destroy(tp, &e);
+		goto errout;
+	}
 
 	tcf_exts_change(tp, &new->exts, &e);
 	tcf_em_tree_change(tp, &new->ematches, &t);
@@ -145,6 +147,9 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 	if (head)
 		call_rcu(&head->rcu, cls_cgroup_destroy_rcu);
 	return 0;
+errout:
+	kfree(new);
+	return err;
 }
 
 static void cls_cgroup_destroy(struct tcf_proto *tp)

[net-next PATCH v2 4/4] net: sched: cls_fw: add missing tcf_exts_init call in fw_change()

[John Fastabend]

When allocating a new structure we also need to call tcf_exts_init
to initialize exts.

A follow up patch might be in order to remove some of this code
and do tcf_exts_assign(). With this we could remove the
tcf_exts_init/tcf_exts_change pattern for some of the classifiers.
As part of the future tcf_actions RCU series this will need to be
done. For now fix the call here.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
 net/sched/cls_fw.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 006b45a..2650285 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -264,6 +264,8 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 #endif /* CONFIG_NET_CLS_IND */
 		fnew->tp = f->tp;
 
+		tcf_exts_init(&fnew->exts, TCA_FW_ACT, TCA_FW_POLICE);
+
 		err = fw_change_attrs(net, tp, fnew, tb, tca, base, ovr);
 		if (err < 0) {
 			kfree(fnew);

Re: [net-next PATCH v2 1/4] net: sched: fix unsued cpu variable

[Cong Wang]

On Mon, Sep 15, 2014 at 11:30 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> kbuild test robot reported an unused variable cpu in cls_u32.c
> after the patch below. This happens when PERF and MARK config
> variables are disabled
>
> Fix this is to use separate variables for perf and mark
> and define the cpu variable inside the ifdef logic.
>
> 'commit 459d5f626da7 ("net: sched: make cls_u32 per cpu")'


Please use the Fixes: tag.


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

Other than that,

Acked-by: Cong Wang <cwang@twopensource.com>

Thanks!

Re: [net-next PATCH v2 3/4] net: sched: cls_cgroup fix possible memory leak of 'new'

[Cong Wang]

On Mon, Sep 15, 2014 at 11:31 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
> head:   54996b529ab70ca1d6f40677cd2698c4f7127e87
> commit: c7953ef23042b7c4fc2be5ecdd216aacff6df5eb [625/646] net: sched: cls_cgroup use RCU
>
> net/sched/cls_cgroup.c:130 cls_cgroup_change() warn: possible memory leak of 'new'
> net/sched/cls_cgroup.c:135 cls_cgroup_change() warn: possible memory leak of 'new'
> net/sched/cls_cgroup.c:139 cls_cgroup_change() warn: possible memory leak of 'new'
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Fixes: commit: c7953ef23042b7c4fc2be5ecdd216aac ("net: sched:
cls_cgroup use RCU")
Acked-by: Cong Wang <cwang@twopensource.com>

Re: [net-next PATCH v2 4/4] net: sched: cls_fw: add missing tcf_exts_init call in fw_change()

[Cong Wang]

On Mon, Sep 15, 2014 at 11:31 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> When allocating a new structure we also need to call tcf_exts_init
> to initialize exts.
>
> A follow up patch might be in order to remove some of this code
> and do tcf_exts_assign(). With this we could remove the
> tcf_exts_init/tcf_exts_change pattern for some of the classifiers.
> As part of the future tcf_actions RCU series this will need to be
> done. For now fix the call here.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Fixes: commit e35a8ee5993ba81fd6c0 ("net: sched: fw use RCU")
Acked-by: Cong Wang <cwang@twopensource.com>

Re: [net-next PATCH v2 1/4] net: sched: fix unsued cpu variable

[David Miller]

From: John Fastabend <john.fastabend@gmail.com>
Date: Mon, 15 Sep 2014 23:30:26 -0700

> kbuild test robot reported an unused variable cpu in cls_u32.c
> after the patch below. This happens when PERF and MARK config
> variables are disabled
> 
> Fix this is to use separate variables for perf and mark
> and define the cpu variable inside the ifdef logic.
> 
> 'commit 459d5f626da7 ("net: sched: make cls_u32 per cpu")'
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.

Re: [net-next PATCH v2 2/4] net: sched: cls_u32 add missing rcu_assign_pointer and annotation

[David Miller]

From: John Fastabend <john.fastabend@gmail.com>
Date: Mon, 15 Sep 2014 23:30:49 -0700

> Add missing rcu_assign_pointer and missing  annotation for ht_up
> in cls_u32.c
> 
> Caught by kbuild bot,
...
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.

Re: [net-next PATCH v2 3/4] net: sched: cls_cgroup fix possible memory leak of 'new'

[David Miller]

From: John Fastabend <john.fastabend@gmail.com>
Date: Mon, 15 Sep 2014 23:31:17 -0700

> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
> head:   54996b529ab70ca1d6f40677cd2698c4f7127e87
> commit: c7953ef23042b7c4fc2be5ecdd216aacff6df5eb [625/646] net: sched: cls_cgroup use RCU
> 
> net/sched/cls_cgroup.c:130 cls_cgroup_change() warn: possible memory leak of 'new'
> net/sched/cls_cgroup.c:135 cls_cgroup_change() warn: possible memory leak of 'new'
> net/sched/cls_cgroup.c:139 cls_cgroup_change() warn: possible memory leak of 'new'
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.

Re: [net-next PATCH v2 4/4] net: sched: cls_fw: add missing tcf_exts_init call in fw_change()

[David Miller]

From: John Fastabend <john.fastabend@gmail.com>
Date: Mon, 15 Sep 2014 23:31:42 -0700

> When allocating a new structure we also need to call tcf_exts_init
> to initialize exts.
> 
> A follow up patch might be in order to remove some of this code
> and do tcf_exts_assign(). With this we could remove the
> tcf_exts_init/tcf_exts_change pattern for some of the classifiers.
> As part of the future tcf_actions RCU series this will need to be
> done. For now fix the call here.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.

Re: [net-next PATCH v2 4/4] net: sched: cls_fw: add missing tcf_exts_init call in fw_change()

[John Fastabend]

On 09/16/2014 01:00 PM, David Miller wrote:
> From: John Fastabend <john.fastabend@gmail.com>
> Date: Mon, 15 Sep 2014 23:31:42 -0700
> 
>> When allocating a new structure we also need to call tcf_exts_init
>> to initialize exts.
>>
>> A follow up patch might be in order to remove some of this code
>> and do tcf_exts_assign(). With this we could remove the
>> tcf_exts_init/tcf_exts_change pattern for some of the classifiers.
>> As part of the future tcf_actions RCU series this will need to be
>> done. For now fix the call here.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> 
> Applied.

Great thanks. A couple more patches for cls_u32 coming next. I'm
fairly sure I broke the selector keys usage and the change filter
case is not completely RCU safe yet depending on the netlink
attributes.

Thanks!
John