tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: [PATCH] Deletion of route scrubs ifa flag IFA_ROUTE even if it's not the automatic route



David Young wrote:
On Mon, Mar 09, 2009 at 10:16:27AM +0000, Roy Marples wrote:
On Tue, 2009-03-03 at 22:47 +0000, Roy Marples wrote:
To make ammends, attached is a patch to fix this. It is intended for
pullup to netbsd-5.
And here is a different patch.
We add a new function, rt_ifa_connected which returns 1 if the route is
the connected route (prefix, subnet, etc) for the ifa.
This makes the code in rtsock.c and route.c a lot simpler. I've also
added some debug messages via the existing RT_DPRINTF define.

For reference, I've also added a patch which applies this before my
origonal patch was comitted to show the intent of what we're trying to
do here.

I think this pretty much covers everything now. Anyone have any issues
with this before I commit?

Please separate the cosmetic changes from the functional changes
before you commit.

Avoid accessing _rt_key except by using the accessor, rt_getkey().

Perhaps the following code logically belongs in rt_replace_ifa()?
Yes, it makes more sense there.

I will commit these patches tomorrow one after the other.
rtsock-revert.diff reverts rtsock r1.119 restoring it to it's pristine state before I touched it.
route-ifaroute.diff has the new fix and is purely functional.

I've elected to keep to the style of the current code and keeping _rt_key. If it should be changed, then it should be done in another patch imo, which doesn't have to be pulled up. Remember, I'm targeting this for pullup to NetBSD-5.

Thanks

Roy
Index: sys/net/rtsock.c
===================================================================
RCS file: /cvsroot/src/sys/net/rtsock.c,v
retrieving revision 1.123
diff -u -p -r1.123 rtsock.c
--- sys/net/rtsock.c    20 Feb 2009 10:54:57 -0000      1.123
+++ sys/net/rtsock.c    10 Mar 2009 20:41:24 -0000
@@ -222,9 +222,9 @@ route_output(struct mbuf *m, ...)
        struct rtentry *rt = NULL;
        struct rtentry *saved_nrt = NULL;
        struct rt_addrinfo info;
-       int len, error = 0, ifa_route = 0;
+       int len, error = 0;
        struct ifnet *ifp = NULL;
-       struct ifaddr *ifa = NULL, *oifa;
+       struct ifaddr *ifa = NULL;
        struct socket *so;
        va_list ap;
        sa_family_t family;
@@ -301,12 +301,6 @@ route_output(struct mbuf *m, ...)
                error = rtrequest1(rtm->rtm_type, &info, &saved_nrt);
                if (error == 0) {
                        (rt = saved_nrt)->rt_refcnt++;
-                       ifa = rt_get_ifa(rt);
-                       /*
-                        * If deleting an automatic route, scrub the flag.
-                        */
-                       if (ifa->ifa_flags & IFA_ROUTE)
-                               ifa->ifa_flags &= ~IFA_ROUTE;
                        goto report;
                }
                break;
@@ -424,28 +418,13 @@ route_output(struct mbuf *m, ...)
                            rt_getkey(rt), info.rti_info[RTAX_GATEWAY])))) {
                                ifp = ifa->ifa_ifp;
                        }
