* [patch net] net: sched: fix memleak for chain zero
@ 2017-09-06 11:14 Jiri Pirko
2017-09-06 17:38 ` Jakub Kicinski
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jiri Pirko @ 2017-09-06 11:14 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, kubakici, mlxsw
From: Jiri Pirko <jiri@mellanox.com>
There's a memleak happening for chain 0. The thing is, chain 0 needs to
be always present, not created on demand. Therefore tcf_block_get upon
creation of block calls the tcf_chain_create function directly. The
chain is created with refcnt == 1, which is not correct in this case and
causes the memleak. So move the refcnt increment into tcf_chain_get
function even for the case when chain needs to be created.
Reported-by: Jakub Kicinski <kubakici@wp.pl>
Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6c5ea84..30ef466 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -197,7 +197,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
list_add_tail(&chain->list, &block->chain_list);
chain->block = block;
chain->index = chain_index;
- chain->refcnt = 1;
+ chain->refcnt = 0;
return chain;
}
@@ -232,15 +232,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
struct tcf_chain *chain;
list_for_each_entry(chain, &block->chain_list, list) {
- if (chain->index == chain_index) {
- chain->refcnt++;
- return chain;
- }
+ if (chain->index == chain_index)
+ goto incref;
}
- if (create)
- return tcf_chain_create(block, chain_index);
- else
- return NULL;
+ chain = create ? tcf_chain_create(block, chain_index) : NULL;
+
+incref:
+ if (chain)
+ chain->refcnt++;
+ return chain;
}
EXPORT_SYMBOL(tcf_chain_get);
--
2.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-06 11:14 [patch net] net: sched: fix memleak for chain zero Jiri Pirko
@ 2017-09-06 17:38 ` Jakub Kicinski
2017-09-06 17:40 ` Cong Wang
2017-09-08 2:18 ` David Miller
2 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2017-09-06 17:38 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, jhs, xiyou.wangcong, mlxsw
On Wed, 6 Sep 2017 13:14:19 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> There's a memleak happening for chain 0. The thing is, chain 0 needs to
> be always present, not created on demand. Therefore tcf_block_get upon
> creation of block calls the tcf_chain_create function directly. The
> chain is created with refcnt == 1, which is not correct in this case and
> causes the memleak. So move the refcnt increment into tcf_chain_get
> function even for the case when chain needs to be created.
>
> Reported-by: Jakub Kicinski <kubakici@wp.pl>
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Tested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-06 11:14 [patch net] net: sched: fix memleak for chain zero Jiri Pirko
2017-09-06 17:38 ` Jakub Kicinski
@ 2017-09-06 17:40 ` Cong Wang
2017-09-06 20:33 ` Jiri Pirko
2017-09-08 2:18 ` David Miller
2 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-09-06 17:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> There's a memleak happening for chain 0. The thing is, chain 0 needs to
> be always present, not created on demand. Therefore tcf_block_get upon
> creation of block calls the tcf_chain_create function directly. The
> chain is created with refcnt == 1, which is not correct in this case and
> causes the memleak. So move the refcnt increment into tcf_chain_get
> function even for the case when chain needs to be created.
>
Your approach could work but you just make the code even
uglier than it is now:
1. The current code is already ugly for special-casing chain 0:
if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
tcf_chain_destroy(chain);
2. With your patch, chain 0 has a different _initial_ refcnt with others.
3. Allowing an object (chain 0) exists with refcnt==0
Compare it with my patch:
1. No special-case for chain 0, the above ugly part is removed
2. Every chain is equal and created with refcnt==1
3. Any chain with refcnt==0 is destroyed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-06 17:40 ` Cong Wang
@ 2017-09-06 20:33 ` Jiri Pirko
2017-09-06 23:37 ` Cong Wang
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Pirko @ 2017-09-06 20:33 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:
>On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> There's a memleak happening for chain 0. The thing is, chain 0 needs to
>> be always present, not created on demand. Therefore tcf_block_get upon
>> creation of block calls the tcf_chain_create function directly. The
>> chain is created with refcnt == 1, which is not correct in this case and
>> causes the memleak. So move the refcnt increment into tcf_chain_get
>> function even for the case when chain needs to be created.
>>
>
>Your approach could work but you just make the code even
>uglier than it is now:
>
>1. The current code is already ugly for special-casing chain 0:
>
> if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
> tcf_chain_destroy(chain);
>
>2. With your patch, chain 0 has a different _initial_ refcnt with others.
No. Initial refcnt is the same. ! for every action that holds the chain.
So actually, it returns it back where it should be.
>
>3. Allowing an object (chain 0) exists with refcnt==0
So? That is for every chain that does not have goto_chain action
pointing at. Please read the code.
>
>Compare it with my patch:
>
>1. No special-case for chain 0, the above ugly part is removed
>
>2. Every chain is equal and created with refcnt==1
>
>3. Any chain with refcnt==0 is destroyed
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-06 20:33 ` Jiri Pirko
@ 2017-09-06 23:37 ` Cong Wang
2017-09-07 6:07 ` Jiri Pirko
0 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-09-06 23:37 UTC (permalink / raw)
To: Jiri Pirko
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
On Wed, Sep 6, 2017 at 1:33 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:
>>On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> From: Jiri Pirko <jiri@mellanox.com>
>>>
>>> There's a memleak happening for chain 0. The thing is, chain 0 needs to
>>> be always present, not created on demand. Therefore tcf_block_get upon
>>> creation of block calls the tcf_chain_create function directly. The
>>> chain is created with refcnt == 1, which is not correct in this case and
>>> causes the memleak. So move the refcnt increment into tcf_chain_get
>>> function even for the case when chain needs to be created.
>>>
>>
>>Your approach could work but you just make the code even
>>uglier than it is now:
>>
>>1. The current code is already ugly for special-casing chain 0:
>>
>> if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
>> tcf_chain_destroy(chain);
>>
>>2. With your patch, chain 0 has a different _initial_ refcnt with others.
>
> No. Initial refcnt is the same. ! for every action that holds the chain.
> So actually, it returns it back where it should be.
Not all all.
tcf_block_get() calls tcf_chain_create(, 0), after your patch
chain 0 has refcnt==0 initially.
Non-0 chain? They are created via tcf_chain_get(), aka,
refcnt==0 initially.
>
>
>>
>>3. Allowing an object (chain 0) exists with refcnt==0
>
> So? That is for every chain that does not have goto_chain action
> pointing at. Please read the code.
So you are pretending to be GC but you are apparently not.
You create all the troubles by setting yourself to believe chain 0
is special and refcnt==0 is okay. Both are wrong.
Actually the !list_empty() check is totally unnecessary too,
it is yet another place you get it wrong, you hide the race
condition in commit 744a4cf63e52 which makes it harder
to expose.
I understand you don't trust me. Look at DaveM's reaction
to your refcnt==0 madness.
Remember, refcnt can be very simple, you just want to
make it harder by abusing it or attempting to invent a GC.
I am going to update my patch (to remove all your madness)
since this is horribly wrong to me. Sorry.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-06 23:37 ` Cong Wang
@ 2017-09-07 6:07 ` Jiri Pirko
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Pirko @ 2017-09-07 6:07 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, David Miller, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
Thu, Sep 07, 2017 at 01:37:59AM CEST, xiyou.wangcong@gmail.com wrote:
>On Wed, Sep 6, 2017 at 1:33 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:
>>>On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> From: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>> There's a memleak happening for chain 0. The thing is, chain 0 needs to
>>>> be always present, not created on demand. Therefore tcf_block_get upon
>>>> creation of block calls the tcf_chain_create function directly. The
>>>> chain is created with refcnt == 1, which is not correct in this case and
>>>> causes the memleak. So move the refcnt increment into tcf_chain_get
>>>> function even for the case when chain needs to be created.
>>>>
>>>
>>>Your approach could work but you just make the code even
>>>uglier than it is now:
>>>
>>>1. The current code is already ugly for special-casing chain 0:
>>>
>>> if (--chain->refcnt == 0 && !chain->filter_chain && chain->index != 0)
>>> tcf_chain_destroy(chain);
>>>
>>>2. With your patch, chain 0 has a different _initial_ refcnt with others.
>>
>> No. Initial refcnt is the same. ! for every action that holds the chain.
>> So actually, it returns it back where it should be.
>
>Not all all.
>
>tcf_block_get() calls tcf_chain_create(, 0), after your patch
>chain 0 has refcnt==0 initially.
>
>Non-0 chain? They are created via tcf_chain_get(), aka,
>refcnt==0 initially.
And if they are created on insertion of the filter, put is caller right
away which returns the ref back to 0. As I said, Non-0 refcnt means
either rule is being inserted/removed of there is an action that holds
reference to this chain. So my patch actually fixes the behaviour making
chain 0 and other chains to behave the same.
>
>
>>
>>
>>>
>>>3. Allowing an object (chain 0) exists with refcnt==0
>>
>> So? That is for every chain that does not have goto_chain action
>> pointing at. Please read the code.
>
>So you are pretending to be GC but you are apparently not.
>
>You create all the troubles by setting yourself to believe chain 0
>is special and refcnt==0 is okay. Both are wrong.
>
>Actually the !list_empty() check is totally unnecessary too,
>it is yet another place you get it wrong, you hide the race
>condition in commit 744a4cf63e52 which makes it harder
>to expose.
>
>I understand you don't trust me. Look at DaveM's reaction
>to your refcnt==0 madness.
>
>Remember, refcnt can be very simple, you just want to
>make it harder by abusing it or attempting to invent a GC.
>
>I am going to update my patch (to remove all your madness)
>since this is horribly wrong to me. Sorry.
It is not so easy to use the refcnt also for filters, I had good reason
to relend on filter_chain list to find out if there is a rule. If you
figure out how to do it better, be my guest. I suggest you do that for
net-next and let's fix the net in the easiest way possible.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-06 11:14 [patch net] net: sched: fix memleak for chain zero Jiri Pirko
2017-09-06 17:38 ` Jakub Kicinski
2017-09-06 17:40 ` Cong Wang
@ 2017-09-08 2:18 ` David Miller
2017-09-09 18:46 ` Cong Wang
2 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2017-09-08 2:18 UTC (permalink / raw)
To: jiri; +Cc: netdev, jhs, xiyou.wangcong, kubakici, mlxsw
From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 6 Sep 2017 13:14:19 +0200
> From: Jiri Pirko <jiri@mellanox.com>
>
> There's a memleak happening for chain 0. The thing is, chain 0 needs to
> be always present, not created on demand. Therefore tcf_block_get upon
> creation of block calls the tcf_chain_create function directly. The
> chain is created with refcnt == 1, which is not correct in this case and
> causes the memleak. So move the refcnt increment into tcf_chain_get
> function even for the case when chain needs to be created.
>
> Reported-by: Jakub Kicinski <kubakici@wp.pl>
> Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
This doesn't apply cleanly any more, please respin.
Thanks Jiri.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-08 2:18 ` David Miller
@ 2017-09-09 18:46 ` Cong Wang
2017-09-10 14:03 ` Jiri Pirko
0 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2017-09-09 18:46 UTC (permalink / raw)
To: David Miller
Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
On Thu, Sep 7, 2017 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
>
> This doesn't apply cleanly any more, please respin.
>
Sigh, you applied this patch despite of strong objections from me.
I seriously doubt your tastes now, David. Fine, this code does not
deserve a good taste at all, let bugs stay there.
Good luck!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-09 18:46 ` Cong Wang
@ 2017-09-10 14:03 ` Jiri Pirko
2017-09-11 23:43 ` Cong Wang
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Pirko @ 2017-09-10 14:03 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Linux Kernel Network Developers, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
Sat, Sep 09, 2017 at 08:46:42PM CEST, xiyou.wangcong@gmail.com wrote:
>On Thu, Sep 7, 2017 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
>>
>> This doesn't apply cleanly any more, please respin.
>>
>
>Sigh, you applied this patch despite of strong objections from me.
>
>I seriously doubt your tastes now, David. Fine, this code does not
>deserve a good taste at all, let bugs stay there.
Actually, the reported bug is resolved by this patch. I like your
approach Cong, we can make it work and follow-up on this patch.
Please stay calm and be patient...
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch net] net: sched: fix memleak for chain zero
2017-09-10 14:03 ` Jiri Pirko
@ 2017-09-11 23:43 ` Cong Wang
0 siblings, 0 replies; 10+ messages in thread
From: Cong Wang @ 2017-09-11 23:43 UTC (permalink / raw)
To: Jiri Pirko
Cc: David Miller, Linux Kernel Network Developers, Jamal Hadi Salim,
Jakub Kicinski, mlxsw
On Sun, Sep 10, 2017 at 7:03 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sat, Sep 09, 2017 at 08:46:42PM CEST, xiyou.wangcong@gmail.com wrote:
>>On Thu, Sep 7, 2017 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
>>>
>>> This doesn't apply cleanly any more, please respin.
>>>
>>
>>Sigh, you applied this patch despite of strong objections from me.
>>
>>I seriously doubt your tastes now, David. Fine, this code does not
>>deserve a good taste at all, let bugs stay there.
>
> Actually, the reported bug is resolved by this patch. I like your
> approach Cong, we can make it work and follow-up on this patch.
> Please stay calm and be patient...
OK. I already rebased my patches and sent v3.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-11 23:43 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 11:14 [patch net] net: sched: fix memleak for chain zero Jiri Pirko
2017-09-06 17:38 ` Jakub Kicinski
2017-09-06 17:40 ` Cong Wang
2017-09-06 20:33 ` Jiri Pirko
2017-09-06 23:37 ` Cong Wang
2017-09-07 6:07 ` Jiri Pirko
2017-09-08 2:18 ` David Miller
2017-09-09 18:46 ` Cong Wang
2017-09-10 14:03 ` Jiri Pirko
2017-09-11 23:43 ` Cong Wang
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).