* [Patch net-next 1/5] net_sched: fix an allocation bug in tcindex_set_parms()
2014-09-15 21:06 [Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex Cong Wang
@ 2014-09-15 21:06 ` Cong Wang
2014-09-15 21:48 ` David Miller
2014-09-15 21:06 ` [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex Cong Wang
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-09-15 21:06 UTC (permalink / raw)
To: netdev; +Cc: john.fastabend, David S. Miller, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex
2014-09-15 21:06 [Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex Cong Wang
2014-09-15 21:06 ` [Patch net-next 1/5] net_sched: fix an allocation bug in tcindex_set_parms() Cong Wang
@ 2014-09-15 21:06 ` Cong Wang
2014-09-15 21:41 ` John Fastabend
2014-09-15 21:49 ` David Miller
2014-09-15 21:06 ` [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify() Cong Wang
` (2 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Cong Wang @ 2014-09-15 21:06 UTC (permalink / raw)
To: netdev; +Cc: john.fastabend, David S. Miller, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex
2014-09-15 21:06 ` [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex Cong Wang
@ 2014-09-15 21:41 ` John Fastabend
2014-09-15 22:15 ` Cong Wang
2014-09-15 21:49 ` David Miller
1 sibling, 1 reply; 17+ messages in thread
From: John Fastabend @ 2014-09-15 21:41 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: john.fastabend, David S. Miller
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;
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex
2014-09-15 21:41 ` John Fastabend
@ 2014-09-15 22:15 ` Cong Wang
2014-09-15 22:46 ` Cong Wang
0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-09-15 22:15 UTC (permalink / raw)
To: John Fastabend
Cc: Linux Kernel Network Developers, john fastabend, David S. Miller
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex
2014-09-15 22:15 ` Cong Wang
@ 2014-09-15 22:46 ` Cong Wang
0 siblings, 0 replies; 17+ messages in thread
From: Cong Wang @ 2014-09-15 22:46 UTC (permalink / raw)
To: John Fastabend
Cc: Linux Kernel Network Developers, john fastabend, David S. Miller
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex
2014-09-15 21:06 ` [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex Cong Wang
2014-09-15 21:41 ` John Fastabend
@ 2014-09-15 21:49 ` David Miller
1 sibling, 0 replies; 17+ messages in thread
From: David Miller @ 2014-09-15 21:49 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, john.fastabend
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify()
2014-09-15 21:06 [Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex Cong Wang
2014-09-15 21:06 ` [Patch net-next 1/5] net_sched: fix an allocation bug in tcindex_set_parms() Cong Wang
2014-09-15 21:06 ` [Patch net-next 2/5] net_sched: fix memory leak in cls_tcindex Cong Wang
@ 2014-09-15 21:06 ` Cong Wang
2014-09-15 21:29 ` John Fastabend
2014-09-15 21:50 ` David Miller
2014-09-15 21:06 ` [Patch net-next 4/5] net_sched: use tcindex_filter_result_init() Cong Wang
2014-09-15 21:06 ` [Patch net-next 5/5] net_sched: clean up tcindex_set_parms() Cong Wang
4 siblings, 2 replies; 17+ messages in thread
From: Cong Wang @ 2014-09-15 21:06 UTC (permalink / raw)
To: netdev; +Cc: john.fastabend, David S. Miller, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify()
2014-09-15 21:06 ` [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify() Cong Wang
@ 2014-09-15 21:29 ` John Fastabend
2014-09-15 21:50 ` David Miller
1 sibling, 0 replies; 17+ messages in thread
From: John Fastabend @ 2014-09-15 21:29 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: john.fastabend, David S. Miller
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>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify()
2014-09-15 21:06 ` [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify() Cong Wang
2014-09-15 21:29 ` John Fastabend
@ 2014-09-15 21:50 ` David Miller
1 sibling, 0 replies; 17+ messages in thread
From: David Miller @ 2014-09-15 21:50 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, john.fastabend
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Patch net-next 4/5] net_sched: use tcindex_filter_result_init()
2014-09-15 21:06 [Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex Cong Wang
` (2 preceding siblings ...)
2014-09-15 21:06 ` [Patch net-next 3/5] net_sched: fix suspicious RCU usage in tcindex_classify() Cong Wang
@ 2014-09-15 21:06 ` Cong Wang
2014-09-15 21:51 ` David Miller
2014-09-15 21:06 ` [Patch net-next 5/5] net_sched: clean up tcindex_set_parms() Cong Wang
4 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2014-09-15 21:06 UTC (permalink / raw)
To: netdev; +Cc: john.fastabend, David S. Miller, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()
2014-09-15 21:06 [Patch net-next 0/5] net_sched: some fixes and cleanup for tcindex Cong Wang
` (3 preceding siblings ...)
2014-09-15 21:06 ` [Patch net-next 4/5] net_sched: use tcindex_filter_result_init() Cong Wang
@ 2014-09-15 21:06 ` Cong Wang
2014-09-15 21:52 ` David Miller
2014-09-15 22:09 ` John Fastabend
4 siblings, 2 replies; 17+ messages in thread
From: Cong Wang @ 2014-09-15 21:06 UTC (permalink / raw)
To: netdev; +Cc: john.fastabend, David S. Miller, 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()
2014-09-15 21:06 ` [Patch net-next 5/5] net_sched: clean up tcindex_set_parms() Cong Wang
@ 2014-09-15 21:52 ` David Miller
2014-09-15 22:09 ` John Fastabend
1 sibling, 0 replies; 17+ messages in thread
From: David Miller @ 2014-09-15 21:52 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, john.fastabend
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()
2014-09-15 21:06 ` [Patch net-next 5/5] net_sched: clean up tcindex_set_parms() Cong Wang
2014-09-15 21:52 ` David Miller
@ 2014-09-15 22:09 ` John Fastabend
2014-09-15 23:39 ` Cong Wang
1 sibling, 1 reply; 17+ messages in thread
From: John Fastabend @ 2014-09-15 22:09 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: john.fastabend, David S. Miller
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;
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Patch net-next 5/5] net_sched: clean up tcindex_set_parms()
2014-09-15 22:09 ` John Fastabend
@ 2014-09-15 23:39 ` Cong Wang
0 siblings, 0 replies; 17+ messages in thread
From: Cong Wang @ 2014-09-15 23:39 UTC (permalink / raw)
To: John Fastabend
Cc: Linux Kernel Network Developers, john fastabend, David S. Miller
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.
^ permalink raw reply [flat|nested] 17+ messages in thread