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

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

[Alexander Duyck]

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

[Alexander Duyck]

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 <jiri@mellanox.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 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

[Eric Dumazet]

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 <jiri@mellanox.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---


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

[Jiri Pirko]

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 <sfeldma@gmail.com>
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")


?


>
>Fixes: 347e3b28c1ba2 ("switchdev: remove FIB offload infrastructure")
>Reported-by: Eric Dumazet <edumazet@google.com>
>Cc: Jiri Pirko <jiri@mellanox.com>
>Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>---
> 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

[Duyck, Alexander H]

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:
> > 
> > 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 <sfeldma@gmail.com>
> 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

[Jiri Pirko]

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:
>> 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 <sfeldma@gmail.com>
>> 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

[Duyck, Alexander H]

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:
> > 
> > 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:
> > > > 
> > > > 
> > > > 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 <sfeldma@gmail.com>
> > > 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

[Jiri Pirko]

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:
>> 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:
>> > > 
>> > > 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 <sfeldma@gmail.com>
>> > > 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

[Duyck, Alexander H]

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:
> > 
> > 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:
> > > > 
> > > > 
> > > > 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:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 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 <sfeldma@gmail.com>
> > > > > 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

[Jiri Pirko]

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:
>> 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:
>> > > 
>> > > 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:
>> > > > > 
>> > > > > 
>> > > > > 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 <sfeldma@gmail.com>
>> > > > > 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

[Duyck, Alexander H]

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:
> > 
> > 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:
> > > > 
> > > > 
> > > > 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:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 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:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 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 <sfeldma@gmail.com>
> > > > > > > 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

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

[Alexander Duyck]

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 <alexander.h.duyck@intel.com>
---
 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

[Eric Dumazet]

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 <alexander.h.duyck@intel.com>
> ---
>  net/ipv4/fib_trie.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

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

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
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.