On Tue, Jun 14, 2022 at 08:14:45PM +0200, Theo Buehler wrote:
> 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?
I decided against this because send_rtmsg() should not alter the kroute
node. In the end I want the kroute code to be a module and have the rtsock
code to be just a few hooks. This should make the code a lot more portable
with far less maintenance work.
> 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];
> >
>
--
:wq Claudio
