On Mon, Dec 07, 2015 at 04:36:17PM +0100, Martin Pieuchot wrote:
> The rtrequest_delete() refactoring exposed an existing bug and
> introduced a regression, both triggered by the same KASSERT().
>
> The regression has been reported there:
> https://marc.info/?l=openbsd-bugs&m=144943901304713&w=2
>
> The problem is that rt_if_remove() will triggers a rtflushclone1() if a
> RTF_CLONING route is attached to an interface. In this case
> rtdeletemsg() tries to get the interface pointer from rt_ifidx and the
> KASSERT() fires.
>
> The bug exposed by the same KASSERT() can be triggered by a call to
> rt_ifa_del() when dhclient(8) tries to add an address already configured
> on the system. In this case the kernel calls rtrequest_delete() with
> a wrong ifp.
>
> Diff below fixes these two bugs. It's big because I don't want to put
> two customs checks in a generic function. This diff is built upon my
> previous rtdeletemsg() one.
>
> It introduces rt_delete() which deletes a route *without* doing a
> lookup. There's various good reasons for that.
> - First of all I don't want to take the risk of matching a similar
> multipath route. The rt_ifa_del() bug is an example.
> - Secondly I'd like to avoid a recursive rtable_* call when routes
> are deleted from rtable_walk().
> - Then I'd rather keep userland-specific checks inside rtrequest()
> which will ends up in rtsock.c one day.
> - Finally it cleans one use of "struct rt_addrinfo" which should be
> limited to userland in order to get rid of RTA_NETMASK in kernel.
>
> Note that it introduces a behavior change. If rtdeletemsg() fails to
> delete a route no message is reported to userland. I believe this was
> only used for debugging.
OK bluhm@
I have merged the diff to -current before reviewing. There it looks
like this.
Index: net/route.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/route.c,v
retrieving revision 1.292
diff -u -p -r1.292 route.c
--- net/route.c 11 Dec 2015 08:58:23 -0000 1.292
+++ net/route.c 16 Dec 2015 18:40:16 -0000
@@ -159,8 +159,7 @@ struct sockaddr *rt_plentosa(sa_family_t
struct ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr *,
u_int);
-int rtrequest_delete(struct rt_addrinfo *, u_int8_t, struct ifnet *,
- struct rtentry **, u_int);
+int rt_delete(struct rtentry *, struct ifnet *, unsigned int);
#ifdef DDB
void db_print_sa(struct sockaddr *);
@@ -613,34 +612,20 @@ out:
}
/*
- * Delete a route and generate a message
+ * Delete a route and generate a message. The caller must hold a reference
+ * on ``rt''.
*/
int
-rtdeletemsg(struct rtentry *rt, struct ifnet *ifp, u_int tableid)
+rtdeletemsg(struct rtentry *rt, struct ifnet *ifp, unsigned int rtableid)
{
int error;
- struct rt_addrinfo info;
- unsigned int ifidx;
- struct sockaddr_in6 sa_mask;
KASSERT(rt->rt_ifidx == ifp->if_index);
- /*
- * Request the new route so that the entry is not actually
- * deleted. That will allow the information being reported to
- * be accurate (and consistent with route_output()).
- */
- bzero((caddr_t)&info, sizeof(info));
- info.rti_info[RTAX_DST] = rt_key(rt);
- if (!ISSET(rt->rt_flags, RTF_HOST))
- info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, &sa_mask);
- info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
- info.rti_flags = rt->rt_flags;
- ifidx = rt->rt_ifidx;
- error = rtrequest_delete(&info, rt->rt_priority, ifp, &rt, tableid);
- rt_missmsg(RTM_DELETE, &info, info.rti_flags, ifidx, error, tableid);
+ error = rt_delete(rt, ifp, rtableid);
if (error == 0)
- rtfree(rt);
+ rt_sendmsg(rt, RTM_DELETE, rtableid);
+
return (error);
}
@@ -671,10 +656,13 @@ rtflushclone1(struct rtentry *rt, void *
* the routes are being purged by rt_if_remove().
*/
if (ifp == NULL)
- return 0;
+ return 0;
- if (ISSET(rt->rt_flags, RTF_CLONED) && rtequal(rt->rt_parent, parent))
- rtdeletemsg(rt, ifp, id);
+ if (ISSET(rt->rt_flags, RTF_CLONED) && rtequal(rt->rt_parent, parent)) {
+ rtref(rt);
+ rtdeletemsg(rt, ifp, id);
+ rtfree(rt);
+ }
if_put(ifp);
return 0;
@@ -818,60 +806,20 @@ rt_getifa(struct rt_addrinfo *info, u_in
}
int
-rtrequest_delete(struct rt_addrinfo *info, u_int8_t prio, struct ifnet *ifp,
- struct rtentry **ret_nrt, u_int tableid)
+rt_delete(struct rtentry *rt, struct ifnet *ifp, unsigned int rtableid)
{
- struct rtentry *rt;
- int error;
-
- splsoftassert(IPL_SOFTNET);
+ struct sockaddr_in6 sa_mask;
- if (!rtable_exists(tableid))
- return (EAFNOSUPPORT);
- rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
- info->rti_info[RTAX_NETMASK], info->rti_info[RTAX_GATEWAY], prio);
- if (rt == NULL)
- return (ESRCH);
+ KASSERT(ifp->if_index == rt->rt_ifidx);
- /* Make sure that's the route the caller want to delete. */
- if (ifp != NULL && ifp->if_index != rt->rt_ifidx) {
- rtfree(rt);
- return (ESRCH);
- }
-
-#ifndef SMALL_KERNEL
- /*
- * If we got multipath routes, we require users to specify
- * a matching gateway.
- */
- if ((rt->rt_flags & RTF_MPATH) &&
- info->rti_info[RTAX_GATEWAY] == NULL) {
- rtfree(rt);
- return (ESRCH);
- }
-#endif
-
- /*
- * Since RTP_LOCAL cannot be set by userland, make
- * sure that local routes are only modified by the
- * kernel.
- */
- if ((rt->rt_flags & (RTF_LOCAL|RTF_BROADCAST)) &&
- prio != RTP_LOCAL) {
- rtfree(rt);
- return (EINVAL);
- }
+ splsoftassert(IPL_SOFTNET);
- error = rtable_delete(tableid, info->rti_info[RTAX_DST],
- info->rti_info[RTAX_NETMASK], rt);
- if (error != 0) {
- rtfree(rt);
+ if (rtable_delete(rtableid, rt_key(rt), rt_plen2mask(rt, &sa_mask), rt))
return (ESRCH);
- }
/* clean up any cloned children */
if ((rt->rt_flags & RTF_CLONING) != 0)
- rtflushclone(tableid, rt);
+ rtflushclone(rtableid, rt);
rtfree(rt->rt_gwroute);
rt->rt_gwroute = NULL;
@@ -881,23 +829,10 @@ rtrequest_delete(struct rt_addrinfo *inf
rt->rt_flags &= ~RTF_UP;
- if (ifp == NULL) {
- ifp = if_get(rt->rt_ifidx);
- KASSERT(ifp != NULL);
- ifp->if_rtrequest(ifp, RTM_DELETE, rt);
- if_put(ifp);
- } else {
- KASSERT(ifp->if_index == rt->rt_ifidx);
- ifp->if_rtrequest(ifp, RTM_DELETE, rt);
- }
+ ifp->if_rtrequest(ifp, RTM_DELETE, rt);
atomic_inc_int(&rttrash);
- if (ret_nrt != NULL)
- *ret_nrt = rt;
- else
- rtfree(rt);
-
return (0);
}
@@ -924,9 +859,50 @@ rtrequest(int req, struct rt_addrinfo *i
info->rti_info[RTAX_NETMASK] = NULL;
switch (req) {
case RTM_DELETE:
- error = rtrequest_delete(info, prio, NULL, ret_nrt, tableid);
- if (error)
+ rt = rtable_lookup(tableid, info->rti_info[RTAX_DST],
+ info->rti_info[RTAX_NETMASK],
+ info->rti_info[RTAX_GATEWAY], prio);
+ if (rt == NULL)
+ return (ESRCH);
+
+#ifndef SMALL_KERNEL
+ /*
+ * If we got multipath routes, we require users to specify
+ * a matching gateway.
+ */
+ if (ISSET(rt->rt_flags, RTF_MPATH) &&
+ info->rti_info[RTAX_GATEWAY] == NULL) {
+ rtfree(rt);
+ return (ESRCH);
+ }
+#endif
+
+ /*
+ * Since RTP_LOCAL cannot be set by userland, make
+ * sure that local routes are only modified by the
+ * kernel.
+ */
+ if (ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST) &&
+ prio != RTP_LOCAL) {
+ rtfree(rt);
+ return (EINVAL);
+ }
+
+ ifp = if_get(rt->rt_ifidx);
+ KASSERT(ifp != NULL);
+ error = rt_delete(rt, ifp, tableid);
+ if_put(ifp);
+
+ if (error) {
+ rtfree(rt);
return (error);
+ }
+
+ if (ret_nrt != NULL)
+ *ret_nrt = rt;
+ else
+ rtfree(rt);
+
break;
case RTM_RESOLVE:
@@ -1265,8 +1241,8 @@ rt_ifa_del(struct ifaddr *ifa, int flags
struct rtentry *rt;
struct mbuf *m = NULL;
struct sockaddr *deldst;
- struct rt_addrinfo info;
- struct sockaddr_rtlabel sa_rl;
+ struct sockaddr *mask = NULL;
+ struct sockaddr *gateway = NULL;
unsigned int rtableid = ifp->if_rdomain;
uint8_t prio = ifp->if_priority + RTP_STATIC;
int error;
@@ -1286,16 +1262,10 @@ rt_ifa_del(struct ifaddr *ifa, int flags
dst = deldst;
}
- memset(&info, 0, sizeof(info));
- info.rti_ifa = ifa;
- info.rti_flags = flags;
- info.rti_info[RTAX_DST] = dst;
if ((flags & RTF_LLINFO) == 0)
- info.rti_info[RTAX_GATEWAY] = ifa->ifa_addr;
- info.rti_info[RTAX_LABEL] = rtlabel_id2sa(ifp->if_rtlabelid, &sa_rl);
-
+ gateway = ifa->ifa_addr;
if ((flags & RTF_HOST) == 0)
- info.rti_info[RTAX_NETMASK] = ifa->ifa_netmask;
+ mask = ifa->ifa_netmask;
if (flags & (RTF_LOCAL|RTF_BROADCAST))
prio = RTP_LOCAL;
@@ -1303,13 +1273,32 @@ rt_ifa_del(struct ifaddr *ifa, int flags
if (flags & RTF_CONNECTED)
prio = RTP_CONNECTED;
- error = rtrequest_delete(&info, prio, ifp, &rt, rtableid);
- if (error == 0) {
- rt_sendmsg(rt, RTM_DELETE, rtableid);
- if (flags & RTF_LOCAL)
- rt_sendaddrmsg(rt, RTM_DELADDR);
+ rt = rtable_lookup(rtableid, dst, mask, gateway, prio);
+ if (rt == NULL)
+ return (ESRCH);
+
+#ifndef SMALL_KERNEL
+ /*
+ * If we got multipath routes, we require users to specify
+ * a matching gateway.
+ */
+ if (ISSET(rt->rt_flags, RTF_MPATH) && gateway == NULL) {
rtfree(rt);
+ return (ESRCH);
}
+#endif
+
+ /* Make sure that's the route the caller want to delete. */
+ if (rt->rt_ifidx != ifp->if_index) {
+ rtfree(rt);
+ return (ESRCH);
+ }
+
+ error = rtdeletemsg(rt, ifp, rtableid);
+ if (error == 0 && (flags & RTF_LOCAL))
+ rt_sendaddrmsg(rt, RTM_DELADDR);
+ rtfree(rt);
+
if (m != NULL)
m_free(m);
@@ -1721,9 +1710,12 @@ rt_if_remove_rtdelete(struct rtentry *rt
struct ifnet *ifp = vifp;
if (rt->rt_ifidx == ifp->if_index) {
- int cloning = (rt->rt_flags & RTF_CLONING);
+ int error, cloning = ISSET(rt->rt_flags, RTF_CLONING);
- if (rtdeletemsg(rt, ifp, id) == 0 && cloning)
+ rtref(rt);
+ error = rtdeletemsg(rt, ifp, id);
+ rtfree(rt);
+ if (error == 0 && cloning)
return (EAGAIN);
}
@@ -1781,7 +1773,9 @@ rt_if_linkstate_change(struct rtentry *r
* clone a new route from a better source.
*/
if (rt->rt_flags & RTF_CLONED) {
+ rtref(rt);
rtdeletemsg(rt, ifp, id);
+ rtfree(rt);
return (0);
}
/* take route down */