-                       oifa = rt->rt_ifa;
-                       if (oifa && oifa->ifa_flags & IFA_ROUTE) {
-                               /*
-                                * If changing an automatically added route,
-                                * remove the flag and store the fact.
-                                */
-                               oifa->ifa_flags &= ~IFA_ROUTE;
-                               ifa_route = 1;
-                       }
                        if (ifa) {
+                               struct ifaddr *oifa = rt->rt_ifa;
                                if (oifa != ifa) {
                                        if (oifa && oifa->ifa_rtrequest) {
                                                oifa->ifa_rtrequest(RTM_DELETE,
                                                    rt, &info);
                                        }
-                                       /*
-                                        * If changing an automatically added
-                                        * route, store this if not static.
-                                        */
-                                       if (ifa_route &&
-                                           !(rt->rt_flags & RTF_STATIC))
-                                               ifa->ifa_flags |= IFA_ROUTE;
                                        rt_replace_ifa(rt, ifa);
                                        rt->rt_ifp = ifp;
                                }
Index: sys/net/route.c
===================================================================
RCS file: /cvsroot/src/sys/net/route.c,v
retrieving revision 1.115
diff -u -p -r1.115 route.c
--- sys/net/route.c     20 Feb 2009 10:57:19 -0000      1.115
+++ sys/net/route.c     10 Mar 2009 20:41:45 -0000
@@ -192,9 +192,50 @@ rt_set_ifa1(struct rtentry *rt, struct i
                rt->rt_ifa_seqno = *ifa->ifa_seqno;
 }
 
+/*
+ * Is this route the connected route for the ifa?
+ */
+static int
+rt_ifa_connected(const struct rtentry *rt, const struct ifaddr *ifa)
+{
+       const struct sockaddr *key, *dst, *odst;
+       struct sockaddr_storage maskeddst;
+
+       key = rt_getkey(rt);
+       dst = rt->rt_flags & RTF_HOST ? ifa->ifa_dstaddr : ifa->ifa_addr;
+       if (dst == NULL ||
+           dst->sa_family != key->sa_family ||
+           dst->sa_len != key->sa_len)
+               return 0;
+       if ((rt->rt_flags & RTF_HOST) == 0 && ifa->ifa_netmask) {
+               odst = dst;
+               dst = (struct sockaddr *)&maskeddst;
+               rt_maskedcopy(odst, (struct sockaddr *)&maskeddst,
+                   ifa->ifa_netmask);
+       }
+       return (memcmp(dst, key, dst->sa_len) == 0);
+}
+
 void
 rt_replace_ifa(struct rtentry *rt, struct ifaddr *ifa)
 {
+       if (rt->rt_ifa &&
+           rt->rt_ifa != ifa &&
+           rt->rt_ifa->ifa_flags & IFA_ROUTE &&
+           rt_ifa_connected(rt, rt->rt_ifa))
+       {
+               RT_DPRINTF("rt->_rt_key = %p, ifa = %p, "
+                   "replace deleted IFA_ROUTE\n",
+                   (void *)rt->_rt_key, (void *)rt->rt_ifa);
+               rt->rt_ifa->ifa_flags &= ~IFA_ROUTE;
+               if (rt_ifa_connected(rt, ifa)) {
+                       RT_DPRINTF("rt->_rt_key = %p, ifa = %p, "
+                           "replace added IFA_ROUTE\n",
+                           (void *)rt->_rt_key, (void *)ifa);
+                       ifa->ifa_flags |= IFA_ROUTE;
+               }
+       }
+
        IFAREF(ifa);
        IFAFREE(rt->rt_ifa);
        rt_set_ifa1(rt, ifa);
@@ -672,8 +713,17 @@ rtrequest1(int req, struct rt_addrinfo *
                        rt->rt_parent = NULL;
                }
                rt->rt_flags &= ~RTF_UP;
-               if ((ifa = rt->rt_ifa) && ifa->ifa_rtrequest)
-                       ifa->ifa_rtrequest(RTM_DELETE, rt, info);
+               if ((ifa = rt->rt_ifa)) {
+                       if (ifa->ifa_flags & IFA_ROUTE &&
+                           rt_ifa_connected(rt, ifa)) {
+                               RT_DPRINTF("rt->_rt_key = %p, ifa = %p, "
+                                   "deleted IFA_ROUTE\n",
+                                   (void *)rt->_rt_key, (void *)ifa);
+                               ifa->ifa_flags &= ~IFA_ROUTE;
+                       }
+                       if (ifa->ifa_rtrequest)
+                               ifa->ifa_rtrequest(RTM_DELETE, rt, info);
+               }
                rttrash++;
                if (ret_nrt)
                        *ret_nrt = rt;


Home | Main Index | Thread Index | Old Index