[PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Kristian Evensen]

From: Kristian Evensen <kristian.evensen@gmail.com>

When a link is set as down using for example "ip link set dev X down", routes
are deleted, but RTM_DELROUTE messages are not sent to RTNLGRP_IPV4_ROUTE. This
patch makes trie_flush_list() send a RTM_DELROUTE for each route that is
removed, and makes rtnelink route deletion behavior consistent across commands.
The parameter lists for trie_flush_list() and trie_flush_leaf() are expanded to
include required information otherwise unavailable in trie_flush_list().

One use case that is simplified by this patch is IPv4 WAN connection monitoring.
Instead of listening for and handling both RTM_*ROUTE and RTM_*LINK-messages, it
is sufficient to handle RTM_*ROUTE.

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>

---
 net/ipv4/fib_trie.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index ec9a9ef..acd5b3b 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1698,15 +1698,23 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
 	return 0;
 }
 
-static int trie_flush_list(struct list_head *head)
+static int trie_flush_list(struct list_head *head, u32 tb_id, t_key key,
+		int plen)
 {
 	struct fib_alias *fa, *fa_node;
 	int found = 0;
+	struct nl_info cfg;
+
+	memset(&cfg, 0, sizeof(cfg));
 
 	list_for_each_entry_safe(fa, fa_node, head, fa_list) {
 		struct fib_info *fi = fa->fa_info;
 
 		if (fi && (fi->fib_flags & RTNH_F_DEAD)) {
+			cfg.nl_net = fi->fib_net;
+			rtmsg_fib(RTM_DELROUTE, htonl(key), fa, plen, tb_id,
+					&cfg, 0);
+
 			list_del_rcu(&fa->fa_list);
 			fib_release_info(fa->fa_info);
 			alias_free_mem_rcu(fa);
@@ -1716,7 +1724,7 @@ static int trie_flush_list(struct list_head *head)
 	return found;
 }
 
-static int trie_flush_leaf(struct leaf *l)
+static int trie_flush_leaf(struct leaf *l, u32 tb_id)
 {
 	int found = 0;
 	struct hlist_head *lih = &l->list;
@@ -1724,7 +1732,7 @@ static int trie_flush_leaf(struct leaf *l)
 	struct leaf_info *li = NULL;
 
 	hlist_for_each_entry_safe(li, tmp, lih, hlist) {
-		found += trie_flush_list(&li->falh);
+		found += trie_flush_list(&li->falh, tb_id, l->key, li->plen);
 
 		if (list_empty(&li->falh)) {
 			hlist_del_rcu(&li->hlist);
@@ -1813,7 +1821,7 @@ int fib_table_flush(struct fib_table *tb)
 	int found = 0;
 
 	for (l = trie_firstleaf(t); l; l = trie_nextleaf(l)) {
-		found += trie_flush_leaf(l);
+		found += trie_flush_leaf(l, tb->tb_id);
 
 		if (ll && hlist_empty(&ll->list))
 			trie_leaf_remove(t, ll);
-- 
1.8.1.2

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Sergei Shtylyov]

Hello.

On 17-10-2013 15:13, Kristian Evensen wrote:

> From: Kristian Evensen <kristian.evensen@gmail.com>

> When a link is set as down using for example "ip link set dev X down", routes
> are deleted, but RTM_DELROUTE messages are not sent to RTNLGRP_IPV4_ROUTE. This
> patch makes trie_flush_list() send a RTM_DELROUTE for each route that is
> removed, and makes rtnelink route deletion behavior consistent across commands.
> The parameter lists for trie_flush_list() and trie_flush_leaf() are expanded to
> include required information otherwise unavailable in trie_flush_list().

> One use case that is simplified by this patch is IPv4 WAN connection monitoring.
> Instead of listening for and handling both RTM_*ROUTE and RTM_*LINK-messages, it
> is sufficient to handle RTM_*ROUTE.

> Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>

> ---
>   net/ipv4/fib_trie.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)

> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index ec9a9ef..acd5b3b 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -1698,15 +1698,23 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
>   	return 0;
>   }
>
> -static int trie_flush_list(struct list_head *head)
> +static int trie_flush_list(struct list_head *head, u32 tb_id, t_key key,
> +		int plen)

    The continuation line should start right under *struct* on the first line, 
according to the networking coding style.

>   {
>   	struct fib_alias *fa, *fa_node;
>   	int found = 0;
> +	struct nl_info cfg;
> +
> +	memset(&cfg, 0, sizeof(cfg));
>
>   	list_for_each_entry_safe(fa, fa_node, head, fa_list) {
>   		struct fib_info *fi = fa->fa_info;
>
>   		if (fi && (fi->fib_flags & RTNH_F_DEAD)) {
> +			cfg.nl_net = fi->fib_net;
> +			rtmsg_fib(RTM_DELROUTE, htonl(key), fa, plen, tb_id,
> +					&cfg, 0);

    Here the line should start right under RTM_DELROUTE.

WBR, Sergei

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Kristian Evensen]

Hi,

>    The continuation line should start right under *struct* on the first
> line, according to the networking coding style.

Thank you very much for letting me know. Will resubmit the patch.

-Kristian

[PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Kristian Evensen]

From: Kristian Evensen <kristian.evensen@gmail.com>

When a link is set as down using for example "ip link set dev X down", routes
are deleted, but RTM_DELROUTE messages are not sent to RTNLGRP_IPV4_ROUTE. This
patch makes trie_flush_list() send a RTM_DELROUTE for each route that is
removed, and makes rtnelink route deletion behavior consistent across commands.
The parameter lists for trie_flush_list() and trie_flush_leaf() are expanded to
include required information otherwise unavailable in trie_flush_list().

One use case that is simplified by this patch is IPv4 WAN connection monitoring.
Instead of listening for and handling both RTM_*ROUTE and RTM_*LINK-messages, it
is sufficient to handle RTM_*ROUTE.

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>

---
 net/ipv4/fib_trie.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index ec9a9ef..629747a 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1698,15 +1698,23 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
 	return 0;
 }
 
-static int trie_flush_list(struct list_head *head)
+static int trie_flush_list(struct list_head *head, u32 tb_id, t_key key,
+			    int plen)
 {
 	struct fib_alias *fa, *fa_node;
 	int found = 0;
+	struct nl_info cfg;
+
+	memset(&cfg, 0, sizeof(cfg));
 
 	list_for_each_entry_safe(fa, fa_node, head, fa_list) {
 		struct fib_info *fi = fa->fa_info;
 
 		if (fi && (fi->fib_flags & RTNH_F_DEAD)) {
+			cfg.nl_net = fi->fib_net;
+			rtmsg_fib(RTM_DELROUTE, htonl(key), fa, plen, tb_id,
+				  &cfg, 0);
+
 			list_del_rcu(&fa->fa_list);
 			fib_release_info(fa->fa_info);
 			alias_free_mem_rcu(fa);
@@ -1716,7 +1724,7 @@ static int trie_flush_list(struct list_head *head)
 	return found;
 }
 
-static int trie_flush_leaf(struct leaf *l)
+static int trie_flush_leaf(struct leaf *l, u32 tb_id)
 {
 	int found = 0;
 	struct hlist_head *lih = &l->list;
@@ -1724,7 +1732,7 @@ static int trie_flush_leaf(struct leaf *l)
 	struct leaf_info *li = NULL;
 
 	hlist_for_each_entry_safe(li, tmp, lih, hlist) {
-		found += trie_flush_list(&li->falh);
+		found += trie_flush_list(&li->falh, tb_id, l->key, li->plen);
 
 		if (list_empty(&li->falh)) {
 			hlist_del_rcu(&li->hlist);
@@ -1813,7 +1821,7 @@ int fib_table_flush(struct fib_table *tb)
 	int found = 0;
 
 	for (l = trie_firstleaf(t); l; l = trie_nextleaf(l)) {
-		found += trie_flush_leaf(l);
+		found += trie_flush_leaf(l, tb->tb_id);
 
 		if (ll && hlist_empty(&ll->list))
 			trie_leaf_remove(t, ll);
-- 
1.8.1.2

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Joe Perches]

On Thu, 2013-10-17 at 18:54 +0200, Kristian Evensen wrote:
> From: Kristian Evensen <kristian.evensen@gmail.com>
> 
> When a link is set as down using for example "ip link set dev X down", routes
> are deleted, but RTM_DELROUTE messages are not sent to RTNLGRP_IPV4_ROUTE. This
> patch makes trie_flush_list() send a RTM_DELROUTE for each route that is
> removed, and makes rtnelink route deletion behavior consistent across commands.
> The parameter lists for trie_flush_list() and trie_flush_leaf() are expanded to
> include required information otherwise unavailable in trie_flush_list().
[]
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
[]
> @@ -1698,15 +1698,23 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
>  	return 0;
>  }
>  
> -static int trie_flush_list(struct list_head *head)
> +static int trie_flush_list(struct list_head *head, u32 tb_id, t_key key,
> +			    int plen)
>  {
>  	struct fib_alias *fa, *fa_node;
>  	int found = 0;
> +	struct nl_info cfg;
> +
> +	memset(&cfg, 0, sizeof(cfg));

Perhaps this memset should be moved into the
list_for_each_entry_safe loop where cfg is used.
 
>  	list_for_each_entry_safe(fa, fa_node, head, fa_list) {
>  		struct fib_info *fi = fa->fa_info;
>  
>  		if (fi && (fi->fib_flags & RTNH_F_DEAD)) {

			memset(&cfg, 0, sizeof(cfg));

?

> +			cfg.nl_net = fi->fib_net;
> +			rtmsg_fib(RTM_DELROUTE, htonl(key), fa, plen, tb_id,
> +				  &cfg, 0);

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Julian Anastasov]

	Hello,

On Thu, 17 Oct 2013, Kristian Evensen wrote:

> From: Kristian Evensen <kristian.evensen@gmail.com>
> 
> When a link is set as down using for example "ip link set dev X down", routes
> are deleted, but RTM_DELROUTE messages are not sent to RTNLGRP_IPV4_ROUTE. This
> patch makes trie_flush_list() send a RTM_DELROUTE for each route that is
> removed, and makes rtnelink route deletion behavior consistent across commands.
> The parameter lists for trie_flush_list() and trie_flush_leaf() are expanded to
> include required information otherwise unavailable in trie_flush_list().
> 
> One use case that is simplified by this patch is IPv4 WAN connection monitoring.
> Instead of listening for and handling both RTM_*ROUTE and RTM_*LINK-messages, it
> is sufficient to handle RTM_*ROUTE.

	IIRC, such notifications were not implemented
to avoid storms to routing daemons. Not sure if this is
still true.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Kristian Evensen]

Hi,

On Thu, Oct 17, 2013 at 9:52 PM, Julian Anastasov <ja@ssi.bg> wrote:
>         IIRC, such notifications were not implemented
> to avoid storms to routing daemons. Not sure if this is
> still true.

Thanks for letting me know. I was wondering if this was the case, but
then I saw that the deletion of IPv6 routes are announced (when
running "ip link set dev X down") and assumed that message storms was
(is?) not considered a problem.

-Kristian

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[Kristian Evensen]

Hi,

On Thu, Oct 17, 2013 at 7:23 PM, Joe Perches <joe@perches.com> wrote:
>
> Perhaps this memset should be moved into the
> list_for_each_entry_safe loop where cfg is used.

Thanks for your comment. I decided to put it there to avoid calling it
for each route that will be deleted. Since the struct is never
modified (only read in fib_rtmsg()) and we have the rtnl_lock, I
assumed it to be safe. Are there cases where this assumption is wrong?

-Kristian

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Thu, 17 Oct 2013 22:52:43 +0300 (EEST)

> 
> 	Hello,
> 
> On Thu, 17 Oct 2013, Kristian Evensen wrote:
> 
>> From: Kristian Evensen <kristian.evensen@gmail.com>
>> 
>> When a link is set as down using for example "ip link set dev X down", routes
>> are deleted, but RTM_DELROUTE messages are not sent to RTNLGRP_IPV4_ROUTE. This
>> patch makes trie_flush_list() send a RTM_DELROUTE for each route that is
>> removed, and makes rtnelink route deletion behavior consistent across commands.
>> The parameter lists for trie_flush_list() and trie_flush_leaf() are expanded to
>> include required information otherwise unavailable in trie_flush_list().
>> 
>> One use case that is simplified by this patch is IPv4 WAN connection monitoring.
>> Instead of listening for and handling both RTM_*ROUTE and RTM_*LINK-messages, it
>> is sufficient to handle RTM_*ROUTE.
> 
> 	IIRC, such notifications were not implemented
> to avoid storms to routing daemons. Not sure if this is
> still true.

I'm definitely not applying this patch.

Routing daemons maintain the routing table internally, and they
therefore can purge their internal data structures when the link
down event occurs.

This is how it has worked forever and for real BGP tables the
RTM_DELROUTE traffic would be enormous and at nearly DoS levels
for anyone listening for them.

Re: [PATCH net-next] fib_trie: Send RTM_DELROUTE when link goes down

[David Miller]

From: Kristian Evensen <kristian.evensen@gmail.com>
Date: Thu, 17 Oct 2013 22:11:39 +0200

> On Thu, Oct 17, 2013 at 9:52 PM, Julian Anastasov <ja@ssi.bg> wrote:
>>         IIRC, such notifications were not implemented
>> to avoid storms to routing daemons. Not sure if this is
>> still true.
> 
> Thanks for letting me know. I was wondering if this was the case, but
> then I saw that the deletion of IPv6 routes are announced (when
> running "ip link set dev X down") and assumed that message storms was
> (is?) not considered a problem.

I really wish IPV6 wouldn't be inconsistent and behave that way.

[PATCH net-next] fib: Use const struct nl_info * in rtmsg_fib

[Joe Perches]

The rtmsg_fib function doesn't modify this argument so mark
it const.

Signed-off-by: Joe Perches <joe@perches.com>
---
On Thu, 2013-10-17 at 22:14 +0200, Kristian Evensen wrote:
> Hi,

Hi Kristian.

> On Thu, Oct 17, 2013 at 7:23 PM, Joe Perches <joe@perches.com> wrote:
> >
> > Perhaps this memset should be moved into the
> > list_for_each_entry_safe loop where cfg is used.
> 
> Thanks for your comment. I decided to put it there to avoid calling it
> for each route that will be deleted. Since the struct is never
> modified (only read in fib_rtmsg()) and we have the rtnl_lock, I
> assumed it to be safe. Are there cases where this assumption is wrong?

I didn't look too hard.
I just glanced at the prototype and saw it wasn't const.

If rtmsg_fib() doesn't modify cfg (and it doesn't seem to)
then the prototype and function should be marked const.

And you're right, it doesn't need to be inside the loop.

cheers, Joe

 net/ipv4/fib_lookup.h    | 2 +-
 net/ipv4/fib_semantics.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/fib_lookup.h b/net/ipv4/fib_lookup.h
index af0f14a..50cfb3e 100644
--- a/net/ipv4/fib_lookup.h
+++ b/net/ipv4/fib_lookup.h
@@ -32,7 +32,7 @@ extern int fib_dump_info(struct sk_buff *skb, u32 pid, u32 seq, int event,
 			 int dst_len, u8 tos, struct fib_info *fi,
 			 unsigned int);
 extern void rtmsg_fib(int event, __be32 key, struct fib_alias *fa,
-		      int dst_len, u32 tb_id, struct nl_info *info,
+		      int dst_len, u32 tb_id, const struct nl_info *info,
 		      unsigned int nlm_flags);
 extern struct fib_alias *fib_find_alias(struct list_head *fah,
 					u8 tos, u32 prio);
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index d5dbca5..e63f47a 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -380,7 +380,7 @@ static inline size_t fib_nlmsg_size(struct fib_info *fi)
 }
 
 void rtmsg_fib(int event, __be32 key, struct fib_alias *fa,
-	       int dst_len, u32 tb_id, struct nl_info *info,
+	       int dst_len, u32 tb_id, const struct nl_info *info,
 	       unsigned int nlm_flags)
 {
 	struct sk_buff *skb;

Re: [PATCH net-next] fib: Use const struct nl_info * in rtmsg_fib

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Thu, 17 Oct 2013 13:34:11 -0700

> The rtmsg_fib function doesn't modify this argument so mark
> it const.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Applied, thanks Joe.