svn commit: r347982 - head/sys/net
Rodney W. Grimes
freebsd at gndrsh.dnsmgr.net
Mon May 20 04:50:15 UTC 2019
> 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.
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