[PATCHv2 net-next 0/2] send notify when delete source address routes

[PATCHv2 net-next 0/2] send notify when delete source address routes

[Hangbin Liu]

After deleting an interface address, the relate perfer source address routes
are also deleted. But there is no notify for the route deleting, which makes
route daemons like NetworkManager keep a wrong cache. Fix this by sending
notify when delete src routes.

Run fib_tests.sh and all passed.

Tests passed: 203
Tests failed:   0

v2: Add a bit in fib_info to mark the deleted src route.

Hangbin Liu (2):
  fib: convert fib_nh_is_v6 and nh_updated to use a single bit
  ipv4/fib: send notify when delete source address routes

 include/net/ip_fib.h     | 5 +++--
 net/ipv4/fib_semantics.c | 3 ++-
 net/ipv4/fib_trie.c      | 4 ++++
 net/ipv4/nexthop.c       | 4 ++--
 4 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.38.1

[PATCHv2 net-next 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit

[Hangbin Liu]

The FIB info structure currently looks like this:
struct fib_info {
        struct hlist_node          fib_hash;             /*     0    16 */
        [...]
        u32                        fib_priority;         /*    80     4 */

        /* XXX 4 bytes hole, try to pack */

        struct dst_metrics *       fib_metrics;          /*    88     8 */
        int                        fib_nhs;              /*    96     4 */
        bool                       fib_nh_is_v6;         /*   100     1 */
        bool                       nh_updated;           /*   101     1 */

        /* XXX 2 bytes hole, try to pack */

        struct nexthop *           nh;                   /*   104     8 */
        struct callback_head       rcu __attribute__((__aligned__(8))); /*   112    16 */
        /* --- cacheline 2 boundary (128 bytes) --- */
        struct fib_nh              fib_nh[];             /*   128     0 */

        /* size: 128, cachelines: 2, members: 21 */
        /* sum members: 122, holes: 2, sum holes: 6 */
        /* forced alignments: 1 */
} __attribute__((__aligned__(8)));

Let's convert fib_nh_is_v6 and nh_updated to use a single bit, so that
we can add other functional bits in later patch.

Suggested-by: Ido Schimmel <idosch@idosch.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/net/ip_fib.h     | 4 ++--
 net/ipv4/fib_semantics.c | 2 +-
 net/ipv4/nexthop.c       | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index a378eff827c7..a91f8a28689a 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -152,8 +152,8 @@ struct fib_info {
 #define fib_rtt fib_metrics->metrics[RTAX_RTT-1]
 #define fib_advmss fib_metrics->metrics[RTAX_ADVMSS-1]
 	int			fib_nhs;
-	bool			fib_nh_is_v6;
-	bool			nh_updated;
+	u8			fib_nh_is_v6:1,
+				nh_updated:1;
 	struct nexthop		*nh;
 	struct rcu_head		rcu;
 	struct fib_nh		fib_nh[];
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 65ba18a91865..ce1c10e408cf 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1572,7 +1572,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 			fib_info_update_nhc_saddr(net, &nexthop_nh->nh_common,
 						  fi->fib_scope);
 			if (nexthop_nh->fib_nh_gw_family == AF_INET6)
-				fi->fib_nh_is_v6 = true;
+				fi->fib_nh_is_v6 = 1;
 		} endfor_nexthops(fi)
 
 		fib_rebalance(fi);
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 93f14d39fef6..5243bb80bade 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -2213,12 +2213,12 @@ static void __nexthop_replace_notify(struct net *net, struct nexthop *nh,
 		 * and then walk the fib tables once
 		 */
 		list_for_each_entry(fi, &nh->fi_list, nh_list)
-			fi->nh_updated = true;
+			fi->nh_updated = 1;
 
 		fib_info_notify_update(net, info);
 
 		list_for_each_entry(fi, &nh->fi_list, nh_list)
-			fi->nh_updated = false;
+			fi->nh_updated = 0;
 	}
 
 	list_for_each_entry(f6i, &nh->f6i_list, nh_list)
-- 
2.38.1

[PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Hangbin Liu]

After deleting an interface address in fib_del_ifaddr(), the function
scans the fib_info list for stray entries and calls fib_flush() and
fib_table_flush(). Then the stray entries will be deleted silently and no
RTM_DELROUTE notification will be sent.

This lack of notification can make routing daemons like NetworkManager,
miss the routing changes. e.g.

+ ip link add dummy1 type dummy
+ ip link add dummy2 type dummy
+ ip link set dummy1 up
+ ip link set dummy2 up
+ ip addr add 192.168.5.5/24 dev dummy1
+ ip route add 7.7.7.0/24 dev dummy2 src 192.168.5.5
+ ip -4 route
7.7.7.0/24 dev dummy2 scope link src 192.168.5.5
192.168.5.0/24 dev dummy1 proto kernel scope link src 192.168.5.5
+ ip monitor route
+ ip addr del 192.168.5.5/24 dev dummy1
Deleted 192.168.5.0/24 dev dummy1 proto kernel scope link src 192.168.5.5
Deleted broadcast 192.168.5.255 dev dummy1 table local proto kernel scope link src 192.168.5.5
Deleted local 192.168.5.5 dev dummy1 table local proto kernel scope host src 192.168.5.5

As Ido reminded, fib_table_flush() isn't only called when an address is
deleted, but also when an interface is deleted or put down. The lack of
notification in these cases is deliberate. And commit 7c6bb7d2faaf
("net/ipv6: Add knob to skip DELROUTE message on device down") introduced
a sysctl to make IPv6 behave like IPv4 in this regard. So we can't send
the route delete notify blindly in fib_table_flush().

To fix this issue, let's add a new bit in "struct fib_info" to track the
deleted prefer source address routes, and only send notify for them.

After update:
+ ip monitor route
+ ip addr del 192.168.5.5/24 dev dummy1
Deleted 192.168.5.0/24 dev dummy1 proto kernel scope link src 192.168.5.5
Deleted broadcast 192.168.5.255 dev dummy1 table local proto kernel scope link src 192.168.5.5
Deleted local 192.168.5.5 dev dummy1 table local proto kernel scope host src 192.168.5.5
Deleted 7.7.7.0/24 dev dummy2 scope link src 192.168.5.5

Suggested-by: Thomas Haller <thaller@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v2: Add a bit in fib_info to mark the deleted src route.
---
 include/net/ip_fib.h     | 3 ++-
 net/ipv4/fib_semantics.c | 1 +
 net/ipv4/fib_trie.c      | 4 ++++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index a91f8a28689a..12dedfa4c9fd 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -153,7 +153,8 @@ struct fib_info {
 #define fib_advmss fib_metrics->metrics[RTAX_ADVMSS-1]
 	int			fib_nhs;
 	u8			fib_nh_is_v6:1,
-				nh_updated:1;
+				nh_updated:1,
+				pfsrc_removed:1;
 	struct nexthop		*nh;
 	struct rcu_head		rcu;
 	struct fib_nh		fib_nh[];
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index ce1c10e408cf..6cd919b442a7 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1884,6 +1884,7 @@ int fib_sync_down_addr(struct net_device *dev, __be32 local)
 			continue;
 		if (fi->fib_prefsrc == local) {
 			fi->fib_flags |= RTNH_F_DEAD;
+			fi->pfsrc_removed = 1;
 			ret++;
 		}
 	}
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 74d403dbd2b4..9cadeeaaa93a 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2026,6 +2026,7 @@ void fib_table_flush_external(struct fib_table *tb)
 int fib_table_flush(struct net *net, struct fib_table *tb, bool flush_all)
 {
 	struct trie *t = (struct trie *)tb->tb_data;
+	struct nl_info info = { .nl_net = net };
 	struct key_vector *pn = t->kv;
 	unsigned long cindex = 1;
 	struct hlist_node *tmp;
@@ -2088,6 +2089,9 @@ int fib_table_flush(struct net *net, struct fib_table *tb, bool flush_all)
 
 			fib_notify_alias_delete(net, n->key, &n->leaf, fa,
 						NULL);
+			if (fi->pfsrc_removed)
+				rtmsg_fib(RTM_DELROUTE, htonl(n->key), fa,
+					  KEYLENGTH - fa->fa_slen, tb->tb_id, &info, 0);
 			hlist_del_rcu(&fa->fa_list);
 			fib_release_info(fa->fa_info);
 			alias_free_mem_rcu(fa);
-- 
2.38.1

Re: [PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Ido Schimmel]

On Wed, Aug 09, 2023 at 10:02:34PM +0800, Hangbin Liu wrote:
> After deleting an interface address in fib_del_ifaddr(), the function
> scans the fib_info list for stray entries and calls fib_flush() and
> fib_table_flush(). Then the stray entries will be deleted silently and no
> RTM_DELROUTE notification will be sent.
> 
> This lack of notification can make routing daemons like NetworkManager,
> miss the routing changes. e.g.

[...]

> To fix this issue, let's add a new bit in "struct fib_info" to track the
> deleted prefer source address routes, and only send notify for them.

In the other thread Thomas mentioned that NM already requests a route
dump following address deletion [1]. If so, can Thomas or you please
explain how this patch is going to help NM? Is the intention to optimize
things and avoid the dump request (which can only work on new kernels)?

[1] https://lore.kernel.org/netdev/07fcfd504148b3c721fda716ad0a549662708407.camel@redhat.com/

Re: [PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Hangbin Liu]

On Thu, Aug 10, 2023 at 06:08:28PM +0300, Ido Schimmel wrote:
> On Wed, Aug 09, 2023 at 10:02:34PM +0800, Hangbin Liu wrote:
> > After deleting an interface address in fib_del_ifaddr(), the function
> > scans the fib_info list for stray entries and calls fib_flush() and
> > fib_table_flush(). Then the stray entries will be deleted silently and no
> > RTM_DELROUTE notification will be sent.
> > 
> > This lack of notification can make routing daemons like NetworkManager,
> > miss the routing changes. e.g.
> 
> [...]
> 
> > To fix this issue, let's add a new bit in "struct fib_info" to track the
> > deleted prefer source address routes, and only send notify for them.
> 
> In the other thread Thomas mentioned that NM already requests a route
> dump following address deletion [1]. If so, can Thomas or you please
> explain how this patch is going to help NM? Is the intention to optimize
> things and avoid the dump request (which can only work on new kernels)?
> 
> [1] https://lore.kernel.org/netdev/07fcfd504148b3c721fda716ad0a549662708407.camel@redhat.com/

In my understanding, After deleting an address, deal with the delete notify is
more efficient to maintain the route cache than dump all the routes.

Hi Thomas ,do you have any comments?

Thanks
Hangbin

Re: [PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Hangbin Liu]

Hi Thomas,

Any comments?

Thanks
Hangbin
On Tue, Aug 15, 2023 at 08:55:35PM +0800, Hangbin Liu wrote:
> On Thu, Aug 10, 2023 at 06:08:28PM +0300, Ido Schimmel wrote:
> > On Wed, Aug 09, 2023 at 10:02:34PM +0800, Hangbin Liu wrote:
> > > After deleting an interface address in fib_del_ifaddr(), the function
> > > scans the fib_info list for stray entries and calls fib_flush() and
> > > fib_table_flush(). Then the stray entries will be deleted silently and no
> > > RTM_DELROUTE notification will be sent.
> > > 
> > > This lack of notification can make routing daemons like NetworkManager,
> > > miss the routing changes. e.g.
> > 
> > [...]
> > 
> > > To fix this issue, let's add a new bit in "struct fib_info" to track the
> > > deleted prefer source address routes, and only send notify for them.
> > 
> > In the other thread Thomas mentioned that NM already requests a route
> > dump following address deletion [1]. If so, can Thomas or you please
> > explain how this patch is going to help NM? Is the intention to optimize
> > things and avoid the dump request (which can only work on new kernels)?
> > 
> > [1] https://lore.kernel.org/netdev/07fcfd504148b3c721fda716ad0a549662708407.camel@redhat.com/
> 
> In my understanding, After deleting an address, deal with the delete notify is
> more efficient to maintain the route cache than dump all the routes.
> 
> Hi Thomas ,do you have any comments?
> 
> Thanks
> Hangbin

Re: [PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Thomas Haller]

On Mon, 2023-08-28 at 14:14 +0800, Hangbin Liu wrote:
> > > 
> > > In the other thread Thomas mentioned that NM already requests a
> > > route
> > > dump following address deletion [1]. If so, can Thomas or you
> > > please
> > > explain how this patch is going to help NM? Is the intention to
> > > optimize
> > > things and avoid the dump request (which can only work on new
> > > kernels)?
> > > 
> > > [1]
> > > https://lore.kernel.org/netdev/07fcfd504148b3c721fda716ad0a549662708407.camel@redhat.com/
> > 
> > In my understanding, After deleting an address, deal with the
> > delete notify is
> > more efficient to maintain the route cache than dump all the
> > routes.


Hi,


NetworkManager does so out of necessity, as there is no notification.
Overall, it seems a pretty bad thing to do, because it's expensive if
you have many routes/addresses.

Unfortunately, it's hard to ever drop a workaround, because we never
know when the workaround can be dropped.

Also, it's simply a notification missing, and not tied to
NetworkManager or to maintaining a cache. If you run `ip route
monitor`, you also don't see the notification that kernel drops a
route? The effort that NetworkManager takes to maintain correct
information is not something that most programs would do.


Thomas

Re: [PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Hangbin Liu]

Hi Ido,

Do you think if I should modify the patch description and re-post it?

Thanks
Hangbin
On Mon, Sep 11, 2023 at 11:35:05AM +0200, Thomas Haller wrote:
> On Mon, 2023-08-28 at 14:14 +0800, Hangbin Liu wrote:
> > > > 
> > > > In the other thread Thomas mentioned that NM already requests a
> > > > route
> > > > dump following address deletion [1]. If so, can Thomas or you
> > > > please
> > > > explain how this patch is going to help NM? Is the intention to
> > > > optimize
> > > > things and avoid the dump request (which can only work on new
> > > > kernels)?
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/netdev/07fcfd504148b3c721fda716ad0a549662708407.camel@redhat.com/
> > > 
> > > In my understanding, After deleting an address, deal with the
> > > delete notify is
> > > more efficient to maintain the route cache than dump all the
> > > routes.
> 
> 
> Hi,
> 
> 
> NetworkManager does so out of necessity, as there is no notification.
> Overall, it seems a pretty bad thing to do, because it's expensive if
> you have many routes/addresses.
> 
> Unfortunately, it's hard to ever drop a workaround, because we never
> know when the workaround can be dropped.
> 
> Also, it's simply a notification missing, and not tied to
> NetworkManager or to maintaining a cache. If you run `ip route
> monitor`, you also don't see the notification that kernel drops a
> route? The effort that NetworkManager takes to maintain correct
> information is not something that most programs would do.
> 
> 
> Thomas
> 

Re: [PATCHv2 net-next 2/2] ipv4/fib: send notify when delete source address routes

[Nicolas Dichtel]

Le 12/09/2023 à 04:30, Hangbin Liu a écrit :
> Hi Ido,
> 
> Do you think if I should modify the patch description and re-post it?
At least, this patch seems to be targeted for net, not for net-next.


Regards,
Nicolas