[patch net] net: sched: fix memleak for chain zero

[patch net] net: sched: fix memleak for chain zero

[Jiri Pirko]

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

Re: [patch net] net: sched: fix memleak for chain zero

[Jakub Kicinski]

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>

Re: [patch net] net: sched: fix memleak for chain zero

[Cong Wang]

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

Re: [patch net] net: sched: fix memleak for chain zero

[Jiri Pirko]

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

Re: [patch net] net: sched: fix memleak for chain zero

[Cong Wang]

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.

Re: [patch net] net: sched: fix memleak for chain zero

[Jiri Pirko]

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.

Re: [patch net] net: sched: fix memleak for chain zero

[David Miller]

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.

Re: [patch net] net: sched: fix memleak for chain zero

[Cong Wang]

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!

Re: [patch net] net: sched: fix memleak for chain zero

[Jiri Pirko]

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

Re: [patch net] net: sched: fix memleak for chain zero

[Cong Wang]

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.