[BIRD 2.0.x] Netlink: ignore dead routes
With net.ipv4.conf.XXX.ignore_routes_with_linkdown sysctl, a user can ensure the kernel does not use a route whose target interface is down. The route is marked with a "dead"/RTNH_F_DEAD flag. Currently, BIRD still uses and distributes this route. This patch just ignores such a route. This patch could be backported to 1.6.x. Signed-off-by: Vincent Bernat <vincent@bernat.ch> --- sysdep/linux/netlink.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index f85bcf35685b..c28126510e6e 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -1690,6 +1690,9 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h) if (i->rtm_flags & RTNH_F_ONLINK) ra->nh.flags |= RNF_ONLINK; + if (i->rtm_flags & RTNH_F_DEAD) + return; + neighbor *nbr; nbr = neigh_find(&p->p, ra->nh.gw, ra->nh.iface, (ra->nh.flags & RNF_ONLINK) ? NEF_ONLINK : 0); -- 2.28.0
I have a question: What is then `check link` supposed to do? At least for 1.6, babel is the only protocol which enables it by default, and the others, for in example direct, static, and ospf it is needed to be set by the user, and I would have assumed exactly that behavior. Or is this specific to 2.0 only? Thanks. On 22.10.20 16:16, Vincent Bernat wrote:
With net.ipv4.conf.XXX.ignore_routes_with_linkdown sysctl, a user can ensure the kernel does not use a route whose target interface is down. The route is marked with a "dead"/RTNH_F_DEAD flag. Currently, BIRD still uses and distributes this route. This patch just ignores such a route.
This patch could be backported to 1.6.x.
Signed-off-by: Vincent Bernat <vincent@bernat.ch> --- sysdep/linux/netlink.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index f85bcf35685b..c28126510e6e 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -1690,6 +1690,9 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h) if (i->rtm_flags & RTNH_F_ONLINK) ra->nh.flags |= RNF_ONLINK;
+ if (i->rtm_flags & RTNH_F_DEAD) + return; + neighbor *nbr; nbr = neigh_find(&p->p, ra->nh.gw, ra->nh.iface, (ra->nh.flags & RNF_ONLINK) ? NEF_ONLINK : 0);
❦ 23 octobre 2020 08:48 +02, Bernd Naumann:
I have a question: What is then `check link` supposed to do?
At least for 1.6, babel is the only protocol which enables it by default, and the others, for in example direct, static, and ospf it is needed to be set by the user, and I would have assumed exactly that behavior.
`check link` does not seem to exist for the kernel protocol. It could be an option, but IMO, this is a separate issue: a route the kernel won't use shouldn't be used by BIRD either, so the check for the "dead" flag should be done in all cases. -- Don't stop at one bug. - The Elements of Programming Style (Kernighan & Plauger)
On 23.10.20 11:36, Vincent Bernat wrote:
❦ 23 octobre 2020 08:48 +02, Bernd Naumann:
I have a question: What is then `check link` supposed to do?
At least for 1.6, babel is the only protocol which enables it by default, and the others, for in example direct, static, and ospf it is needed to be set by the user, and I would have assumed exactly that behavior.
`check link` does not seem to exist for the kernel protocol. It could be an option, but IMO, this is a separate issue: a route the kernel won't use shouldn't be used by BIRD either, so the check for the "dead" flag should be done in all cases.
Thanks for the explanation but I'm still left a little bit puzzled. So the issue unfoldes if I get routes via ${hop} and I'm supposed to reach ${hop} on an interface (using its device route for example) but the device is down? So bird will happily exports these routes to the kernel, but the kernel is never able to send packets on that link because its down. Do I understand this correctly? A quick search revealed "Why Link-State Matters" from 2015 https://events.static.linuxfound.org/sites/events/files/slides/LinuxCON2015-... which somehow give me a glace about the issue here... This sysctl setting were unknown to me, and I did not know about `ip netconf` yet. Bernd
❦ 23 octobre 2020 16:17 +02, Bernd Naumann:
So the issue unfoldes if I get routes via ${hop} and I'm supposed to reach ${hop} on an interface (using its device route for example) but the device is down? So bird will happily exports these routes to the kernel, but the kernel is never able to send packets on that link because its down. Do I understand this correctly?
My use case is the reverse: you have a static route with nexthop in the kernel, but the target interface is down. I don't want to BIRD to use and advertise this route. -- Nothing so needs reforming as other people's habits. -- Mark Twain
On Thu, Oct 22, 2020 at 04:16:36PM +0200, Vincent Bernat wrote:
With net.ipv4.conf.XXX.ignore_routes_with_linkdown sysctl, a user can ensure the kernel does not use a route whose target interface is down. The route is marked with a "dead"/RTNH_F_DEAD flag. Currently, BIRD still uses and distributes this route. This patch just ignores such a route.
This patch could be backported to 1.6.x.
Hi Would not be better to convert netlink message with RTNH_F_DEAD to a withdraw? I guess that when a route became marked as dead, kerne sends asynchronous notification about that. This patch would just wait for a periodic scan to remove the route. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Thu, Oct 22, 2020 at 04:16:36PM +0200, Vincent Bernat wrote:
With net.ipv4.conf.XXX.ignore_routes_with_linkdown sysctl, a user can ensure the kernel does not use a route whose target interface is down. The route is marked with a "dead"/RTNH_F_DEAD flag. Currently, BIRD still uses and distributes this route. This patch just ignores such a route.
Hi (Noticed while looking for some missed / forgotten e-mails) Thanks, merged with some changes (handling of direct and multipath routes): https://gitlab.nic.cz/labs/bird/-/commit/df83f626973fda1e67769d295c47d4d246e... Although it would make sense to handle dead routes as withdraws instead of just ingore them (for async notification), it does not matter for sync scan, and as i noticed during testing, Linux kernel does not send async notifications (when the flag changes to dead) anyways, so it does not really matter.
This patch could be backported to 1.6.x.
Signed-off-by: Vincent Bernat <vincent@bernat.ch> --- sysdep/linux/netlink.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index f85bcf35685b..c28126510e6e 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -1690,6 +1690,9 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h) if (i->rtm_flags & RTNH_F_ONLINK) ra->nh.flags |= RNF_ONLINK;
+ if (i->rtm_flags & RTNH_F_DEAD) + return; + neighbor *nbr; nbr = neigh_find(&p->p, ra->nh.gw, ra->nh.iface, (ra->nh.flags & RNF_ONLINK) ? NEF_ONLINK : 0); -- 2.28.0
-- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
❦ 14 janvier 2021 04:18 +01, Ondrej Zajicek:
Although it would make sense to handle dead routes as withdraws instead of just ingore them (for async notification), it does not matter for sync scan, and as i noticed during testing, Linux kernel does not send async notifications (when the flag changes to dead) anyways, so it does not really matter.
It would makes sense to fix Linux for that. I'll try to send a patch and ping here again if it gets accepted. -- Make sure comments and code agree. - The Elements of Programming Style (Kernighan & Plauger)
❦ 14 janvier 2021 08:23 +01, Vincent Bernat:
Although it would make sense to handle dead routes as withdraws instead of just ingore them (for async notification), it does not matter for sync scan, and as i noticed during testing, Linux kernel does not send async notifications (when the flag changes to dead) anyways, so it does not really matter.
It would makes sense to fix Linux for that. I'll try to send a patch and ping here again if it gets accepted.
It is more complex that I would have expected. First, in-kernel, the next-hop only has RTNH_F_LINKDOWN, not RTNH_F_DEAD. This later flag is added when sending the flags over netlink only. Second, there is no async notification when a route goes down either. There is a notification on the interface. How BIRD handles this case? Is a route scan triggered when an interface goes down? I'll test more later, it's a bit late for me. -- He that breaks a thing to find out what it is has left the path of wisdom. -- J.R.R. Tolkien
On Thu, Jan 14, 2021 at 07:46:04PM +0100, Vincent Bernat wrote:
It is more complex that I would have expected. First, in-kernel, the next-hop only has RTNH_F_LINKDOWN, not RTNH_F_DEAD. This later flag is added when sending the flags over netlink only.
Second, there is no async notification when a route goes down either. There is a notification on the interface. How BIRD handles this case? Is a route scan triggered when an interface goes down? I'll test more later, it's a bit late for me.
Hi Yes, scan is triggered in krt_if_notify() for iface-admin-down event. Perhaps we can also trigger scan for iface-link-down event. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
❦ 15 janvier 2021 05:39 +01, Ondrej Zajicek:
It is more complex that I would have expected. First, in-kernel, the next-hop only has RTNH_F_LINKDOWN, not RTNH_F_DEAD. This later flag is added when sending the flags over netlink only.
Second, there is no async notification when a route goes down either. There is a notification on the interface. How BIRD handles this case? Is a route scan triggered when an interface goes down? I'll test more later, it's a bit late for me.
Hi
Yes, scan is triggered in krt_if_notify() for iface-admin-down event. Perhaps we can also trigger scan for iface-link-down event.
Hello, You mean this part in krt.c? if ((flags & IF_CHANGE_DOWN) && KRT_CF->learn) krt_scan_timer_kick(p); I was also confused by the debug code in iface.c: if (i->flags & IF_ADMIN_UP) debug(" LINK-UP"); I think it should be ADMIN-UP and the if for IF_LINK_UP should be added. I can test such a change in a few days. -- Don't stop at one bug. - The Elements of Programming Style (Kernighan & Plauger)
On Fri, Jan 15, 2021 at 12:01:47PM +0100, Vincent Bernat wrote:
I was also confused by the debug code in iface.c:
if (i->flags & IF_ADMIN_UP) debug(" LINK-UP");
I think it should be ADMIN-UP and the if for IF_LINK_UP should be added.
Yes. this seems like a remnant from the distant past. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Hi all, I'll pop up this old discussion. I've found a bug with bird handling its routes when ignore_routes_with_linkdown is enabled. And I suspect it is related to this patch. The problem is that bird does not replace or delete the routes it installed when they are marked as dead. For example if we stop it with "graceful restart", the routes are left in the table and after we start bird again with updated config, this old route remains in the table. I can reproduce it with bird compiled from master branch now. Now, that I have found this thread, I suppose, that this changes blind bird for all such routes during the scan. As a result bird also ignores its own routes (proto bird), which causes problems. I think there should be an exception for the routes that are installed by bird itself. They should not be exported to the other protocols anyway, I suppose. So the original question of this thread should not be affected. Steps to reproduce follows. Output from 2 consoles is mixed here. bird{1,2}.conf are attached, the second one has static routes commented out. Prepare: # ip netns add a # ip netns exec a bash # ip link set lo up # ip link add type veth # sysctl -w net.ipv4.conf.veth0.ignore_routes_with_linkdown=1 # ip link set veth0 up # ip addr add 10.0.0.1/24 dev veth0 Test: # ip ro 10.0.0.0/24 dev veth0 proto kernel scope link src 10.0.0.1 dead linkdown # bird -d -l -c bird1.conf bird: Chosen router ID 10.0.0.1 according to interface veth0 bird: Started # ip ro 10.0.0.0/24 dev veth0 proto kernel scope link src 10.0.0.1 dead linkdown 192.168.1.0/24 via 10.0.0.2 dev veth0 proto bird metric 32 dead linkdown 192.168.2.0/24 dev lo proto bird scope link metric 32 # birdc -l graceful restart BIRD 2.0.8 ready. Graceful restart requested bird: Shutting down for graceful restart bird: Shutdown completed # ip ro 10.0.0.0/24 dev veth0 proto kernel scope link src 10.0.0.1 dead linkdown 192.168.1.0/24 via 10.0.0.2 dev veth0 proto bird metric 32 dead linkdown 192.168.2.0/24 dev lo proto bird scope link metric 32 # bird -d -l -c bird2.conf -R bird: Chosen router ID 10.0.0.1 according to interface veth0 bird: Graceful restart started bird: Graceful restart done bird: Started # ip ro 10.0.0.0/24 dev veth0 proto kernel scope link src 10.0.0.1 dead linkdown 192.168.1.0/24 via 10.0.0.2 dev veth0 proto bird metric 32 dead linkdown <---- tihs route should have been deleted On Thu, Jan 14, 2021 at 4:22 AM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Thu, Oct 22, 2020 at 04:16:36PM +0200, Vincent Bernat wrote:
With net.ipv4.conf.XXX.ignore_routes_with_linkdown sysctl, a user can ensure the kernel does not use a route whose target interface is down. The route is marked with a "dead"/RTNH_F_DEAD flag. Currently, BIRD still uses and distributes this route. This patch just ignores such a route.
Hi
(Noticed while looking for some missed / forgotten e-mails)
Thanks, merged with some changes (handling of direct and multipath routes):
https://gitlab.nic.cz/labs/bird/-/commit/df83f626973fda1e67769d295c47d4d246e...
Although it would make sense to handle dead routes as withdraws instead of just ingore them (for async notification), it does not matter for sync scan, and as i noticed during testing, Linux kernel does not send async notifications (when the flag changes to dead) anyways, so it does not really matter.
This patch could be backported to 1.6.x.
Signed-off-by: Vincent Bernat <vincent@bernat.ch> --- sysdep/linux/netlink.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index f85bcf35685b..c28126510e6e 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -1690,6 +1690,9 @@ nl_parse_route(struct nl_parse_state *s, struct nlmsghdr *h) if (i->rtm_flags & RTNH_F_ONLINK) ra->nh.flags |= RNF_ONLINK;
+ if (i->rtm_flags & RTNH_F_DEAD) + return; + neighbor *nbr; nbr = neigh_find(&p->p, ra->nh.gw, ra->nh.iface, (ra->nh.flags & RNF_ONLINK) ? NEF_ONLINK : 0); -- 2.28.0
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Mon, Dec 27, 2021 at 02:51:34PM +0100, Alexander Zubkov wrote:
Hi all,
I'll pop up this old discussion. I've found a bug with bird handling its routes when ignore_routes_with_linkdown is enabled. And I suspect it is related to this patch. The problem is that bird does not replace or delete the routes it installed when they are marked as dead. For example if we stop it with "graceful restart", the routes are left in the table and after we start bird again with updated config, this old route remains in the table. I can reproduce it with bird compiled from master branch now. Now, that I have found this thread, I suppose, that this changes blind bird for all such routes during the scan. As a result bird also ignores its own routes (proto bird), which causes problems. I think there should be an exception for the routes that are installed by bird itself.
Hi You are right, it makes sense to apply dead-flag handling only to to non-bird routes. Will fix that. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
I made a patch for that. It works at least for my test case. I had to add additional parameter to nl_parse_multipath() function to pass information about this exception for dead routes. But may be there are better ways to do that. For example to set "s->krt_src" earlier, but I do not know if it will not break something else. Or may be it is even better to add additional field to "struct nl_parse_state" for that. On Mon, Dec 27, 2021 at 5:35 PM Ondrej Zajicek <santiago@crfreenet.org> wrote:
On Mon, Dec 27, 2021 at 02:51:34PM +0100, Alexander Zubkov wrote:
Hi all,
I'll pop up this old discussion. I've found a bug with bird handling its routes when ignore_routes_with_linkdown is enabled. And I suspect it is related to this patch. The problem is that bird does not replace or delete the routes it installed when they are marked as dead. For example if we stop it with "graceful restart", the routes are left in the table and after we start bird again with updated config, this old route remains in the table. I can reproduce it with bird compiled from master branch now. Now, that I have found this thread, I suppose, that this changes blind bird for all such routes during the scan. As a result bird also ignores its own routes (proto bird), which causes problems. I think there should be an exception for the routes that are installed by bird itself.
Hi
You are right, it makes sense to apply dead-flag handling only to to non-bird routes. Will fix that.
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Sat, Jan 01, 2022 at 08:10:01PM +0100, Alexander Zubkov wrote:
I made a patch for that. It works at least for my test case. I had to add additional parameter to nl_parse_multipath() function to pass information about this exception for dead routes. But may be there are better ways to do that. For example to set "s->krt_src" earlier, but I do not know if it will not break something else. Or may be it is even better to add additional field to "struct nl_parse_state" for that.
Thanks, fixed. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
participants (5)
-
Alexander Zubkov -
Bernd Naumann -
Ondrej Zajicek -
Vincent Bernat -
Vincent Bernat