[net PATCH 0/2] ipv4: Fix memory leaks and reference issues in fib

STALE3549d

14 messages, 4 authors, 2016-11-16 · open the first message on its own page

[net PATCH 0/2] ipv4: Fix memory leaks and reference issues in fib

From: Alexander Duyck <hidden>
Date: 2016-11-15 16:47:14 

This series fixes one major issue and one minor issue in the fib tables.

The major issue is that we had lost the functionality that was flushing the
local table entries from main after we had unmerged the two tries.  In
order to regain the functionality I have performed a partial revert and
then moved the functionality for flushing the external entries from main
into fib_unmerge.

The minor issue was a memory leak that could occur in the event that we
weren't able to add an alias to the local trie resulting in the fib alias
being leaked.

---

Alexander Duyck (2):
      ipv4: Restore fib_trie_flush_external function and fix call ordering
      ipv4: Fix memory leak in exception case for splitting tries


 include/net/ip_fib.h    |    1 +
 net/ipv4/fib_frontend.c |   20 ++++++++++----
 net/ipv4/fib_trie.c     |   69 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 84 insertions(+), 6 deletions(-)

[net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Alexander Duyck <hidden>
Date: 2016-11-15 16:47:20 

The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.

Fixes: 347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
Reported-by: Eric Dumazet <edumazet@google.com>
Cc: Jiri Pirko <redacted>
Signed-off-by: Alexander Duyck <redacted>
---
 include/net/ip_fib.h    |    1 +
 net/ipv4/fib_frontend.c |   20 +++++++++++---
 net/ipv4/fib_trie.c     |   65 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index b9314b4..f390c3b 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -243,6 +243,7 @@ int fib_table_dump(struct fib_table *table, struct sk_buff *skb,
 		   struct netlink_callback *cb);
 int fib_table_flush(struct net *net, struct fib_table *table);
 struct fib_table *fib_trie_unmerge(struct fib_table *main_tb);
+void fib_table_flush_external(struct fib_table *table);
 void fib_free_table(struct fib_table *tb);
 
 #ifndef CONFIG_IP_MULTIPLE_TABLES
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index c3b8047..161fc0f 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -151,7 +151,7 @@ static void fib_replace_table(struct net *net, struct fib_table *old,
 
 int fib_unmerge(struct net *net)
 {
-	struct fib_table *old, *new;
+	struct fib_table *old, *new, *main_table;
 
 	/* attempt to fetch local table if it has been allocated */
 	old = fib_get_table(net, RT_TABLE_LOCAL);
@@ -162,11 +162,21 @@ int fib_unmerge(struct net *net)
 	if (!new)
 		return -ENOMEM;
 
+	/* table is already unmerged */
+	if (new == old)
+		return 0;
+
 	/* replace merged table with clean table */
-	if (new != old) {
-		fib_replace_table(net, old, new);
-		fib_free_table(old);
-	}
+	fib_replace_table(net, old, new);
+	fib_free_table(old);
+
+	/* attempt to fetch main table if it has been allocated */
+	main_table = fib_get_table(net, RT_TABLE_MAIN);
+	if (!main_table)
+		return 0;
+
+	/* flush local entries from main table */
+	fib_table_flush_external(main_table);
 
 	return 0;
 }
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 4cff74d..735edc9 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1760,6 +1760,71 @@ struct fib_table *fib_trie_unmerge(struct fib_table *oldtb)
 	return NULL;
 }
 
+/* Caller must hold RTNL */
+void fib_table_flush_external(struct fib_table *tb)
+{
+	struct trie *t = (struct trie *)tb->tb_data;
+	struct key_vector *pn = t->kv;
+	unsigned long cindex = 1;
+	struct hlist_node *tmp;
+	struct fib_alias *fa;
+
+	/* walk trie in reverse order */
+	for (;;) {
+		unsigned char slen = 0;
+		struct key_vector *n;
+
+		if (!(cindex--)) {
+			t_key pkey = pn->key;
+
+			/* cannot resize the trie vector */
+			if (IS_TRIE(pn))
+				break;
+
+			/* resize completed node */
+			pn = resize(t, pn);
+			cindex = get_index(pkey, pn);
+
+			continue;
+		}
+
+		/* grab the next available node */
+		n = get_child(pn, cindex);
+		if (!n)
+			continue;
+
+		if (IS_TNODE(n)) {
+			/* record pn and cindex for leaf walking */
+			pn = n;
+			cindex = 1ul << n->bits;
+
+			continue;
+		}
+
+		hlist_for_each_entry_safe(fa, tmp, &n->leaf, fa_list) {
+			/* if alias was cloned to local then we just
+			 * need to remove the local copy from main
+			 */
+			if (tb->tb_id != fa->tb_id) {
+				hlist_del_rcu(&fa->fa_list);
+				alias_free_mem_rcu(fa);
+				continue;
+			}
+
+			/* record local slen */
+			slen = fa->fa_slen;
+		}
+
+		/* update leaf slen */
+		n->slen = slen;
+
+		if (hlist_empty(&n->leaf)) {
+			put_child_root(pn, n->key, NULL);
+			node_free(n);
+		}
+	}
+}
+
 /* Caller must hold RTNL. */
 int fib_table_flush(struct net *net, struct fib_table *tb)
 {

[net PATCH 2/2] ipv4: Fix memory leak in exception case for splitting tries

From: Alexander Duyck <hidden>
Date: 2016-11-15 16:47:25 

Fix a small memory leak that can occur where we leak a fib_alias in the
event of us not being able to insert it into the local table.

Fixes: 0ddcf43d5d4a0 ("ipv4: FIB Local/MAIN table collapse")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Alexander Duyck <redacted>
---
 net/ipv4/fib_trie.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 735edc9..026f309 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1743,8 +1743,10 @@ struct fib_table *fib_trie_unmerge(struct fib_table *oldtb)
 				local_l = fib_find_node(lt, &local_tp, l->key);
 
 			if (fib_insert_alias(lt, local_tp, local_l, new_fa,
-					     NULL, l->key))
+					     NULL, l->key)) {
+				kmem_cache_free(fn_alias_kmem, new_fa);
 				goto out;
+			}
 		}
 
 		/* stop loop if key wrapped back to 0 */

