Hi Martin,
On Mon, 4 Sep 2017 14:18:50 +0200 Martin Pieuchot <m...@openbsd.org> wrote:
> On 04/09/17(Mon) 13:10, Gerhard Roth wrote:
> > Hi,
> >
> > I noticed a problem with the routing table that is easy to reproduce: put
> > multiple IPs on the same carp interface:
>
> Great bug analysis, however returning EAGAIN for every route update is a
> no go if you have a big routing table like BGP full feeds.
>
> We should return EAGAIN only if the multipath list contains multiple
> elements.
>
> But since we're now returning EAGAIN much often we want to skip route
> entries that have already been borough UP/taken down.
>
> Diff below does that, does it work for you? Do you mind adding a
> regression test?
I can confirm that your version works for me. Thanks for the improvement.
Regarding the regression test: gimme some time ;)
Gerhard
>
> Index: net/route.c
> ===================================================================
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.366
> diff -u -p -r1.366 route.c
> --- net/route.c 11 Aug 2017 21:24:19 -0000 1.366
> +++ net/route.c 4 Sep 2017 12:08:16 -0000
> @@ -1550,6 +1550,7 @@ rt_if_linkstate_change(struct rtentry *r
> {
> struct ifnet *ifp = arg;
> struct sockaddr_in6 sa_mask;
> + int error;
>
> if (rt->rt_ifidx != ifp->if_index)
> return (0);
> @@ -1561,38 +1562,37 @@ rt_if_linkstate_change(struct rtentry *r
> }
>
> if (LINK_STATE_IS_UP(ifp->if_link_state) && ifp->if_flags & IFF_UP) {
> - if (!(rt->rt_flags & RTF_UP)) {
> - /* bring route up */
> - rt->rt_flags |= RTF_UP;
> - rtable_mpath_reprio(id, rt_key(rt),
> - rt_plen2mask(rt, &sa_mask),
> - rt->rt_priority & RTP_MASK, rt);
> - }
> + if (ISSET(rt->rt_flags, RTF_UP))
> + return (0);
> +
> + /* bring route up */
> + rt->rt_flags |= RTF_UP;
> + error = rtable_mpath_reprio(id, rt_key(rt),
> + rt_plen2mask(rt, &sa_mask), rt->rt_priority & RTP_MASK, rt);
> } else {
> - if (rt->rt_flags & RTF_UP) {
> - /*
> - * Remove redirected and cloned routes (mainly ARP)
> - * from down interfaces so we have a chance to get
> - * new routes from a better source.
> - */
> - if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
> - !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
> - int error;
> -
> - if ((error = rtdeletemsg(rt, ifp, id)))
> - return (error);
> - return (EAGAIN);
> - }
> - /* take route down */
> - rt->rt_flags &= ~RTF_UP;
> - rtable_mpath_reprio(id, rt_key(rt),
> - rt_plen2mask(rt, &sa_mask),
> - rt->rt_priority | RTP_DOWN, rt);
> + /*
> + * Remove redirected and cloned routes (mainly ARP)
> + * from down interfaces so we have a chance to get
> + * new routes from a better source.
> + */
> + if (ISSET(rt->rt_flags, RTF_CLONED|RTF_DYNAMIC) &&
> + !ISSET(rt->rt_flags, RTF_CACHED|RTF_BFD)) {
> + if ((error = rtdeletemsg(rt, ifp, id)))
> + return (error);
> + return (EAGAIN);
> }
> +
> + if (!ISSET(rt->rt_flags, RTF_UP))
> + return (0);
> +
> + /* take route down */
> + rt->rt_flags &= ~RTF_UP;
> + error = rtable_mpath_reprio(id, rt_key(rt),
> + rt_plen2mask(rt, &sa_mask), rt->rt_priority | RTP_DOWN, rt);
> }
> if_group_routechange(rt_key(rt), rt_plen2mask(rt, &sa_mask));
>
> - return (0);
> + return (error);
> }
>
> struct sockaddr *
> Index: net/rtable.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 rtable.c
> --- net/rtable.c 30 Jul 2017 18:18:08 -0000 1.61
> +++ net/rtable.c 4 Sep 2017 12:02:00 -0000
> @@ -751,6 +751,7 @@ rtable_mpath_reprio(unsigned int rtablei
> rt->rt_priority = prio;
> rtable_mpath_insert(an, rt);
> rtfree(rt);
> + error = EAGAIN;
> }
> rw_exit_write(&ar->ar_lock);
>
