Download raw body.
ospfd: change imsg handling for kroute messages
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);
>
ospfd: change imsg handling for kroute messages