* [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
* [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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.