On Tue, Jun 14, 2022 at 06:36:13PM +0200, Claudio Jeker wrote:
> This diff does introduce a new flag for routes that have been inserted
> into the FIB. Now I actually renamed the F_BGPD_INSERTED flag to F_BGPD
> and reused F_BGPD_INSERTED as this new flag.
> Adjust and use the send_rtmsg() return value to set F_BGPD_INSERTED if the
> route message was successfully sent.
>
> Additionally this removes F_DYNAMIC and tracking of dynamic routes (routes
> generated by the kernel because of PMTU or some other event). A routing
> protocol does not need to work with them so just filter them out like ARP
> and ND routes.
>
> While there also just remove protect_lo() it is a function that is not
> really making anything better. There are other checks in place for
> 127.0.0.1 and ::1.
Would it not be cleaner to set or unset the F_BGPD_INSERTED flag
inside send_rt{,6}msg() depending on action instead of doing that
after inspecting the return code?
ok tb
>
> --
> :wq Claudio
>
> Index: bgpctl/bgpctl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.276
> diff -u -p -r1.276 bgpctl.c
> --- bgpctl/bgpctl.c 21 Mar 2022 10:16:23 -0000 1.276
> +++ bgpctl/bgpctl.c 14 Jun 2022 15:31:59 -0000
> @@ -633,14 +633,12 @@ fmt_fib_flags(uint16_t flags)
> else
> strlcpy(buf, "*", sizeof(buf));
>
> - if (flags & F_BGPD_INSERTED)
> + if (flags & F_BGPD)
> strlcat(buf, "B", sizeof(buf));
> else if (flags & F_CONNECTED)
> strlcat(buf, "C", sizeof(buf));
> else if (flags & F_STATIC)
> strlcat(buf, "S", sizeof(buf));
> - else if (flags & F_DYNAMIC)
> - strlcat(buf, "D", sizeof(buf));
> else
> strlcat(buf, " ", sizeof(buf));
>
> Index: bgpctl/output.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 output.c
> --- bgpctl/output.c 6 Feb 2022 09:52:32 -0000 1.20
> +++ bgpctl/output.c 14 Jun 2022 15:51:49 -0000
> @@ -46,7 +46,7 @@ show_head(struct parse_result *res)
> break;
> case SHOW_FIB:
> printf("flags: * = valid, B = BGP, C = Connected, "
> - "S = Static, D = Dynamic\n");
> + "S = Static\n");
> printf(" "
> "N = BGP Nexthop reachable via this route\n");
> printf(" r = reject route, b = blackhole route\n\n");
> Index: bgpctl/output_json.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 output_json.c
> --- bgpctl/output_json.c 21 Mar 2022 10:16:23 -0000 1.14
> +++ bgpctl/output_json.c 14 Jun 2022 15:32:05 -0000
> @@ -357,14 +357,12 @@ json_fib(struct kroute_full *kf)
> json_do_printf("prefix", "%s/%u", log_addr(&kf->prefix), kf->prefixlen);
> json_do_uint("priority", kf->priority);
> json_do_bool("up", !(kf->flags & F_DOWN));
> - if (kf->flags & F_BGPD_INSERTED)
> + if (kf->flags & F_BGPD)
> origin = "bgp";
> else if (kf->flags & F_CONNECTED)
> origin = "connected";
> else if (kf->flags & F_STATIC)
> origin = "static";
> - else if (kf->flags & F_DYNAMIC)
> - origin = "dynamic";
> else
> origin = "unknown";
> json_do_printf("origin", "%s", origin);
> Index: bgpctl/parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> retrieving revision 1.109
> diff -u -p -r1.109 parser.c
> --- bgpctl/parser.c 21 Mar 2022 10:16:23 -0000 1.109
> +++ bgpctl/parser.c 14 Jun 2022 15:29:05 -0000
> @@ -151,15 +151,15 @@ static const struct token t_show_summary
> };
>
> static const struct token t_show_fib[] = {
> - { NOTOKEN, "", NONE, NULL},
> - { FLAG, "connected", F_CONNECTED, t_show_fib},
> - { FLAG, "static", F_STATIC, t_show_fib},
> - { FLAG, "bgp", F_BGPD_INSERTED, t_show_fib},
> - { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
> - { KEYWORD, "table", NONE, t_show_fib_table},
> - { FAMILY, "", NONE, t_show_fib},
> - { ADDRESS, "", NONE, NULL},
> - { ENDTOKEN, "", NONE, NULL}
> + { NOTOKEN, "", NONE, NULL},
> + { FLAG, "connected", F_CONNECTED, t_show_fib},
> + { FLAG, "static", F_STATIC, t_show_fib},
> + { FLAG, "bgp", F_BGPD, t_show_fib},
> + { FLAG, "nexthop", F_NEXTHOP, t_show_fib},
> + { KEYWORD, "table", NONE, t_show_fib_table},
> + { FAMILY, "", NONE, t_show_fib},
> + { ADDRESS, "", NONE, NULL},
> + { ENDTOKEN, "", NONE, NULL}
> };
>
> static const struct token t_show_rib[] = {
> Index: bgpd/bgpd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
> retrieving revision 1.245
> diff -u -p -r1.245 bgpd.c
> --- bgpd/bgpd.c 9 Jun 2022 16:45:19 -0000 1.245
> +++ bgpd/bgpd.c 14 Jun 2022 15:33:34 -0000
> @@ -1123,9 +1123,9 @@ bgpd_filternexthop(struct kroute *kr, st
> return (0);
>
> if (cflags & BGPD_FLAG_NEXTHOP_BGP) {
> - if (kr && kr->flags & F_BGPD_INSERTED)
> + if (kr && kr->flags & F_BGPD)
> return (0);
> - if (kr6 && kr6->flags & F_BGPD_INSERTED)
> + if (kr6 && kr6->flags & F_BGPD)
> return (0);
> }
>
> Index: bgpd/bgpd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.428
> diff -u -p -r1.428 bgpd.h
> --- bgpd/bgpd.h 9 Jun 2022 16:45:19 -0000 1.428
> +++ bgpd/bgpd.h 14 Jun 2022 15:31:24 -0000
> @@ -74,13 +74,13 @@
>
> #define SOCKET_NAME "/var/run/bgpd.sock"
>
> -#define F_BGPD_INSERTED 0x0001
> +#define F_BGPD 0x0001
> #define F_KERNEL 0x0002
> #define F_CONNECTED 0x0004
> #define F_NEXTHOP 0x0008
> #define F_DOWN 0x0010
> #define F_STATIC 0x0020
> -#define F_DYNAMIC 0x0040
> +#define F_BGPD_INSERTED 0x0040
> #define F_REJECT 0x0080
> #define F_BLACKHOLE 0x0100
> #define F_LONGER 0x0200
> Index: bgpd/kroute.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.255
> diff -u -p -r1.255 kroute.c
> --- bgpd/kroute.c 14 Jun 2022 14:06:48 -0000 1.255
> +++ bgpd/kroute.c 14 Jun 2022 16:21:22 -0000
> @@ -172,7 +172,6 @@ struct kroute6_node *kroute6_match(struc
> void kroute_detach_nexthop(struct ktable *,
> struct knexthop_node *);
>
> -int protect_lo(struct ktable *);
> uint8_t prefixlen_classful(in_addr_t);
> uint8_t mask2prefixlen(in_addr_t);
> uint8_t mask2prefixlen6(struct sockaddr_in6 *);
> @@ -310,8 +309,6 @@ ktable_new(u_int rtableid, u_int rdomid,
> /* ... and load it */
> if (fetchtable(kt) == -1)
> return (-1);
> - if (protect_lo(kt) == -1)
> - return (-1);
>
> /* everything is up and running */
> kt->state = RECONF_REINIT;
> @@ -503,7 +500,7 @@ kr4_change(struct ktable *kt, struct kro
> kr->r.prefix.s_addr = kl->prefix.v4.s_addr;
> kr->r.prefixlen = kl->prefixlen;
> kr->r.nexthop.s_addr = kl->nexthop.v4.s_addr;
> - kr->r.flags = kl->flags | F_BGPD_INSERTED;
> + kr->r.flags = kl->flags | F_BGPD;
> kr->r.priority = RTP_MINE;
> kr->r.labelid = labelid;
>
> @@ -526,8 +523,8 @@ kr4_change(struct ktable *kt, struct kro
> kr->r.flags &= ~F_REJECT;
> }
>
> - if (send_rtmsg(kr_state.fd, action, kt, &kr->r) == -1)
> - return (-1);
> + if (send_rtmsg(kr_state.fd, action, kt, &kr->r))
> + kr->r.flags |= F_BGPD_INSERTED;
>
> return (0);
> }
> @@ -562,7 +559,7 @@ kr6_change(struct ktable *kt, struct kro
> kr6->r.prefixlen = kl->prefixlen;
> memcpy(&kr6->r.nexthop, &kl->nexthop.v6,
> sizeof(struct in6_addr));
> - kr6->r.flags = kl->flags | F_BGPD_INSERTED;
> + kr6->r.flags = kl->flags | F_BGPD;
> kr6->r.priority = RTP_MINE;
> kr6->r.labelid = labelid;
>
> @@ -586,8 +583,8 @@ kr6_change(struct ktable *kt, struct kro
> kr6->r.flags &= ~F_REJECT;
> }
>
> - if (send_rt6msg(kr_state.fd, action, kt, &kr6->r) == -1)
> - return (-1);
> + if (send_rt6msg(kr_state.fd, action, kt, &kr6->r))
> + kr6->r.flags |= F_BGPD_INSERTED;
>
> return (0);
> }
> @@ -634,7 +631,7 @@ krVPN4_change(struct ktable *kt, struct
> kr->r.prefix.s_addr = kl->prefix.v4.s_addr;
> kr->r.prefixlen = kl->prefixlen;
> kr->r.nexthop.s_addr = kl->nexthop.v4.s_addr;
> - kr->r.flags = kl->flags | F_BGPD_INSERTED | F_MPLS;
> + kr->r.flags = kl->flags | F_BGPD | F_MPLS;
> kr->r.priority = RTP_MINE;
> kr->r.labelid = labelid;
> kr->r.mplslabel = mplslabel;
> @@ -661,9 +658,8 @@ krVPN4_change(struct ktable *kt, struct
> kr->r.flags &= ~F_REJECT;
> }
>
> - if (send_rtmsg(kr_state.fd, action, kt, &kr->r) == -1)
> - return (-1);
> -
> + if (send_rtmsg(kr_state.fd, action, kt, &kr->r))
> + kr->r.flags |= F_BGPD_INSERTED;
> return (0);
> }
>
> @@ -710,7 +706,7 @@ krVPN6_change(struct ktable *kt, struct
> kr6->r.prefixlen = kl->prefixlen;
> memcpy(&kr6->r.nexthop, &kl->nexthop.v6,
> sizeof(struct in6_addr));
> - kr6->r.flags = kl->flags | F_BGPD_INSERTED | F_MPLS;
> + kr6->r.flags = kl->flags | F_BGPD | F_MPLS;
> kr6->r.priority = RTP_MINE;
> kr6->r.labelid = labelid;
> kr6->r.mplslabel = mplslabel;
> @@ -738,8 +734,8 @@ krVPN6_change(struct ktable *kt, struct
> kr6->r.flags &= ~F_REJECT;
> }
>
> - if (send_rt6msg(kr_state.fd, action, kt, &kr6->r) == -1)
> - return (-1);
> + if (send_rt6msg(kr_state.fd, action, kt, &kr6->r))
> + kr6->r.flags |= F_BGPD_INSERTED;
>
> return (0);
> }
> @@ -810,8 +806,7 @@ kr4_delete(struct ktable *kt, struct kro
> if (!(kr->r.flags & F_BGPD_INSERTED))
> return (0);
>
> - if (send_rtmsg(kr_state.fd, RTM_DELETE, kt, &kr->r) == -1)
> - return (-1);
> + send_rtmsg(kr_state.fd, RTM_DELETE, kt, &kr->r);
>
> if (kroute_remove(kt, kr) == -1)
> return (-1);
> @@ -831,8 +826,7 @@ kr6_delete(struct ktable *kt, struct kro
> if (!(kr6->r.flags & F_BGPD_INSERTED))
> return (0);
>
> - if (send_rt6msg(kr_state.fd, RTM_DELETE, kt, &kr6->r) == -1)
> - return (-1);
> + send_rt6msg(kr_state.fd, RTM_DELETE, kt, &kr6->r);
>
> if (kroute6_remove(kt, kr6) == -1)
> return (-1);
> @@ -852,8 +846,7 @@ krVPN4_delete(struct ktable *kt, struct
> if (!(kr->r.flags & F_BGPD_INSERTED))
> return (0);
>
> - if (send_rtmsg(kr_state.fd, RTM_DELETE, kt, &kr->r) == -1)
> - return (-1);
> + send_rtmsg(kr_state.fd, RTM_DELETE, kt, &kr->r);
>
> if (kroute_remove(kt, kr) == -1)
> return (-1);
> @@ -873,8 +866,7 @@ krVPN6_delete(struct ktable *kt, struct
> if (!(kr6->r.flags & F_BGPD_INSERTED))
> return (0);
>
> - if (send_rt6msg(kr_state.fd, RTM_DELETE, kt, &kr6->r) == -1)
> - return (-1);
> + send_rt6msg(kr_state.fd, RTM_DELETE, kt, &kr6->r);
>
> if (kroute6_remove(kt, kr6) == -1)
> return (-1);
> @@ -909,12 +901,15 @@ kr_fib_couple(u_int rtableid)
> kt->fib_sync = 1;
>
> RB_FOREACH(kr, kroute_tree, &kt->krt)
> - if ((kr->r.flags & F_BGPD_INSERTED))
> - send_rtmsg(kr_state.fd, RTM_ADD, kt, &kr->r);
> + if (kr->r.flags & F_BGPD) {
> + if (send_rtmsg(kr_state.fd, RTM_ADD, kt, &kr->r))
> + kr->r.flags |= F_BGPD_INSERTED;
> + }
> RB_FOREACH(kr6, kroute6_tree, &kt->krt6)
> - if ((kr6->r.flags & F_BGPD_INSERTED))
> - send_rt6msg(kr_state.fd, RTM_ADD, kt, &kr6->r);
> -
> + if (kr6->r.flags & F_BGPD) {
> + if (send_rt6msg(kr_state.fd, RTM_ADD, kt, &kr6->r))
> + kr6->r.flags |= F_BGPD_INSERTED;
> + }
> log_info("kernel routing table %u (%s) coupled", kt->rtableid,
> kt->descr);
> }
> @@ -942,11 +937,15 @@ kr_fib_decouple(u_int rtableid)
> return;
>
> RB_FOREACH(kr, kroute_tree, &kt->krt)
> - if ((kr->r.flags & F_BGPD_INSERTED))
> - send_rtmsg(kr_state.fd, RTM_DELETE, kt, &kr->r);
> + if ((kr->r.flags & F_BGPD_INSERTED)) {
> + if (send_rtmsg(kr_state.fd, RTM_DELETE, kt, &kr->r))
> + kr->r.flags &= ~F_BGPD_INSERTED;
> + }
> RB_FOREACH(kr6, kroute6_tree, &kt->krt6)
> - if ((kr6->r.flags & F_BGPD_INSERTED))
> - send_rt6msg(kr_state.fd, RTM_DELETE, kt, &kr6->r);
> + if ((kr6->r.flags & F_BGPD_INSERTED)) {
> + if (send_rt6msg(kr_state.fd, RTM_DELETE, kt, &kr6->r))
> + kr6->r.flags &= ~F_BGPD_INSERTED;
> + }
>
> kt->fib_sync = 0;
>
> @@ -1405,10 +1404,6 @@ kr_redistribute(int type, struct ktable
> if (!(kr->flags & F_KERNEL))
> return;
>
> - /* Dynamic routes are not redistributable. */
> - if (kr->flags & F_DYNAMIC)
> - return;
> -
> /*
> * We consider the loopback net and multicast addresses
> * as not redistributable.
> @@ -1455,10 +1450,6 @@ kr_redistribute6(int type, struct ktable
> if (!(kr6->flags & F_KERNEL))
> return;
>
> - /* Dynamic routes are not redistributable. */
> - if (kr6->flags & F_DYNAMIC)
> - return;
> -
> /*
> * We consider unspecified, loopback, multicast, link- and site-local,
> * IPv4 mapped and IPv4 compatible addresses as not redistributable.
> @@ -2568,39 +2559,6 @@ kroute_detach_nexthop(struct ktable *kt,
> * misc helpers
> */
>
> -int
> -protect_lo(struct ktable *kt)
> -{
> - struct kroute_node *kr;
> - struct kroute6_node *kr6;
> -
> - /* special protection for 127/8 */
> - if ((kr = calloc(1, sizeof(*kr))) == NULL) {
> - log_warn("%s", __func__);
> - return (-1);
> - }
> - kr->r.prefix.s_addr = htonl(INADDR_LOOPBACK & IN_CLASSA_NET);
> - kr->r.prefixlen = 8;
> - kr->r.flags = F_KERNEL|F_CONNECTED;
> -
> - if (RB_INSERT(kroute_tree, &kt->krt, kr) != NULL)
> - free(kr); /* kernel route already there, no problem */
> -
> - /* special protection for loopback */
> - if ((kr6 = calloc(1, sizeof(*kr6))) == NULL) {
> - log_warn("%s", __func__);
> - return (-1);
> - }
> - memcpy(&kr6->r.prefix, &in6addr_loopback, sizeof(kr6->r.prefix));
> - kr6->r.prefixlen = 128;
> - kr6->r.flags = F_KERNEL|F_CONNECTED;
> -
> - if (RB_INSERT(kroute6_tree, &kt->krt6, kr6) != NULL)
> - free(kr6); /* kernel route already there, no problem */
> -
> - return (0);
> -}
> -
> uint8_t
> prefixlen_classful(in_addr_t ina)
> {
> @@ -3016,7 +2974,7 @@ retry:
> log_info("route %s/%u vanished before delete",
> inet_ntoa(kroute->prefix),
> kroute->prefixlen);
> - return (0);
> + return (1);
> }
> }
> log_warn("%s: action %u, prefix %s/%u", __func__, hdr.rtm_type,
> @@ -3024,7 +2982,7 @@ retry:
> return (0);
> }
>
> - return (0);
> + return (1);
> }
>
> int
> @@ -3157,7 +3115,7 @@ retry:
> log_info("route %s/%u vanished before delete",
> log_in6addr(&kroute->prefix),
> kroute->prefixlen);
> - return (0);
> + return (1);
> }
> }
> log_warn("%s: action %u, prefix %s/%u", __func__, hdr.rtm_type,
> @@ -3165,7 +3123,7 @@ retry:
> return (0);
> }
>
> - return (0);
> + return (1);
> }
>
> int
> @@ -3432,8 +3390,8 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
> sa = (struct sockaddr *)((char *)rtm + rtm->rtm_hdrlen);
> get_rtaddrs(rtm->rtm_addrs, sa, rti_info);
>
> - /* Skip ARP/ND cache and broadcast routes. */
> - if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST))
> + /* Skip ARP/ND cache, broadcast and dynamic routes. */
> + if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST|RTF_DYNAMIC))
> return (-1);
>
> if ((sa = rti_info[RTAX_DST]) == NULL) {
> @@ -3450,8 +3408,6 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
> kl->flags |= F_BLACKHOLE;
> if (rtm->rtm_flags & RTF_REJECT)
> kl->flags |= F_REJECT;
> - if (rtm->rtm_flags & RTF_DYNAMIC)
> - kl->flags |= F_DYNAMIC;
>
> kl->priority = rtm->rtm_priority;
> label = (struct sockaddr_rtlabel *)rti_info[RTAX_LABEL];
>
