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

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

commit 459d5f626da75573e985a7197b0919c3b143146c
Author: John Fastabend [off-list ref]
Date:   Fri Sep 12 20:08:47 2014 -0700

    net: sched: make cls_u32 per cpu

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

Signed-off-by: John Fastabend <redacted>
---
 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;
 			}

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

Caught by kbuild bot,

quoted

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

quoted

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

quoted

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

quoted

net/sched/cls_u32.c:359:18: sparse: dereference of noderef expression

Signed-off-by: John Fastabend <redacted>
---
 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..19b2808 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_assign_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);

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 <redacted>
Signed-off-by: John Fastabend <redacted>
---
 net/sched/cls_cgroup.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 3b75487..52b7900 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c

@@ -127,16 +127,16 @@ 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;
+		goto errout;
 
 	tcf_exts_change(tp, &new->exts, &e);
 	tcf_em_tree_change(tp, &new->ematches, &t);

@@ -145,6 +145,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)

On Mon, Sep 15, 2014 at 7:48 PM, John Fastabend
[off-list ref] wrote:

I think you need to call tcf_exts_destroy() too after tcf_exts_validate(),
while you are on it. :)

On 09/15/2014 08:04 PM, Cong Wang wrote:

On Mon, Sep 15, 2014 at 7:48 PM, John Fastabend
[off-list ref] wrote:

quoted

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 3b75487..52b7900 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c

@@ -127,16 +127,16 @@ 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;
+               goto errout;

I think you need to call tcf_exts_destroy() too after tcf_exts_validate(),
while you are on it. :)

Yep, and did a quick audit looks like its handled correctly in the other
classifiers.

Also there is a needed fix for cls_fw I'll include with the update for this patch.

.John

--
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 09/15/2014 07:48 PM, John Fastabend wrote:

also believe this should be RCU_INIT_POINTER() the rcu_assign_pointer() to attach
n to the hash table happens below so there are no concurrent readers until after
the assign.

however while reviewing this I realize there is a 'copy'/'update' pattern I
missed in the u32_set_parms case when the classid and ifindex is changed. So
a fix coming for that shortly.

 	n->handle = handle;
 	n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;
 	tcf_exts_init(&n->exts, TCA_U32_ACT, TCA_U32_POLICE);

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