Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 21 May 2019 01:46:17 +0300
From:      Alexander V. Chernikov <melifaro@freebsd.org>
To:        "rgrimes@freebsd.org" <rgrimes@freebsd.org>
Cc:        "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
Subject:   Re: svn commit: r347982 - head/sys/net
Message-ID:  <17209181558392377@myt4-ea6eba8eca77.qloud-c.yandex.net>
References:  <201905192149.x4JLnvt5028495@repo.freebsd.org> <201905200450.x4K4o9uI072252@gndrsh.dnsmgr.net>

next in thread | previous in thread | raw e-mail | index | archive | help
20.05.2019, 07:50, "Rodney W. Grimes" <freebsd@gndrsh.dnsmgr.net>:
>>  Author: melifaro
>>  Date: Sun May 19 21:49:56 2019
>>  New Revision: 347982
>>  URL: https://svnweb.freebsd.org/changeset/base/347982
>>
>>  Log:
>>    Fix rt_ifa selection during loopback route insertion process.
>>      Currently such routes are added with a link-level IFA, which is
>>      plain wrong. Only after the insertion they get fixed by the special
>>      link_rtrequest() ifa handler. This behaviour complicates routing code
>>      and makes ifa selection more complex.
>>    Streamline this process by explicitly moving link_rtrequest() logic
>>      to the pre-insertion rt_getifa_fib() ifa selector. Avoid calling all
>>      this logic in the loopback route case by explicitly specifying
>>      proper rt_ifa inside the ifa_maintain_loopback_route().?
>>
>>    MFC after: 2 weeks
>>    Differential Revision: https://reviews.freebsd.org/D20076
>
> I shall again state that from a routing protocol perspecitive
> and a POLA perspective having the kernel doing route maintanance
> of any kind is fundementally wrong.
>
> I still continue to stronly object to ifa_maintain_loopback_route
> code even being present in our kernel. Having these routes
> is a micro optimization at best, and cause issues when real
> and actual routing protocols are in use.
I agree with that. It indeed causes decent amount of complications and I (as another router type person) prefer to have it gone as well.
However, one has to carefully check uRPF functionality in our firewalls, implement fib-aware in_localip_more (and its IPv6 counterpart) to make it happen.
>
> Bruce Evans and myself have locally killed this code, and
> just about every router type person I show it to gets ill
> seeing it.
>
> Show many another system that does this and I might reconsider,
> but I have never ever seen one.
>
>>  Modified:
>>    head/sys/net/if.c
>>    head/sys/net/route.c
>>
>>  Modified: head/sys/net/if.c
>>  ==============================================================================
>>  --- head/sys/net/if.c Sun May 19 20:28:49 2019 (r347981)
>>  +++ head/sys/net/if.c Sun May 19 21:49:56 2019 (r347982)
>>  @@ -264,7 +264,6 @@ static void if_route(struct ifnet *, int flag, int fam
>>   static int if_setflag(struct ifnet *, int, int, int *, int);
>>   static int if_transmit(struct ifnet *ifp, struct mbuf *m);
>>   static void if_unroute(struct ifnet *, int flag, int fam);
>>  -static void link_rtrequest(int, struct rtentry *, struct rt_addrinfo *);
>>   static int if_delmulti_locked(struct ifnet *, struct ifmultiaddr *, int);
>>   static void do_link_state_change(void *, int);
>>   static int if_getgroup(struct ifgroupreq *, struct ifnet *);
>>  @@ -862,7 +861,6 @@ if_attach_internal(struct ifnet *ifp, int vmove, struc
>>                   sdl->sdl_type = ifp->if_type;
>>                   ifp->if_addr = ifa;
>>                   ifa->ifa_ifp = ifp;
>>  - ifa->ifa_rtrequest = link_rtrequest;
>>                   ifa->ifa_addr = (struct sockaddr *)sdl;
>>                   sdl = (struct sockaddr_dl *)(socksize + (caddr_t)sdl);
>>                   ifa->ifa_netmask = (struct sockaddr *)sdl;
>>  @@ -1892,6 +1890,7 @@ static int
>>   ifa_maintain_loopback_route(int cmd, const char *otype, struct ifaddr *ifa,
>>       struct sockaddr *ia)
>>   {
>>  + struct epoch_tracker et;
>>           int error;
>>           struct rt_addrinfo info;
>>           struct sockaddr_dl null_sdl;
>>  @@ -1902,6 +1901,16 @@ ifa_maintain_loopback_route(int cmd, const char *otype
>>           bzero(&info, sizeof(info));
>>           if (cmd != RTM_DELETE)
>>                   info.rti_ifp = V_loif;
>>  + if (cmd == RTM_ADD) {
>>  + /* explicitly specify (loopback) ifa */
>>  + if (info.rti_ifp != NULL) {
>>  + NET_EPOCH_ENTER(et);
>>  + info.rti_ifa = ifaof_ifpforaddr(ifa->ifa_addr, info.rti_ifp);
>>  + if (info.rti_ifa != NULL)
>>  + ifa_ref(info.rti_ifa);
>>  + NET_EPOCH_EXIT(et);
>>  + }
>>  + }
>>           info.rti_flags = ifa->ifa_flags | RTF_HOST | RTF_STATIC | RTF_PINNED;
>>           info.rti_info[RTAX_DST] = ia;
>>           info.rti_info[RTAX_GATEWAY] = (struct sockaddr *)&null_sdl;
>>  @@ -2208,39 +2217,6 @@ ifa_preferred(struct ifaddr *cur, struct ifaddr *next)
>>
>>           return (cur->ifa_carp && (!next->ifa_carp ||
>>               ((*carp_master_p)(next) && !(*carp_master_p)(cur))));
>>  -}
>>  -
>>  -#include <net/if_llatbl.h>
>>  -
>>  -/*
>>  - * Default action when installing a route with a Link Level gateway.
>>  - * Lookup an appropriate real ifa to point to.
>>  - * This should be moved to /sys/net/link.c eventually.
>>  - */
>>  -static void
>>  -link_rtrequest(int cmd, struct rtentry *rt, struct rt_addrinfo *info)
>>  -{
>>  - struct epoch_tracker et;
>>  - struct ifaddr *ifa, *oifa;
>>  - struct sockaddr *dst;
>>  - struct ifnet *ifp;
>>  -
>>  - if (cmd != RTM_ADD || ((ifa = rt->rt_ifa) == NULL) ||
>>  - ((ifp = ifa->ifa_ifp) == NULL) || ((dst = rt_key(rt)) == NULL))
>>  - return;
>>  - NET_EPOCH_ENTER(et);
>>  - ifa = ifaof_ifpforaddr(dst, ifp);
>>  - if (ifa) {
>>  - oifa = rt->rt_ifa;
>>  - if (oifa != ifa) {
>>  - ifa_free(oifa);
>>  - ifa_ref(ifa);
>>  - }
>>  - rt->rt_ifa = ifa;
>>  - if (ifa->ifa_rtrequest && ifa->ifa_rtrequest != link_rtrequest)
>>  - ifa->ifa_rtrequest(cmd, rt, info);
>>  - }
>>  - NET_EPOCH_EXIT(et);
>>   }
>>
>>   struct sockaddr_dl *
>>
>>  Modified: head/sys/net/route.c
>>  ==============================================================================
>>  --- head/sys/net/route.c Sun May 19 20:28:49 2019 (r347981)
>>  +++ head/sys/net/route.c Sun May 19 21:49:56 2019 (r347982)
>>  @@ -1276,12 +1276,14 @@ rt_notifydelete(struct rtentry *rt, struct rt_addrinfo
>>   /*
>>    * Look up rt_addrinfo for a specific fib. Note that if rti_ifa is defined,
>>    * it will be referenced so the caller must free it.
>>  + *
>>  + * Assume basic consistency checks are executed by callers:
>>  + * RTAX_DST exists, if RTF_GATEWAY is set, RTAX_GATEWAY exists as well.
>>    */
>>   int
>>   rt_getifa_fib(struct rt_addrinfo *info, u_int fibnum)
>>   {
>>           struct epoch_tracker et;
>>  - struct ifaddr *ifa;
>>           int needref, error;
>>
>>           /*
>>  @@ -1291,21 +1293,54 @@ rt_getifa_fib(struct rt_addrinfo *info, u_int fibnum)
>>           error = 0;
>>           needref = (info->rti_ifa == NULL);
>>           NET_EPOCH_ENTER(et);
>>  +
>>  + /* If we have interface specified by the ifindex in the address, use it */
>>           if (info->rti_ifp == NULL && ifpaddr != NULL &&
>>  - ifpaddr->sa_family == AF_LINK &&
>>  - (ifa = ifa_ifwithnet(ifpaddr, 0, fibnum)) != NULL) {
>>  - info->rti_ifp = ifa->ifa_ifp;
>>  + ifpaddr->sa_family == AF_LINK) {
>>  + const struct sockaddr_dl *sdl = (const struct sockaddr_dl *)ifpaddr;
>>  + if (sdl->sdl_index != 0)
>>  + info->rti_ifp = ifnet_byindex_locked(sdl->sdl_index);
>>           }
>>  + /*
>>  + * If we have source address specified, try to find it
>>  + * TODO: avoid enumerating all ifas on all interfaces.
>>  + */
>>           if (info->rti_ifa == NULL && ifaaddr != NULL)
>>                   info->rti_ifa = ifa_ifwithaddr(ifaaddr);
>>           if (info->rti_ifa == NULL) {
>>                   struct sockaddr *sa;
>>
>>  - sa = ifaaddr != NULL ? ifaaddr :
>>  - (gateway != NULL ? gateway : dst);
>>  - if (sa != NULL && info->rti_ifp != NULL)
>>  + /*
>>  + * Most common use case for the userland-supplied routes.
>>  + *
>>  + * Choose sockaddr to select ifa.
>>  + * -- if ifp is set --
>>  + * Order of preference:
>>  + * 1) IFA address
>>  + * 2) gateway address
>>  + * Note: for interface routes link-level gateway address
>>  + * is specified to indicate the interface index without
>>  + * specifying RTF_GATEWAY. In this case, ignore gateway
>>  + * Note: gateway AF may be different from dst AF. In this case,
>>  + * ignore gateway
>>  + * 3) final destination.
>>  + * 4) if all of these fails, try to get at least link-level ifa.
>>  + * -- else --
>>  + * try to lookup gateway or dst in the routing table to get ifa
>>  + */
>>  + if (info->rti_info[RTAX_IFA] != NULL)
>>  + sa = info->rti_info[RTAX_IFA];
>>  + else if ((info->rti_flags & RTF_GATEWAY) != 0 &&
>>  + gateway->sa_family == dst->sa_family)
>>  + sa = gateway;
>>  + else
>>  + sa = dst;
>>  + if (info->rti_ifp != NULL) {
>>                           info->rti_ifa = ifaof_ifpforaddr(sa, info->rti_ifp);
>>  - else if (dst != NULL && gateway != NULL)
>>  + /* Case 4 */
>>  + if (info->rti_ifa == NULL && gateway != NULL)
>>  + info->rti_ifa = ifaof_ifpforaddr(gateway, info->rti_ifp);
>>  + } else if (dst != NULL && gateway != NULL)
>>                           info->rti_ifa = ifa_ifwithroute(flags, dst, gateway,
>>                                                           fibnum);
>>                   else if (sa != NULL)
>
> --
> Rod Grimes rgrimes@freebsd.org





Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?17209181558392377>