Re: [net PATCH 2/2] ipv4: Fix memory leak in exception case for splitting tries

From: Eric Dumazet <hidden>
Date: 2016-11-15 18:24:12 

On Tue, 2016-11-15 at 05:46 -0500, Alexander Duyck wrote:
Fix a small memory leak that can occur where we leak a fib_alias in the
event of us not being able to insert it into the local table.

Fixes: 0ddcf43d5d4a0 ("ipv4: FIB Local/MAIN table collapse")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Alexander Duyck <redacted>
---
 net/ipv4/fib_trie.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Acked-by: Eric Dumazet <edumazet@google.com>

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Eric Dumazet <hidden>
Date: 2016-11-15 18:30:15 

On Tue, 2016-11-15 at 05:46 -0500, Alexander Duyck wrote:
The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.

Fixes: 347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
Reported-by: Eric Dumazet <edumazet@google.com>
Cc: Jiri Pirko <redacted>
Signed-off-by: Alexander Duyck <redacted>
---

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks for sorting this out Alex !

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Jiri Pirko <jiri@resnulli.us>
Date: 2016-11-15 19:51:10 

Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")


?
quoted hunk
Fixes: 347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
Reported-by: Eric Dumazet <edumazet@google.com>
Cc: Jiri Pirko <redacted>
Signed-off-by: Alexander Duyck <redacted>
---
include/net/ip_fib.h    |    1 +
net/ipv4/fib_frontend.c |   20 +++++++++++---
net/ipv4/fib_trie.c     |   65 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index b9314b4..f390c3b 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -243,6 +243,7 @@ int fib_table_dump(struct fib_table *table, struct sk_buff *skb,
		   struct netlink_callback *cb);
int fib_table_flush(struct net *net, struct fib_table *table);
struct fib_table *fib_trie_unmerge(struct fib_table *main_tb);
+void fib_table_flush_external(struct fib_table *table);
void fib_free_table(struct fib_table *tb);

#ifndef CONFIG_IP_MULTIPLE_TABLES
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index c3b8047..161fc0f 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -151,7 +151,7 @@ static void fib_replace_table(struct net *net, struct fib_table *old,

int fib_unmerge(struct net *net)
{
-	struct fib_table *old, *new;
+	struct fib_table *old, *new, *main_table;

	/* attempt to fetch local table if it has been allocated */
	old = fib_get_table(net, RT_TABLE_LOCAL);
@@ -162,11 +162,21 @@ int fib_unmerge(struct net *net)
	if (!new)
		return -ENOMEM;

+	/* table is already unmerged */
+	if (new == old)
+		return 0;
+
	/* replace merged table with clean table */
-	if (new != old) {
-		fib_replace_table(net, old, new);
-		fib_free_table(old);
-	}
+	fib_replace_table(net, old, new);
+	fib_free_table(old);
+
+	/* attempt to fetch main table if it has been allocated */
+	main_table = fib_get_table(net, RT_TABLE_MAIN);
+	if (!main_table)
+		return 0;
+
+	/* flush local entries from main table */
+	fib_table_flush_external(main_table);

	return 0;
}
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 4cff74d..735edc9 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1760,6 +1760,71 @@ struct fib_table *fib_trie_unmerge(struct fib_table *oldtb)
	return NULL;
}

