On Wed, Sep 26, 2018 at 08:14:09PM +0200, Sebastian Benoit wrote:
> Jon Williams([email protected]) on 2018.09.25 16:51:25 -0400:
> > >Synopsis: Newly connected rtlabel networks are not advertised by bgpd
> > >Category: system
> > >Environment:
> > System : OpenBSD 6.3
> > Details : OpenBSD 6.3 (GENERIC.MP) #107: Sat Mar 24 14:21:59 MDT 2018
> > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > Architecture: OpenBSD.amd64
> > Machine : amd64
> > >Description:
> > "network (inet|inet6) rtlabel" should pick up new routes with the label.
> > >How-To-Repeat:
> > 1) Run bgpd with a configuration containing:
> >
> > network inet rtlabel export
> >
> > Add a route with this label and start bgpd.
> >
> > `bgpctl show network` will contain this route
> >
> > Delete the route and recreate it. `bgpctl show network` will *not*
> > contain this route.
> >
> > 2) Run bgpd with a configuration containing:
> >
> > network inet static
> >
> > Add a static route and start bgpd.
> >
> > `bgpctl show network` will contain this route
> >
> > Delete the route and recreate it. `bgpctl show network` will contain this
> > route.
>
>
> Here is a possible fix for your problem, please test.
>
> Diff is against -current.
>
> If you apply this on 6.3, diff will show
>
> Hunk #1 succeeded at 3146 (offset -31 lines).
> ...
>
> That should be ok.
>
> diff --git usr.sbin/bgpd/kroute.c usr.sbin/bgpd/kroute.c
> index 99bda3c50a3..21a2ab27a8c 100644
> --- usr.sbin/bgpd/kroute.c
> +++ usr.sbin/bgpd/kroute.c
> @@ -3177,10 +3177,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rtm, struct
> sockaddr *rti_info[RTAX_MAX],
> struct sockaddr *sa;
> struct sockaddr_in *sa_in;
> struct sockaddr_in6 *sa_in6;
> + struct sockaddr_rtlabel *label;
> struct kroute_node *kr;
> struct kroute6_node *kr6;
> struct bgpd_addr prefix;
> int flags, oflags, mpath = 0, changed = 0;
> + int rtlabel_changed = 0;
> u_int16_t ifindex;
> u_int8_t prefixlen;
> u_int8_t prio;
> @@ -3209,6 +3211,8 @@ dispatch_rtmsg_addr(struct rt_msghdr *rtm, struct
> sockaddr *rti_info[RTAX_MAX],
> #endif
>
> prio = rtm->rtm_priority;
> + label = (struct sockaddr_rtlabel *)rti_info[RTAX_LABEL];
> +
> switch (sa->sa_family) {
> case AF_INET:
> prefix.aid = AID_INET;
> @@ -3340,10 +3344,37 @@ dispatch_rtmsg_addr(struct rt_msghdr *rtm, struct
> sockaddr *rti_info[RTAX_MAX],
>
> if (kr->r.flags & F_NEXTHOP)
> flags |= F_NEXTHOP;
> +
> + if (label != NULL) {
> + rtlabel_unref(kr->r.labelid);
> + kr->r.labelid = 0;
> + flags |= F_RTLABEL;
> + kr->r.labelid =
> + rtlabel_name2id(label->sr_label);
Do you want to check here if the label remaind the same before setting
rtlabel_changed? At least it would reduce the work done when a RTM_CHANGE
did not modify the label.
> + rtlabel_changed = 1;
> + } else if (kr->r.labelid && label == NULL) {
> + rtlabel_unref(kr->r.labelid);
> + kr->r.labelid = 0;
> + flags &= ~F_RTLABEL;
> + rtlabel_changed = 1;
> + }
> +
> oflags = kr->r.flags;
> if (flags != oflags)
> changed = 1;
> kr->r.flags = flags;
> +
> + if (rtlabel_changed) {
> + if (oflags & F_REDISTRIBUTED) {
> + kr->r.flags |= F_REDISTRIBUTED;
> + kr_redistribute(
> + IMSG_NETWORK_REMOVE, kt,
> + &kr->r);
> + }
> + kr_redistribute(IMSG_NETWORK_ADD,
> + kt, &kr->r);
> + }
> +
> if ((oflags & F_CONNECTED) &&
> !(flags & F_CONNECTED)) {
> kif_kr_remove(kr);
> @@ -3380,6 +3411,9 @@ add4:
> kr->r.ifindex = ifindex;
> kr->r.priority = prio;
>
> + kr->r.flags |= F_RTLABEL;
> + kr->r.labelid = rtlabel_name2id(label->sr_label);
> +
This here feel wrong. There should be some check that the label is
actually present before setting F_RTLABEL and dereferencing label.
> kroute_insert(kt, kr);
> }
> break;
> @@ -3418,10 +3452,37 @@ add4:
>
> if (kr6->r.flags & F_NEXTHOP)
> flags |= F_NEXTHOP;
> +
> + if (label != NULL) {
> + rtlabel_unref(kr6->r.labelid);
> + kr6->r.labelid = 0;
> + flags |= F_RTLABEL;
> + kr6->r.labelid =
> + rtlabel_name2id(label->sr_label);
> + rtlabel_changed = 1;
> + } else if (kr6->r.labelid && label == NULL) {
> + rtlabel_unref(kr6->r.labelid);
> + kr6->r.labelid = 0;
> + flags &= ~F_RTLABEL;
> + rtlabel_changed = 1;
> + }
> +
> oflags = kr6->r.flags;
> if (flags != oflags)
> changed = 1;
> kr6->r.flags = flags;
> +
> + if (rtlabel_changed) {
> + if (oflags & F_REDISTRIBUTED) {
> + kr6->r.flags |= F_REDISTRIBUTED;
> + kr_redistribute6(
> + IMSG_NETWORK_REMOVE, kt,
> + &kr6->r);
> + }
> + kr_redistribute6(IMSG_NETWORK_ADD,
> + kt, &kr6->r);
> + }
> +
> if ((oflags & F_CONNECTED) &&
> !(flags & F_CONNECTED)) {
> kif_kr6_remove(kr6);
> @@ -3434,6 +3495,7 @@ add4:
> kr_redistribute6(IMSG_NETWORK_ADD,
> kt, &kr6->r);
> }
> +
> if (kr6->r.flags & F_NEXTHOP && changed)
> knexthop_track(kt, kr6);
> }
> @@ -3461,6 +3523,9 @@ add6:
> kr6->r.ifindex = ifindex;
> kr6->r.priority = prio;
>
> + kr6->r.flags |= F_RTLABEL;
> + kr6->r.labelid = rtlabel_name2id(label->sr_label);
> +
> kroute6_insert(kt, kr6);
> }
> break;
>
--
:wq Claudio
