On Mon, Mar 21, 2016 at 05:11:04PM +0100, Peter Hessler wrote:
> On 2016 Mar 21 (Mon) at 16:22:53 +0100 (+0100), Claudio Jeker wrote:
> :On Mon, Mar 21, 2016 at 03:54:38PM +0100, Peter Hessler wrote:
> :> We ran into a situation where we accidentally blackholed traffic going to
> :> a new Internet Exchange. When we added the new vlans and new peers, the
> :> nexthop address on that vlan was *not* our neighbor's address, but
> :> instead used our own IP address on that new interface. "bgpctl show rib",
> :> showed the correct IP, but the wrong one was used when installing into
> :> the fib.
> :>
> :> Restarting bgpd installed the correct nexthop.
> :>
> :> Additionally, a user had reported that "network inet connected" didn't
> :> work any more. This fix also convinces bgpd to redistribute the network
> :> as soon as it is created. The underlying cause was a combination of
> :> missing the F_CONNECTED flag, as well as having an ifindex of 0, which
> :> is lo0's index.
> :>
> :> OK?
> :
> :I noticed this breakage as well some time ago but never managed to debug
> :it (too many production systems and not enough test systems).
> :This was caused by the change of cloning routes to show the IP address
> :instead of the link local one.
> :Now about the diff I'm not 100% sure. It for sure changes beheviour a bit.
> :ifindex is now always set and not only for connected routes. Not sure if
> :this is bad or not...
> :mpath should probably be set to 0 in the RTF_CONNECTED case to keep same
> :behaviour. On the other hand it is now possible to have multiple routes to
> :the same network even for connected routes (not sure if it is possible on
> :the same prio or not).
> :
> :IMO this is a more conservative version...
> :
> : case AF_INET:
> : case AF_INET6:
> : if (rtm->rtm_flags & RTF_CONNECTED) {
> : flags |= F_CONNECTED;
> : ifindex = rtm->rtm_index;
> : sa = NULL;
> : mpath = 0;
> : }
> : break;
> :
> :I think this is less dangerous or what you think?
>
> setting ifindex should always be safe, because that is the ifindex that
> is valid for that route. Setting it to zero (lo0) is Wrong(tm).
This is where I'm not sure. Since it is well possible that kroute
considers 0 as not directly connected. I just fear that by having a
non-zero ifindex on non connected routes we may end up doing something
else different. This is why I'm at the moment somewhat conservative. In
the long run you are right. ifindex should always be set and correct.
> I'm not 100% convinced about mpath=0, or about setting sa=NULL, so I
> left those alone for this version. I did see that we only used sa when
> we already had a route, not when we are creating one. I don't have
> super-strong feelings about them, though.
The idea here is to emulate the old code as much as possible.
If we set the sa may we end up with a nexthop route to the wrong (as in
our) IP (which is currently the case)?
> Since mpi@ says that RTAX_GATEWAY now returns the af, I could also simply
> add those as FALLTHROUGH for the main switch statement...
You need to add the RTF_CONNECTED case because any other route in your
system will have a AF_INET or AF_INET6 nexthop. This is more or less what
my code snipped is trying to do.
> :
> :> Index: kroute.c
> :> ===================================================================
> :> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> :> retrieving revision 1.207
> :> diff -u -p -u -p -r1.207 kroute.c
> :> --- kroute.c 5 Dec 2015 18:28:04 -0000 1.207
> :> +++ kroute.c 21 Mar 2016 14:44:52 -0000
> :> @@ -3178,6 +3178,12 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
> :> sa = NULL;
> :> mpath = 0; /* link local stuff can't be mpath */
> :> break;
> :> + case AF_INET:
> :> + case AF_INET6:
> :> + if (rtm->rtm_flags & RTF_CONNECTED)
> :> + flags |= F_CONNECTED;
> :> + ifindex = rtm->rtm_index;
> :> + break;
> :> }
> :>
> :> if (rtm->rtm_type == RTM_DELETE) {
> :>
> :>
> :>
> :>
> :> --
> :> If you are a fatalist, what can you do about it?
> :> -- Ann Edwards-Duff
> :
> :--
> ::wq Claudio
>
> --
> Death is nature's way of telling you to slow down.
>
--
:wq Claudio
