svn commit: r347982 - head/sys/net
Alexander V. Chernikov
melifaro at freebsd.org
Mon May 20 22:46:32 UTC 2019
20.05.2019, 07:50, "Rodney W. Grimes" <freebsd at 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 at freebsd.org
More information about the svn-src-all
mailing list