On Tue, Jun 20, 2023 at 05:31:34PM +0100, Stuart Henderson wrote: > This hasn't blown up yet... any interest?
Some comments to the kroute.c changes below. Everything else is fine. > ----- Forwarded message from Stuart Henderson <s...@spacehopper.org> ----- > > From: Stuart Henderson <s...@spacehopper.org> > Date: Fri, 26 May 2023 14:40:45 +0100 > To: tech@openbsd.org > Subject: ospf6d fib reload [Re: bgpd fix for possible crash in SE] > Mail-Followup-To: tech@openbsd.org > > On 2023/05/26 13:52, Stuart Henderson wrote: > > I think my main issues come around LS_REFRESH_TIME intervals, when > > there's loads of churn and "ospf6d: ospf engine" can be busy for > > minutes at a time (not always, but very often). Don't know if that rings > > any bells for anyone... (I am now reminded that RTM_DESYNC isn't handled > > by ospf6d which probably doesn't help matters). > > Here's a first attempt at porting the fib reload/desync diffs from > ospfd to ospf6d ... Not sure if it's good yet, but it didn't immediately > crash and burn when I ran "ospf6ctl fib reload", at least. > ... > Index: ospf6d/kroute.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospf6d/kroute.c,v > retrieving revision 1.67 > diff -u -p -r1.67 kroute.c > --- ospf6d/kroute.c 8 Mar 2023 04:43:14 -0000 1.67 > +++ ospf6d/kroute.c 26 May 2023 13:37:55 -0000 > @@ -45,16 +45,22 @@ struct { > u_int32_t rtseq; > pid_t pid; > int fib_sync; > + int fib_serial; > u_int8_t fib_prio; > int fd; > - struct event ev; > + struct event ev, reload; Please put reload on its own line. > u_int rdomain; > +#define KR_RELOAD_IDLE 0 > +#define KR_RELOAD_FETCH 1 > +#define KR_RELOAD_HOLD 2 > + int reload_state; > } kr_state; > > struct kroute_node { > RB_ENTRY(kroute_node) entry; > struct kroute_node *next; > struct kroute r; > + int serial; > }; > > void kr_redist_remove(struct kroute_node *, struct kroute_node *); > @@ -90,7 +96,10 @@ void if_announce(void *); > int send_rtmsg(int, int, struct kroute *); > int dispatch_rtmsg(void); > int fetchtable(void); > -int rtmsg_process(char *, size_t); > +int refetchtable(void); > +int rtmsg_process(char *, size_t); > +void kr_fib_reload_timer(int, short, void *); > +void kr_fib_reload_arm_timer(int); > > RB_HEAD(kroute_tree, kroute_node) krt; > RB_PROTOTYPE(kroute_tree, kroute_node, entry, kroute_compare) > @@ -165,6 +174,9 @@ kr_init(int fs, u_int rdomain, int redis > kr_dispatch_msg, NULL); > event_add(&kr_state.ev, NULL); > > + kr_state.reload_state = KR_RELOAD_IDLE; > + evtimer_set(&kr_state.reload, kr_fib_reload_timer, NULL); > + > return (0); > } > > @@ -374,6 +386,62 @@ kr_fib_decouple(void) > } > > void > +kr_fib_reload_timer(int fd, short event, void *bula) > +{ > + if (kr_state.reload_state == KR_RELOAD_FETCH) { > + kr_fib_reload(); > + kr_state.reload_state = KR_RELOAD_HOLD; > + kr_fib_reload_arm_timer(KR_RELOAD_HOLD_TIMER); > + } else { > + kr_state.reload_state = KR_RELOAD_IDLE; > + } > +} > + > +void > +kr_fib_reload_arm_timer(int delay) > +{ > + struct timeval tv; > + > + timerclear(&tv); > + tv.tv_sec = delay / 1000; > + tv.tv_usec = (delay % 1000) * 1000; > + > + if (evtimer_add(&kr_state.reload, &tv) == -1) > + fatal("add_reload_timer"); > +} > + > +void > +kr_fib_reload(void) > +{ > + struct kroute_node *krn, *kr, *kn; > + Maybe include the: log_info("reloading interface list and routing table"); line from ospfd/kroute.c here as well. > + kr_state.fib_serial++; > + > + if (fetchifs(0) != 0 || fetchtable() != 0) > + return; > + > + for (kr = RB_MIN(kroute_tree, &krt); kr != NULL; kr = krn) { > + krn = RB_NEXT(kroute_tree, &krt, kr); > + > + do { > + kn = kr->next; > + > + if (kr->serial != kr_state.fib_serial) { > + > + if (kr->r.priority == RTP_OSPF) { Here you should use kr_state.fib_prio instead of RTP_OSPF. > + kr->serial = kr_state.fib_serial; > + if (send_rtmsg(kr_state.fd, > + RTM_ADD, &kr->r) != 0) > + break; > + } else > + kroute_remove(kr); > + } > + > + } while ((kr = kn) != NULL); > + } > +} > + > +void > kr_fib_update_prio(u_int8_t fib_prio) > { > struct kroute_node *kr; > @@ -664,6 +732,8 @@ kroute_insert(struct kroute_node *kr) > { > struct kroute_node *krm, *krh; > > + kr->serial = kr_state.fib_serial; > + > if ((krh = RB_INSERT(kroute_tree, &krt, kr)) != NULL) { > /* > * Multipath route, add at end of list. > @@ -1279,7 +1349,7 @@ rtmsg_process(char *buf, size_t len) > int flags, mpath; > unsigned int scope; > u_short ifindex = 0; > - int rv; > + int rv, delay; > size_t offset; > char *next; > > @@ -1395,13 +1465,10 @@ rtmsg_process(char *buf, size_t len) > > if ((okr = kroute_find(&prefix, prefixlen, prio)) > != NULL) { > - /* just add new multipath routes */ > - if (mpath && rtm->rtm_type == RTM_ADD) > - goto add; > - /* get the correct route */ > kr = okr; > - if (mpath && (kr = kroute_matchgw(okr, > - &nexthop, scope)) == NULL) { > + if ((mpath || prio == kr_state.fib_prio) && > + (kr = kroute_matchgw(okr, &nexthop, scope)) > == > + NULL) { > log_warnx("rtmsg_process: mpath route" > " not found"); > /* add routes we missed out earlier */ > @@ -1432,13 +1499,15 @@ rtmsg_process(char *buf, size_t len) > kr->r.flags |= F_DOWN; > > /* just readd, the RDE will care */ > - kr_redistribute(okr); > + okr->serial = kr_state.fib_serial; > + kr_redistribute(kr); I think this should be: kr->serial = kr_state.fib_serial; kr_redistribute(okr); kr_redistribute() needs to be called with the head kroute but the serial needs to be set on the route itself. > } else { > add: > if ((kr = calloc(1, > sizeof(struct kroute_node))) == NULL) { > log_warn("rtmsg_process calloc"); > - return (-1); > + rv = -1; > + break; This change is wrong. This must remain as return (-1); > } > kr->r.prefix = prefix; > kr->r.prefixlen = prefixlen; > @@ -1517,6 +1586,23 @@ add: > break; > case RTM_IFANNOUNCE: > if_announce(next); > + break; > + case RTM_DESYNC: > + /* > + * We lost some routing packets. Schedule a reload > + * of the kernel route/interface information. > + */ > + if (kr_state.reload_state == KR_RELOAD_IDLE) { > + delay = KR_RELOAD_TIMER; > + log_info("desync; scheduling fib reload"); > + } else { > + delay = KR_RELOAD_HOLD_TIMER; > + log_debug("desync during KR_RELOAD_%s", > + kr_state.reload_state == > + KR_RELOAD_FETCH ? "FETCH" : "HOLD"); > + } > + kr_state.reload_state = KR_RELOAD_FETCH; > + kr_fib_reload_arm_timer(delay); > break; > default: > /* ignore for now */ As said above everything else is OK. -- :wq Claudio