Download raw body.
ospfd: change imsg handling for kroute messages
On Tue, Sep 08, 2026 at 06:58:29PM +0200, Theo Buehler wrote:
> 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.
This comes from the time when static was verboten in the kernel and people
applied it to userland as well.
Fixing this is something for another day since ideally all the other
functions should be changed as well.
> > 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.
Damn. Fixed.
> >
> > - 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);
> >
>
--
:wq Claudio
ospfd: change imsg handling for kroute messages