* [net-next PATCH 1/3] net: sched: fix unsued cpu variable
@ 2014-09-16 2:47 John Fastabend
2014-09-16 2:48 ` [net-next PATCH 2/3] net: sched: cls_u32 add missing rcu_assign_pointer and annotation John Fastabend
2014-09-16 2:48 ` [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' John Fastabend
0 siblings, 2 replies; 6+ messages in thread
From: John Fastabend @ 2014-09-16 2:47 UTC (permalink / raw)
To: xiyou.wangcong, davem, eric.dumazet; +Cc: netdev, jhs
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 <john.fastabend@gmail.com>
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 <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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* [net-next PATCH 2/3] net: sched: cls_u32 add missing rcu_assign_pointer and annotation 2014-09-16 2:47 [net-next PATCH 1/3] net: sched: fix unsued cpu variable John Fastabend @ 2014-09-16 2:48 ` John Fastabend 2014-09-16 6:25 ` John Fastabend 2014-09-16 2:48 ` [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' John Fastabend 1 sibling, 1 reply; 6+ messages in thread From: John Fastabend @ 2014-09-16 2:48 UTC (permalink / raw) To: xiyou.wangcong, davem, eric.dumazet; +Cc: netdev, jhs 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..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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net-next PATCH 2/3] net: sched: cls_u32 add missing rcu_assign_pointer and annotation 2014-09-16 2:48 ` [net-next PATCH 2/3] net: sched: cls_u32 add missing rcu_assign_pointer and annotation John Fastabend @ 2014-09-16 6:25 ` John Fastabend 0 siblings, 0 replies; 6+ messages in thread From: John Fastabend @ 2014-09-16 6:25 UTC (permalink / raw) To: John Fastabend, xiyou.wangcong, davem, eric.dumazet; +Cc: netdev, jhs On 09/15/2014 07:48 PM, John Fastabend wrote: > 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..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); 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' 2014-09-16 2:47 [net-next PATCH 1/3] net: sched: fix unsued cpu variable John Fastabend 2014-09-16 2:48 ` [net-next PATCH 2/3] net: sched: cls_u32 add missing rcu_assign_pointer and annotation John Fastabend @ 2014-09-16 2:48 ` John Fastabend 2014-09-16 3:04 ` Cong Wang 1 sibling, 1 reply; 6+ messages in thread From: John Fastabend @ 2014-09-16 2:48 UTC (permalink / raw) To: xiyou.wangcong, davem, eric.dumazet; +Cc: netdev, jhs 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 | 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) ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' 2014-09-16 2:48 ` [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' John Fastabend @ 2014-09-16 3:04 ` Cong Wang 2014-09-16 3:29 ` John Fastabend 0 siblings, 1 reply; 6+ messages in thread From: Cong Wang @ 2014-09-16 3:04 UTC (permalink / raw) To: John Fastabend Cc: Cong Wang, David Miller, Eric Dumazet, netdev, Jamal Hadi Salim On Mon, Sep 15, 2014 at 7:48 PM, John Fastabend <john.fastabend@gmail.com> wrote: > 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. :) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' 2014-09-16 3:04 ` Cong Wang @ 2014-09-16 3:29 ` John Fastabend 0 siblings, 0 replies; 6+ messages in thread From: John Fastabend @ 2014-09-16 3:29 UTC (permalink / raw) To: Cong Wang, John Fastabend Cc: Cong Wang, David Miller, Eric Dumazet, netdev, Jamal Hadi Salim On 09/15/2014 08:04 PM, Cong Wang wrote: > On Mon, Sep 15, 2014 at 7:48 PM, John Fastabend > <john.fastabend@gmail.com> wrote: >> 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-16 6:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-16 2:47 [net-next PATCH 1/3] net: sched: fix unsued cpu variable John Fastabend 2014-09-16 2:48 ` [net-next PATCH 2/3] net: sched: cls_u32 add missing rcu_assign_pointer and annotation John Fastabend 2014-09-16 6:25 ` John Fastabend 2014-09-16 2:48 ` [net-next PATCH 3/3] net: sched: cls_cgroup fix possible memory leak of 'new' John Fastabend 2014-09-16 3:04 ` Cong Wang 2014-09-16 3:29 ` John Fastabend
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).