[PATCHv3 net 0/2] IPv4: send notify when delete source address routes

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCHv3 net 0/2] IPv4: send notify when delete source address routes
@ 2023-09-21  3:14 Hangbin Liu
  2023-09-21  3:14 ` [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit Hangbin Liu
  2023-09-21  3:14 ` [PATCHv3 net 2/2] ipv4/fib: send notify when delete source address routes Hangbin Liu
  0 siblings, 2 replies; 6+ messages in thread
From: Hangbin Liu @ 2023-09-21  3:14 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Ido Schimmel,
	David Ahern, Benjamin Poirier, Thomas Haller, Stephen Hemminger,
	Eric Dumazet, Nicolas Dichtel, 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, or monitor like `ip monitor route` miss the routing changes
when delete src routes.

Run fib_tests.sh and all passed.

Tests passed: 203
Tests failed:   0

v3: Update patch description. Target to net as Nicolas suggested.
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.41.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit
  2023-09-21  3:14 [PATCHv3 net 0/2] IPv4: send notify when delete source address routes Hangbin Liu
@ 2023-09-21  3:14 ` Hangbin Liu
  2023-09-21 13:03   ` David Ahern
  2023-09-21  3:14 ` [PATCHv3 net 2/2] ipv4/fib: send notify when delete source address routes Hangbin Liu
  1 sibling, 1 reply; 6+ messages in thread
From: Hangbin Liu @ 2023-09-21  3:14 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Ido Schimmel,
	David Ahern, Benjamin Poirier, Thomas Haller, Stephen Hemminger,
	Eric Dumazet, Nicolas Dichtel, Hangbin Liu, Ido Schimmel

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 f0c13864180e..6d05469cf5da 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 eafa4a033515..b2858b0a1229 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1573,7 +1573,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 bbff68b5b5d4..54ba53c89b3d 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.41.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit
  2023-09-21  3:14 ` [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit Hangbin Liu
@ 2023-09-21 13:03   ` David Ahern
  2023-09-22  1:29     ` Hangbin Liu
  0 siblings, 1 reply; 6+ messages in thread
From: David Ahern @ 2023-09-21 13:03 UTC (permalink / raw)
  To: Hangbin Liu, netdev
  Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Ido Schimmel,
	Benjamin Poirier, Thomas Haller, Stephen Hemminger, Eric Dumazet,
	Nicolas Dichtel, Ido Schimmel

On 9/20/23 9:14 PM, Hangbin Liu wrote:
> 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 */

2B hole here and you want to add a single flag so another bool. I would
prefer the delay to a bitfield until all holes are consumed.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit
  2023-09-21 13:03   ` David Ahern
@ 2023-09-22  1:29     ` Hangbin Liu
  2023-09-22  1:51       ` David Ahern
  0 siblings, 1 reply; 6+ messages in thread
From: Hangbin Liu @ 2023-09-22  1:29 UTC (permalink / raw)
  To: David Ahern
  Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Ido Schimmel, Benjamin Poirier, Thomas Haller, Stephen Hemminger,
	Eric Dumazet, Nicolas Dichtel, Ido Schimmel

On Thu, Sep 21, 2023 at 07:03:20AM -0600, David Ahern wrote:
> On 9/20/23 9:14 PM, Hangbin Liu wrote:
> > 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 */
> 
> 2B hole here and you want to add a single flag so another bool. I would
> prefer the delay to a bitfield until all holes are consumed.
> 

OK, just in case I didn't misunderstand. I should add a `bool pfsrc_removed`
here and drop the first patch, right?

Thanks
Hangbin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit
  2023-09-22  1:29     ` Hangbin Liu
@ 2023-09-22  1:51       ` David Ahern
  0 siblings, 0 replies; 6+ messages in thread
From: David Ahern @ 2023-09-22  1:51 UTC (permalink / raw)
  To: Hangbin Liu
  Cc: netdev, David S. Miller, Jakub Kicinski, Paolo Abeni,
	Ido Schimmel, Benjamin Poirier, Thomas Haller, Stephen Hemminger,
	Eric Dumazet, Nicolas Dichtel, Ido Schimmel

On 9/21/23 7:29 PM, Hangbin Liu wrote:
> On Thu, Sep 21, 2023 at 07:03:20AM -0600, David Ahern wrote:
>> On 9/20/23 9:14 PM, Hangbin Liu wrote:
>>> 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 */
>>
>> 2B hole here and you want to add a single flag so another bool. I would
>> prefer the delay to a bitfield until all holes are consumed.
>>
> 
> OK, just in case I didn't misunderstand. I should add a `bool pfsrc_removed`
> here and drop the first patch, right?


yes.

The bitfield has higher overhead. Let's reserve that overhead to when
there are no more holes and a new field pushes the struct over 128B.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCHv3 net 2/2] ipv4/fib: send notify when delete source address routes
  2023-09-21  3:14 [PATCHv3 net 0/2] IPv4: send notify when delete source address routes Hangbin Liu
  2023-09-21  3:14 ` [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit Hangbin Liu
@ 2023-09-21  3:14 ` Hangbin Liu
  1 sibling, 0 replies; 6+ messages in thread
From: Hangbin Liu @ 2023-09-21  3:14 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Ido Schimmel,
	David Ahern, Benjamin Poirier, Thomas Haller, Stephen Hemminger,
	Eric Dumazet, Nicolas Dichtel, 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, or monitor like
`ip monitor route` 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>
---
v3: update patch description
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 6d05469cf5da..d7fc03c1d115 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 b2858b0a1229..ced474d5584d 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -1887,6 +1887,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 d13fb9e76b97..9bdfdab906fe 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -2027,6 +2027,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;
@@ -2089,6 +2090,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.41.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-09-22  1:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-21  3:14 [PATCHv3 net 0/2] IPv4: send notify when delete source address routes Hangbin Liu
2023-09-21  3:14 ` [PATCHv3 net 1/2] fib: convert fib_nh_is_v6 and nh_updated to use a single bit Hangbin Liu
2023-09-21 13:03   ` David Ahern
2023-09-22  1:29     ` Hangbin Liu
2023-09-22  1:51       ` David Ahern
2023-09-21  3:14 ` [PATCHv3 net 2/2] ipv4/fib: send notify when delete source address routes Hangbin Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).