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

From: Jiri Pirko <redacted>

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 <redacted>
Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Jiri Pirko <redacted>
---
 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

On Wed,  6 Sep 2017 13:14:19 +0200, Jiri Pirko wrote:

From: Jiri Pirko <redacted>

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 <redacted>
Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Jiri Pirko <redacted>

Tested-by: Jakub Kicinski <redacted>

On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko [off-list ref] wrote:

From: Jiri Pirko <redacted>

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

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 [off-list ref] wrote:

quoted

From: Jiri Pirko <redacted>

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

On Wed, Sep 6, 2017 at 1:33 PM, Jiri Pirko [off-list ref] wrote:

Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:

quoted

On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko [off-list ref] wrote:

quoted

From: Jiri Pirko <redacted>

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.

quoted

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.

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 [off-list ref] wrote:

quoted

Wed, Sep 06, 2017 at 07:40:02PM CEST, xiyou.wangcong@gmail.com wrote:

quoted

On Wed, Sep 6, 2017 at 4:14 AM, Jiri Pirko [off-list ref] wrote:

quoted

From: Jiri Pirko <redacted>

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.

quoted

quoted

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.

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed,  6 Sep 2017 13:14:19 +0200

From: Jiri Pirko <redacted>

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 <redacted>
Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
Signed-off-by: Jiri Pirko <redacted>

This doesn't apply cleanly any more, please respin.

Thanks Jiri.

On Thu, Sep 7, 2017 at 7:18 PM, David Miller [off-list ref] 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!

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 [off-list ref] wrote:

quoted

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

On Sun, Sep 10, 2017 at 7:03 AM, Jiri Pirko [off-list ref] wrote:

Sat, Sep 09, 2017 at 08:46:42PM CEST, xiyou.wangcong@gmail.com wrote:

quoted

On Thu, Sep 7, 2017 at 7:18 PM, David Miller [off-list ref] wrote:

quoted

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.