Re: rtdeletemsg & KASSERT
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 03 - 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 - 1.292
> +++ net/route.c 16 Dec 2015 22:42:34 -
> @@ -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 intifidx;
> - 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), 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, _mask);
> - info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> - info.rti_flags = rt->rt_flags;
> - ifidx = rt->rt_ifidx;
> - error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
> - rt_missmsg(RTM_DELETE, , 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
Re: rtdeletemsg & KASSERT
Now I got a panic with this diff.
login: panic: kernel diagnostic assertion "!ISSET(rt->rt_flags, RTF_UP)"
failed: file "../../../../net/route.c", line 444
Stopped at Debugger+0x9: leave
TIDPIDUID PRFLAGS PFLAGS CPU COMMAND
Debugger() at Debugger+0x9
panic() at panic+0xfe
__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
softclock() at softclock+0x315
softintr_dispatch() at softintr_dispatch+0x72
Xsoftclock() at Xsoftclock+0x1f
--- interrupt ---
end of kernel
end trace frame: 0x2710, count: 3
0x8:
http://www.openbsd.org/ddb.html describes the minimum info required in bug
reports. Insufficient info makes it difficult to find and fix bugs.
It happened after running the ARP regression test. I cannot reproduce
it by running the test. Perhaps I have to wait for the arp timer.
The kernel is running on a single CPU qemu.
bluhm
On Wed, Dec 16, 2015 at 07:59:40PM +0100, Alexander Bluhm wrote:
> 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 - 1.292
> +++ net/route.c 16 Dec 2015 18:40:16 -
> @@ -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 intifidx;
> - 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), 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, _mask);
> - info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> - info.rti_flags = rt->rt_flags;
> - ifidx = rt->rt_ifidx;
> - error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
> - rt_missmsg(RTM_DELETE, , 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
> -
Re: rtdeletemsg & KASSERT
On Wed, Dec 16, 2015 at 08:47:02PM +0100, Alexander Bluhm wrote: > It happened after running the ARP regression test. I cannot reproduce > it by running the test. Perhaps I have to wait for the arp timer. Reproduceable by waiting for the arp timeout, then it crashes. I will investigate. bluhm root@q70:.../~# netstat -nrf inet Routing tables Internet: DestinationGatewayFlags Refs Use Mtu Prio Iface 10.188.70/24 10.188.70.70 UC 01 - 4 vio0 10.188.70.17 fe:e1:ba:d0:d5:6d UHLS 03 - 8 vio0 10.188.70.70 70:5f:ca:21:8d:70 UHLl 0 14 - 1 vio0 10.188.70.255 10.188.70.70 UHb00 - 1 vio0 10.188.211/24 10.188.211.70 UC 00 - 4 vio1 10.188.211.70 70:5f:ca:21:8d:80 UHLl 01 - 1 vio1 10.188.211.255 10.188.211.70 UHb00 - 1 vio1 10.188.212/24 10.188.211.71 UGS00 - 8 vio1 10.188.213/24 10.188.211.71 UGS00 - 8 vio1 10.188.217/24 127.0.0.1 UGRS 00 32768 8 lo0 10.188.218/24 127.0.0.1 UGRS 00 32768 8 lo0 127/8 127.0.0.1 UGRS 00 32768 8 lo0 127.0.0.1 127.0.0.1 UHl00 32768 1 lo0 224/4 127.0.0.1 URS00 32768 8 lo0 root@q70:.../~# panic: kernel diagnostic assertion "!ISSET(rt->rt_flags, RTF_UP)" failed: file "../../../../net/route.c", line 444 Stopped at Debugger+0x9: leave TIDPIDUID PRFLAGS PFLAGS CPU COMMAND Debugger() at Debugger+0x9 panic() at panic+0xfe __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 softclock() at softclock+0x315 softintr_dispatch() at softintr_dispatch+0x72 Xsoftclock() at Xsoftclock+0x1f --- interrupt --- end of kernel end trace frame: 0x2710, count: 3 0x8: http://www.openbsd.org/ddb.html describes the minimum info required in bug reports. Insufficient info makes it difficult to find and fix bugs. ddb>
Re: rtdeletemsg & KASSERT
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=144943901304713=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 - 1.292
+++ net/route.c 16 Dec 2015 18:40:16 -
@@ -159,8 +159,7 @@ struct sockaddr *rt_plentosa(sa_family_t
struct ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr *,
u_int);
-intrtrequest_delete(struct rt_addrinfo *, u_int8_t, struct ifnet *,
- struct rtentry **, u_int);
+intrt_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 intifidx;
- 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), 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, _mask);
- info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
- info.rti_flags = rt->rt_flags;
- ifidx = rt->rt_ifidx;
- error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
- rt_missmsg(RTM_DELETE, , 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 =
Re: rtdeletemsg & KASSERT
On Wed, Dec 16, 2015 at 09:46:26PM +0100, Alexander Bluhm wrote:
> 10.188.70.17 fe:e1:ba:d0:d5:6d UHLS 03 - 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.
bluhm
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 - 1.292
+++ net/route.c 16 Dec 2015 22:42:34 -
@@ -159,8 +159,7 @@ struct sockaddr *rt_plentosa(sa_family_t
struct ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr *,
u_int);
-intrtrequest_delete(struct rt_addrinfo *, u_int8_t, struct ifnet *,
- struct rtentry **, u_int);
+intrt_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 intifidx;
- 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), 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, _mask);
- info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
- info.rti_flags = rt->rt_flags;
- ifidx = rt->rt_ifidx;
- error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
- rt_missmsg(RTM_DELETE, , 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);
+
rtdeletemsg & KASSERT
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=144943901304713=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.
Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.289
diff -u -p -r1.289 route.c
--- net/route.c 5 Dec 2015 10:07:55 - 1.289
+++ net/route.c 7 Dec 2015 15:10:15 -
@@ -159,8 +159,7 @@ struct sockaddr *rt_plentosa(sa_family_t
struct ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr *,
u_int);
-intrtrequest_delete(struct rt_addrinfo *, u_int8_t, struct ifnet *,
- struct rtentry **, u_int);
+intrt_delete(struct rtentry *, struct ifnet *, unsigned int);
#ifdef DDB
void db_print_sa(struct sockaddr *);
@@ -613,38 +612,29 @@ 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 intifidx;
- struct sockaddr_in6 sa_mask;
- /*
-* 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), 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, _mask);
- info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
- info.rti_flags = rt->rt_flags;
- ifidx = rt->rt_ifidx;
- error = rtrequest_delete(, rt->rt_priority, ifp, , tableid);
- rt_missmsg(RTM_DELETE, , info.rti_flags, ifidx, error, tableid);
+ KASSERT(rt->rt_ifidx == ifp->if_index);
+
+ error = rt_delete(rt, ifp, rtableid);
if (error == 0)
- rtfree(rt);
+ rt_sendmsg(rt, RTM_DELETE, rtableid);
+
return (error);
}
static inline int
rtequal(struct rtentry *a, struct rtentry *b)
{
+ if (a == b)
+ return 1;
+
if (memcmp(rt_key(a), rt_key(b), rt_key(a)->sa_len) == 0 &&
rt_plen(a) == rt_plen(b))
return 1;
@@ -656,10 +646,25 @@ int
rtflushclone1(struct rtentry *rt, void *arg, u_int id)
{
struct rtentry *parent = arg;
+ struct ifnet *ifp;
+
+ ifp = if_get(rt->rt_ifidx);
+
+ /*
+* This happens when an interface with a RTF_CLONING route is
+* being detached. In this case it's safe to bail because all
+* the routes are being purged by rt_if_remove().
+*/
+ if (ifp == NULL)
+ return 0;
- if ((rt->rt_flags & RTF_CLONED) != 0 && (rt->rt_parent == parent ||
- rtequal(rt->rt_parent, parent)))
- rtdeletemsg(rt, NULL, 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;
}
@@ -801,53 +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)
