On 16/12/15(Wed) 23:57, Alexander Bluhm wrote:
> On Wed, Dec 16, 2015 at 09:46:26PM +0100, Alexander Bluhm wrote:
> > 10.188.70.17 fe:e1:ba:d0:d5:6d UHLS 0 3 - 8 vio0
>
> This is this route that crashed the machine when the arp entry expired.
>
> When I move the rtref()/rtfree() calls into rtdeletemsg() it also
> protects the calls from arptfree().
>
> > __assert() at __assert+0x25
> > rtfree() at rtfree+0x11e
> > rtable_delete() at rtable_delete+0x5d
> > rt_delete() at rt_delete+0x6e
> > rtdeletemsg() at rtdeletemsg+0x2d
> > arptfree() at arptfree+0x4f
> > arptimer() at arptimer+0x63
>
> After rt_delete() has called rtable_delete(), the route is still
> used and should not be destroyed.
The diff below indeed corrects that, but I found another bug in it. The
problem is that rtable_lookup() might return a non perfect match so we
would now end up deleting the RTF_CLONING route instead of failing.
The proper solution would be to always pass a mask (or prefix length) to
rtable_lookup().
> 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 22:42:34 -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,22 @@ 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);
> + rtref(rt);
> + error = rt_delete(rt, ifp, rtableid);
> if (error == 0)
> - rtfree(rt);
> + rt_sendmsg(rt, RTM_DELETE, rtableid);
> + rtfree(rt);
> +
> return (error);
> }
>
> @@ -671,10 +658,10 @@ 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);
> + rtdeletemsg(rt, ifp, id);
>
> if_put(ifp);
> return 0;
> @@ -818,60 +805,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);
> -
> - 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);
> -
> - /* Make sure that's the route the caller want to delete. */
> - if (ifp != NULL && ifp->if_index != rt->rt_ifidx) {
> - rtfree(rt);
> - return (ESRCH);
> - }
> + struct sockaddr_in6 sa_mask;
>
> -#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
> + KASSERT(ifp->if_index == rt->rt_ifidx);
>
> - /*
> - * 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 +828,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 +858,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 +1240,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 +1261,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 +1272,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,7 +1709,7 @@ 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 cloning = ISSET(rt->rt_flags, RTF_CLONING);
>
> if (rtdeletemsg(rt, ifp, id) == 0 && cloning)
> return (EAGAIN);
>