+/* Caller must hold RTNL */
+void fib_table_flush_external(struct fib_table *tb)
+{
+	struct trie *t = (struct trie *)tb->tb_data;
+	struct key_vector *pn = t->kv;
+	unsigned long cindex = 1;
+	struct hlist_node *tmp;
+	struct fib_alias *fa;
+
+	/* walk trie in reverse order */
+	for (;;) {
+		unsigned char slen = 0;
+		struct key_vector *n;
+
+		if (!(cindex--)) {
+			t_key pkey = pn->key;
+
+			/* cannot resize the trie vector */
+			if (IS_TRIE(pn))
+				break;
+
+			/* resize completed node */
+			pn = resize(t, pn);
+			cindex = get_index(pkey, pn);
+
+			continue;
+		}
+
+		/* grab the next available node */
+		n = get_child(pn, cindex);
+		if (!n)
+			continue;
+
+		if (IS_TNODE(n)) {
+			/* record pn and cindex for leaf walking */
+			pn = n;
+			cindex = 1ul << n->bits;
+
+			continue;
+		}
+
+		hlist_for_each_entry_safe(fa, tmp, &n->leaf, fa_list) {
+			/* if alias was cloned to local then we just
+			 * need to remove the local copy from main
+			 */
+			if (tb->tb_id != fa->tb_id) {
+				hlist_del_rcu(&fa->fa_list);
+				alias_free_mem_rcu(fa);
+				continue;
+			}
+
+			/* record local slen */
+			slen = fa->fa_slen;
+		}
+
+		/* update leaf slen */
+		n->slen = slen;
+
+		if (hlist_empty(&n->leaf)) {
+			put_child_root(pn, n->key, NULL);
+			node_free(n);
+		}
+	}
+}
+
/* Caller must hold RTNL. */
int fib_table_flush(struct net *net, struct fib_table *tb)
{

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Duyck, Alexander H <hidden>
Date: 2016-11-15 20:29:11 

On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted
The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.

- Alex

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Jiri Pirko <jiri@resnulli.us>
Date: 2016-11-15 20:31:43 

Tue, Nov 15, 2016 at 09:29:09PM CET, alexander.h.duyck@intel.com wrote:
On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted
The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.
Okay. But please name the fuction differently, as it does not flush
external. Thanks!

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Duyck, Alexander H <hidden>
Date: 2016-11-15 20:49:05 

On Tue, 2016-11-15 at 21:31 +0100, Jiri Pirko wrote:
Tue, Nov 15, 2016 at 09:29:09PM CET, alexander.h.duyck@intel.com wrote:
quoted
On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted

The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.
Okay. But please name the fuction differently, as it does not flush
external. Thanks!
You and I have different meanings for "external".

In my case I am flushing entries that belong to a foreign "external"
table from the table specified. So by "external" I am referring to
entries that don't actually live in main, but actually reside in local.
If you take a look at fib_table_flush that gets rid of all entries,
fib_table_flush_external simply clears the foreign ones.

Also I'd rather maintain naming since it makes it easier if we need to
backport fixes.

Finally, the flag RTNH_F_EXTERNAL was renamed over a year ago in commit
36583eb54d46c ("rename RTNH_F_EXTERNAL to RTNH_F_OFFLOAD") so there
isn't too much likelihood of this being confused for something that
handles offloaded entries.  If you take a look in net/ipv4/* after your
patch there isn't actually anything that references the word external
so the likelihood for any confusion is extremely low.

- Alex

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Jiri Pirko <jiri@resnulli.us>
Date: 2016-11-15 20:52:36 

Tue, Nov 15, 2016 at 09:49:02PM CET, alexander.h.duyck@intel.com wrote:
On Tue, 2016-11-15 at 21:31 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 09:29:09PM CET, alexander.h.duyck@intel.com wrote:
quoted
On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted

The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.
Okay. But please name the fuction differently, as it does not flush
external. Thanks!
You and I have different meanings for "external".

In my case I am flushing entries that belong to a foreign "external"
table from the table specified. So by "external" I am referring to
entries that don't actually live in main, but actually reside in local.
If you take a look at fib_table_flush that gets rid of all entries,
fib_table_flush_external simply clears the foreign ones.

Also I'd rather maintain naming since it makes it easier if we need to
backport fixes.

Finally, the flag RTNH_F_EXTERNAL was renamed over a year ago in commit
36583eb54d46c ("rename RTNH_F_EXTERNAL to RTNH_F_OFFLOAD") so there
isn't too much likelihood of this being confused for something that
handles offloaded entries.  If you take a look in net/ipv4/* after your
patch there isn't actually anything that references the word external
so the likelihood for any confusion is extremely low.
Okay. But if you can, please put a comment to this function in order to
prevent future confusion. Thanks!

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Duyck, Alexander H <hidden>
Date: 2016-11-15 21:01:18 

On Tue, 2016-11-15 at 21:52 +0100, Jiri Pirko wrote:
Tue, Nov 15, 2016 at 09:49:02PM CET, alexander.h.duyck@intel.com wrote:
quoted
On Tue, 2016-11-15 at 21:31 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 09:29:09PM CET, alexander.h.duyck@intel.com wrote:
quoted

On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
quoted

Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted


The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.
Okay. But please name the fuction differently, as it does not flush
external. Thanks!
You and I have different meanings for "external".

In my case I am flushing entries that belong to a foreign "external"
table from the table specified. So by "external" I am referring to
entries that don't actually live in main, but actually reside in local.
If you take a look at fib_table_flush that gets rid of all entries,
fib_table_flush_external simply clears the foreign ones.

Also I'd rather maintain naming since it makes it easier if we need to
backport fixes.

Finally, the flag RTNH_F_EXTERNAL was renamed over a year ago in commit
36583eb54d46c ("rename RTNH_F_EXTERNAL to RTNH_F_OFFLOAD") so there
isn't too much likelihood of this being confused for something that
handles offloaded entries.  If you take a look in net/ipv4/* after your
patch there isn't actually anything that references the word external
so the likelihood for any confusion is extremely low.
Okay. But if you can, please put a comment to this function in order to
prevent future confusion. Thanks!
I'm not sure there is much left to confuse at this point.  The function
has gone from multi-purpose to single purpose so anyone that is messing
with this should only be doing so if they are messing with the unmerge
functionality.

If anything it would be more confusing to refer to functionality that
this function doesn't support in the comments.  All this function does
is flush foreign/external objects from the tree.

I'm willing to review a patch if you have a suggestion for a comment
that would work.  I just want to avoid confusing people by referring to
code and functionality that is no longer relevent.

- Alex

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Jiri Pirko <jiri@resnulli.us>
Date: 2016-11-15 21:07:57 

Tue, Nov 15, 2016 at 10:01:03PM CET, alexander.h.duyck@intel.com wrote:
On Tue, 2016-11-15 at 21:52 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 09:49:02PM CET, alexander.h.duyck@intel.com wrote:
quoted
On Tue, 2016-11-15 at 21:31 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 09:29:09PM CET, alexander.h.duyck@intel.com wrote:
quoted

On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
quoted

Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted


The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.
Okay. But please name the fuction differently, as it does not flush
external. Thanks!
You and I have different meanings for "external".

In my case I am flushing entries that belong to a foreign "external"
table from the table specified. So by "external" I am referring to
entries that don't actually live in main, but actually reside in local.
If you take a look at fib_table_flush that gets rid of all entries,
fib_table_flush_external simply clears the foreign ones.

Also I'd rather maintain naming since it makes it easier if we need to
backport fixes.

Finally, the flag RTNH_F_EXTERNAL was renamed over a year ago in commit
36583eb54d46c ("rename RTNH_F_EXTERNAL to RTNH_F_OFFLOAD") so there
isn't too much likelihood of this being confused for something that
handles offloaded entries.  If you take a look in net/ipv4/* after your
patch there isn't actually anything that references the word external
so the likelihood for any confusion is extremely low.
Okay. But if you can, please put a comment to this function in order to
prevent future confusion. Thanks!
I'm not sure there is much left to confuse at this point.  The function
has gone from multi-purpose to single purpose so anyone that is messing
with this should only be doing so if they are messing with the unmerge
functionality.

If anything it would be more confusing to refer to functionality that
this function doesn't support in the comments.  All this function does
is flush foreign/external objects from the tree.

I'm willing to review a patch if you have a suggestion for a comment
that would work.  I just want to avoid confusing people by referring to
code and functionality that is no longer relevent.
Perhaps I was the only one confused. Fair enough. Thanks Alex.

Re: [net PATCH 1/2] ipv4: Restore fib_trie_flush_external function and fix call ordering

From: Duyck, Alexander H <hidden>
Date: 2016-11-15 21:11:52 

On Tue, 2016-11-15 at 22:07 +0100, Jiri Pirko wrote:
Tue, Nov 15, 2016 at 10:01:03PM CET, alexander.h.duyck@intel.com wrote:
quoted
On Tue, 2016-11-15 at 21:52 +0100, Jiri Pirko wrote:
quoted
Tue, Nov 15, 2016 at 09:49:02PM CET, alexander.h.duyck@intel.com wrote:
quoted

On Tue, 2016-11-15 at 21:31 +0100, Jiri Pirko wrote:
quoted

Tue, Nov 15, 2016 at 09:29:09PM CET, alexander.h.duyck@intel.com wrote:
quoted


On Tue, 2016-11-15 at 20:51 +0100, Jiri Pirko wrote:
quoted


Tue, Nov 15, 2016 at 11:46:06AM CET, alexander.h.duyck@intel.com wrote:
quoted



The patch that removed the FIB offload infrastructure was a bit too
aggressive and also removed code needed to clean up us splitting the table
if additional rules were added.  Specifically the function
fib_trie_flush_external was called at the end of a new rule being added to
flush the foreign trie entries from the main trie.

I updated the code so that we only call fib_trie_flush_external on the main
table so that we flush the entries for local from main.  This way we don't
call it for every rule change which is what was happening previously.
Well, the function was introduced by:

commit 104616e74e0b464d449fdd2ee2f547d2fad71610
Author: Scott Feldman [off-list ref]
Date:   Thu Mar 5 21:21:16 2015 -0800

    switchdev: don't support custom ip rules, for now
    
    Keep switchdev FIB offload model simple for now and don't allow custom ip
    rules.

Why this was not needed before? What changed in between:
104616e74e0b464d449fdd2ee2f547d2fad71610 ("switchdev: don't support custom ip rules, for now")
and
347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
We collapsed the two tables into one in commit 0ddcf43d5d4a ("ipv4: FIB
Local/MAIN table collapse") which was submitted the next day.  Scott
and I were working on things at the same time and the
fib_table_flush_external function was something we had worked out that
would allow him to take care of his use case and me to take care of
cleaning up the tables after unmerging.
Okay. But please name the fuction differently, as it does not flush
external. Thanks!
You and I have different meanings for "external".

In my case I am flushing entries that belong to a foreign "external"
table from the table specified. So by "external" I am referring to
entries that don't actually live in main, but actually reside in local.
If you take a look at fib_table_flush that gets rid of all entries,
fib_table_flush_external simply clears the foreign ones.

Also I'd rather maintain naming since it makes it easier if we need to
backport fixes.

Finally, the flag RTNH_F_EXTERNAL was renamed over a year ago in commit
36583eb54d46c ("rename RTNH_F_EXTERNAL to RTNH_F_OFFLOAD") so there
isn't too much likelihood of this being confused for something that
handles offloaded entries.  If you take a look in net/ipv4/* after your
patch there isn't actually anything that references the word external
so the likelihood for any confusion is extremely low.
Okay. But if you can, please put a comment to this function in order to
prevent future confusion. Thanks!
I'm not sure there is much left to confuse at this point.  The function
has gone from multi-purpose to single purpose so anyone that is messing
with this should only be doing so if they are messing with the unmerge
functionality.

If anything it would be more confusing to refer to functionality that
this function doesn't support in the comments.  All this function does
is flush foreign/external objects from the tree.

I'm willing to review a patch if you have a suggestion for a comment
that would work.  I just want to avoid confusing people by referring to
code and functionality that is no longer relevent.
Perhaps I was the only one confused. Fair enough. Thanks Alex.
No, I would agree the original sharing of the function was a bit
confusing, and that was mostly due to the timeline of things with both
Scott and I rebasing our code on top of each other.

With you removing the FIB offload pieces it simplified it quite a bit
so with this fix we should be back to 100% functionality with the code
much more simplified.

Thanks.

- Alex

Re: [net PATCH 0/2] ipv4: Fix memory leaks and reference issues in fib

From: David Miller <davem@davemloft.net>
Date: 2016-11-16 18:25:12 

From: Alexander Duyck <redacted>
Date: Tue, 15 Nov 2016 05:46:01 -0500

This series fixes one major issue and one minor issue in the fib tables.

The major issue is that we had lost the functionality that was flushing the
local table entries from main after we had unmerged the two tries.  In
order to regain the functionality I have performed a partial revert and
then moved the functionality for flushing the external entries from main
into fib_unmerge.

The minor issue was a memory leak that could occur in the event that we
weren't able to add an alias to the local trie resulting in the fib alias
being leaked.
Series applied and queued up for -stable, thanks Alexander.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help