From: Theo Buehler Subject: Re: ospfd: change imsg handling for kroute messages To: tech@openbsd.org Date: Tue, 8 Sep 2026 18:58:29 +0200 On Tue, Sep 08, 2026 at 10:57:03AM +0200, Claudio Jeker wrote: > kroute change and delete messages fumble directly with imsg.data. > I like to fix this and instead pass the imsg to these functions. > > kr_delete is simple, just extract the kroute and off you go. > In kr_change this is more complex since multiple struct kroute are sent > for multipath routes. > > I split the kr_change_fib code into two versions. > kr_change_one() does an RTM_CHANGE for a single kroute. > By doing that kr_change_fib() becomes a bit simpler since that code now > handles only RTM_ADD. This reduces the complexity of that function. > Internally this still uses ibuf_data() to access the array of kroutes. > I did this since the code really needs to access this as an array and it > uses the right bounds. > > Using diff -w will show that kr_change_fib() was actually only minimally > changed. Reads ok to me. I only have two very important comments. > Index: kroute.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v > diff -u -p -r1.121 kroute.c > --- kroute.c 31 Aug 2026 08:57:41 -0000 1.121 > +++ kroute.c 8 Sep 2026 08:21:31 -0000 > @@ -74,7 +74,8 @@ int kr_redist_eval(struct kroute *, stru > void kr_redistribute(struct kroute_node *); > int kroute_compare(struct kroute_node *, struct kroute_node *); > int kif_compare(struct kif_node *, struct kif_node *); > -int kr_change_fib(struct kroute_node *, struct kroute *, int, int); > +int kr_change_one(struct kroute_node *, struct kroute *); > +int kr_change_fib(struct kroute_node *, struct ibuf *); It is weird that these aren't static. > int kr_delete_fib(struct kroute_node *); > > struct kroute_node *kroute_find(in_addr_t, u_int8_t, u_int8_t); > @@ -200,37 +201,59 @@ kr_init(int fs, u_int rdomain, int redis > } > > int > -kr_change_fib(struct kroute_node *kr, struct kroute *kroute, int krcount, > - int action) > +kr_change_one(struct kroute_node *kr, struct kroute *kroute) > { > - int i; > + /* nexthop within 127/8 -> ignore silently */ > + if ((kroute->nexthop.s_addr & htonl(IN_CLASSA_NET)) == > + htonl(INADDR_LOOPBACK & IN_CLASSA_NET)) > + return (0); > + > + /* send update */ > + if (send_rtmsg(kr_state.fd, RTM_CHANGE, kroute) == -1) > + return (-1); > + > + /* modify first and only entry */ > + kr->r.nexthop.s_addr = kroute->nexthop.s_addr; > + kr->r.flags = kroute->flags | F_OSPFD_INSERTED; > + kr->r.ext_tag = kroute->ext_tag; > + rtlabel_unref(kr->r.rtlabel); > + kr->r.rtlabel = rtlabel_tag2id(kroute->ext_tag); > + rtlabel_ref(kr->r.rtlabel); > + > + return (0); > +} > + > +int > +kr_change_fib(struct kroute_node *kr, struct ibuf *ibuf) > +{ > + unsigned int i, krcount; > struct kroute_node *kn, *nkn; > + struct kroute *kroute; You leave a doubled empty line here. > > - if (action == RTM_ADD) { > - /* > - * First remove all stale multipath routes. > - * This step must be skipped when the action is RTM_CHANGE > - * because it is already a single path route that will be > - * changed. > - */ > - for (kn = kr; kn != NULL; kn = nkn) { > - for (i = 0; i < krcount; i++) { > - if (kn->r.nexthop.s_addr == > - kroute[i].nexthop.s_addr) > - break; > - } > - nkn = kn->next; > - if (i == krcount) { > - /* stale route */ > - if (kr_delete_fib(kn) == -1) > - log_warnx("kr_delete_fib failed"); > - /* > - * if head element was removed we need to adjust > - * the head > - */ > - if (kr == kn) > - kr = nkn; > - } > + > + krcount = ibuf_size(ibuf) / sizeof(*kroute); > + kroute = ibuf_data(ibuf); > + > + /* > + * First remove all stale multipath routes. > + */ > + for (kn = kr; kn != NULL; kn = nkn) { > + for (i = 0; i < krcount; i++) { > + if (kn->r.nexthop.s_addr == > + kroute[i].nexthop.s_addr) > + break; > + } > + nkn = kn->next; > + if (i == krcount) { > + /* stale route */ > + if (kr_delete_fib(kn) == -1) > + log_warnx("kr_delete_fib failed"); > + /* > + * if head element was removed we need to adjust > + * the head > + */ > + if (kr == kn) > + kr = nkn; > } > } > > @@ -243,41 +266,36 @@ kr_change_fib(struct kroute_node *kr, st > htonl(INADDR_LOOPBACK & IN_CLASSA_NET)) > continue; > > - if (action == RTM_ADD && kr) { > - for (kn = kr; kn != NULL; kn = kn->next) { > - if (kn->r.nexthop.s_addr == > - kroute[i].nexthop.s_addr) > - break; > - } > + for (kn = kr; kn != NULL; kn = kn->next) { > + if (kn->r.nexthop.s_addr == > + kroute[i].nexthop.s_addr) > + break; > + } > > - if (kn != NULL) { > - uint16_t label; > + if (kn != NULL) { > + uint16_t label; > > - /* nexthop already present, just change it */ > - kn->r.flags = kroute[i].flags | > - F_OSPFD_INSERTED; > - kn->r.ext_tag = kroute[i].ext_tag; > - label = kn->r.rtlabel; > - rtlabel_unref(kn->r.rtlabel); > - kn->r.rtlabel = rtlabel_tag2id(kn->r.ext_tag); > - rtlabel_ref(kn->r.rtlabel); > - > - /* update if label changed */ > - if (kn->r.rtlabel != label) { > - if (send_rtmsg(kr_state.fd, RTM_CHANGE, > - &kn->r) == -1) > - return (-1); > - } > - continue; > + /* nexthop already present, just change it */ > + kn->r.flags = kroute[i].flags | > + F_OSPFD_INSERTED; > + kn->r.ext_tag = kroute[i].ext_tag; > + label = kn->r.rtlabel; > + rtlabel_unref(kn->r.rtlabel); > + kn->r.rtlabel = rtlabel_tag2id(kn->r.ext_tag); > + rtlabel_ref(kn->r.rtlabel); > + > + /* update if label changed */ > + if (kn->r.rtlabel != label) { > + if (send_rtmsg(kr_state.fd, RTM_CHANGE, > + &kn->r) == -1) > + return (-1); > } > - } else > - /* modify first entry */ > - kn = kr; > - > - /* create new entry unless we are changing the first entry */ > - if (action == RTM_ADD) > - if ((kn = calloc(1, sizeof(*kn))) == NULL) > - fatal(NULL); > + continue; > + } > + > + /* create new entry */ > + if ((kn = calloc(1, sizeof(*kn))) == NULL) > + fatal(NULL); > > kn->r.prefix.s_addr = kroute[i].prefix.s_addr; > kn->r.prefixlen = kroute[i].prefixlen; > @@ -286,35 +304,45 @@ kr_change_fib(struct kroute_node *kr, st > > kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED; > kn->r.ext_tag = kroute[i].ext_tag; > - rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */ > kn->r.rtlabel = rtlabel_tag2id(kn->r.ext_tag); > rtlabel_ref(kn->r.rtlabel); > > - if (action == RTM_ADD) > - kroute_insert(kn); > + kroute_insert(kn); > > /* send update */ > - if (send_rtmsg(kr_state.fd, action, &kn->r) == -1) > + if (send_rtmsg(kr_state.fd, RTM_ADD, &kn->r) == -1) > return (-1); > - > - action = RTM_ADD; > } > + > return (0); > } > > int > -kr_change(struct kroute *kroute, int krcount) > +kr_change(struct imsg *imsg) > { > + struct ibuf ibuf; > + struct kroute kroute; > struct kroute_node *kr; > - int action = RTM_ADD; > > - kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, > + if (imsg_get_ibuf(imsg, &ibuf) == -1) > + fatalx("bad KROUTE_CHANGE imsg received"); > + > + if (ibuf_size(&ibuf) % sizeof(kroute) != 0) > + fatalx("bad KROUTE_CHANGE imsg received"); > + > + if (ibuf_get(&ibuf, &kroute, sizeof(kroute)) == -1) > + fatalx("bad KROUTE_CHANGE imsg received"); > + > + kr = kroute_find(kroute.prefix.s_addr, kroute.prefixlen, > kr_state.fib_prio); > - if (kr != NULL && kr->next == NULL && krcount == 1) > - /* single path OSPF route */ > - action = RTM_CHANGE; > > - return (kr_change_fib(kr, kroute, krcount, action)); > + if (kr != NULL && kr->next == NULL && ibuf_size(&ibuf) == 0) { > + /* single path OSPF route, do inline change */ > + return (kr_change_one(kr, &kroute)); > + } > + > + ibuf_rewind(&ibuf); > + return (kr_change_fib(kr, &ibuf)); > } > > int > @@ -334,11 +362,15 @@ kr_delete_fib(struct kroute_node *kr) > } > > int > -kr_delete(struct kroute *kroute) > +kr_delete(struct imsg *imsg) > { > + struct kroute kroute; > struct kroute_node *kr, *nkr; > > - if ((kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, > + if (imsg_get_data(imsg, &kroute, sizeof(kroute)) == -1) > + fatalx("bad KROUTE_DELETE imsg received"); > + > + if ((kr = kroute_find(kroute.prefix.s_addr, kroute.prefixlen, > kr_state.fib_prio)) == NULL) > return (0); > > Index: ospfd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > diff -u -p -r1.128 ospfd.c > --- ospfd.c 17 Aug 2026 08:58:47 -0000 1.128 > +++ ospfd.c 8 Sep 2026 08:22:39 -0000 > @@ -446,7 +446,7 @@ main_dispatch_rde(int fd, short event, v > struct imsgev *iev = bula; > struct imsgbuf *ibuf; > struct imsg imsg; > - int n, count, shut = 0; > + int n, shut = 0; > > ibuf = &iev->ibuf; > > @@ -473,14 +473,12 @@ main_dispatch_rde(int fd, short event, v > > switch (imsg.hdr.type) { > case IMSG_KROUTE_CHANGE: > - count = (imsg.hdr.len - IMSG_HEADER_SIZE) / > - sizeof(struct kroute); > - if (kr_change(imsg.data, count)) > + if (kr_change(&imsg)) > log_warn("main_dispatch_rde: error changing " > "route"); > break; > case IMSG_KROUTE_DELETE: > - if (kr_delete(imsg.data)) > + if (kr_delete(&imsg)) > log_warn("main_dispatch_rde: error deleting " > "route"); > break; > Index: ospfd.h > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v > diff -u -p -r1.110 ospfd.h > --- ospfd.h 27 Aug 2026 21:18:41 -0000 1.110 > +++ ospfd.h 8 Sep 2026 08:21:31 -0000 > @@ -580,8 +580,8 @@ u_int16_t iso_cksum(void *, u_int16_t, > int kif_init(void); > void kif_clear(void); > int kr_init(int, u_int, int, u_int8_t); > -int kr_change(struct kroute *, int); > -int kr_delete(struct kroute *); > +int kr_change(struct imsg *); > +int kr_delete(struct imsg *); > void kr_shutdown(void); > void kr_fib_couple(void); > void kr_fib_decouple(void); >