Re: routing sockets vs KERNEL_LOCK()
On Tue, Jun 06, 2017 at 01:03:33PM +0200, Martin Pieuchot wrote:
> On 05/06/17(Mon) 16:52, Martin Pieuchot wrote:
> > On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
> > > Routing sockets are not protected by the NET_LOCK(). That's one of the
> > > boundaries of the network stack. That's whhy claudio@ sent some diffs
> > > to no longer require the KERNEL_LOCK() to protect them.
> > >
> > > But right now some rtm_* functions can be executed without
> > > KERNEL_LOCK(). That's wrong. Diff below fixes that and move the
> > > KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
> > > enough to protect the other data structures.
> > >
> > > The scariest bits come from default router advertisements, but I guess
> > > that slaacd(8) saved us ;)
> >
> > Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
> > rt_{set,put}gwroute(). Theses can now be called without KERNEL_LOCK()
> > when inserting an ARP entry.
>
> Hrvoje Popovski found the hard way that rtrequest(RTM_RESOLVE...) still
> need the KERNEL_LOCK() for malloc(9)/free(9).
>
> Here's a more conservative diff. ok?
>
Wondering why we don't push the lock into rtm_miss and rtm_send?
Apart from that I think we can start moving in that direction.
> Index: net/route.c
> ===
> RCS file: /cvs/src/sys/net/route.c,v
> retrieving revision 1.357
> diff -u -p -r1.357 route.c
> --- net/route.c 27 May 2017 09:51:18 - 1.357
> +++ net/route.c 6 Jun 2017 11:01:05 -
> @@ -385,7 +385,7 @@ rt_setgwroute(struct rtentry *rt, u_int
> {
> struct rtentry *nhrt;
>
> - KERNEL_ASSERT_LOCKED();
> + NET_ASSERT_LOCKED();
>
> KASSERT(ISSET(rt->rt_flags, RTF_GATEWAY));
>
> @@ -442,7 +442,7 @@ rt_putgwroute(struct rtentry *rt)
> {
> struct rtentry *nhrt = rt->rt_gwroute;
>
> - KERNEL_ASSERT_LOCKED();
> + NET_ASSERT_LOCKED();
>
> if (!ISSET(rt->rt_flags, RTF_GATEWAY) || nhrt == NULL)
> return;
> @@ -624,7 +624,9 @@ out:
> info.rti_info[RTAX_DST] = dst;
> info.rti_info[RTAX_GATEWAY] = gateway;
> info.rti_info[RTAX_AUTHOR] = src;
> + KERNEL_LOCK();
> rtm_miss(RTM_REDIRECT, &info, flags, prio, ifidx, error, rdomain);
> + KERNEL_UNLOCK();
> }
>
> /*
> @@ -653,8 +655,10 @@ rtdeletemsg(struct rtentry *rt, struct i
> info.rti_flags = rt->rt_flags;
> ifidx = rt->rt_ifidx;
> error = rtrequest_delete(&info, rt->rt_priority, ifp, &rt, tableid);
> + KERNEL_LOCK();
> rtm_miss(RTM_DELETE, &info, info.rti_flags, rt->rt_priority, ifidx,
> error, tableid);
> + KERNEL_UNLOCK();
> if (error == 0)
> rtfree(rt);
> return (error);
> Index: netinet/in_pcb.c
> ===
> RCS file: /cvs/src/sys/netinet/in_pcb.c,v
> retrieving revision 1.220
> diff -u -p -r1.220 in_pcb.c
> --- netinet/in_pcb.c 7 Mar 2017 16:59:40 - 1.220
> +++ netinet/in_pcb.c 6 Jun 2017 10:56:17 -
> @@ -716,8 +716,11 @@ in_losing(struct inpcb *inp)
> info.rti_info[RTAX_DST] = &inp->inp_route.ro_dst;
> info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
> info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, &sa_mask);
> +
> + KERNEL_LOCK();
> rtm_miss(RTM_LOSING, &info, rt->rt_flags, rt->rt_priority,
> rt->rt_ifidx, 0, inp->inp_rtableid);
> + KERNEL_UNLOCK();
> if (rt->rt_flags & RTF_DYNAMIC)
> (void)rtrequest(RTM_DELETE, &info, rt->rt_priority,
> NULL, inp->inp_rtableid);
> Index: netinet6/nd6_rtr.c
> ===
> RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
> retrieving revision 1.159
> diff -u -p -r1.159 nd6_rtr.c
> --- netinet6/nd6_rtr.c30 May 2017 08:58:34 - 1.159
> +++ netinet6/nd6_rtr.c6 Jun 2017 10:56:17 -
> @@ -613,7 +613,9 @@ defrouter_addreq(struct nd_defrouter *ne
> error = rtrequest(RTM_ADD, &info, RTP_DEFAULT, &rt,
> new->ifp->if_rdomain);
> if (error == 0) {
> + KERNEL_LOCK();
> rtm_send(rt, RTM_ADD, new->ifp->if_rdomain);
> + KERNEL_UNLOCK();
> rtfree(rt);
> new->installed = 1;
> }
> @@ -717,7 +719,9 @@ defrouter_delreq(struct nd_defrouter *dr
> error = rtrequest(RTM_DELETE, &info, RTP_DEFAULT, &rt,
> dr->ifp->if_rdomain);
> if (error == 0) {
> + KERNEL_LOCK();
> rtm_send(rt, RTM_DELETE, dr->ifp->if_rdomain);
> + KERNEL_UNLOCK();
> rtfree(rt);
> }
>
>
--
:wq Claudio
Re: routing sockets vs KERNEL_LOCK()
On 6.6.2017. 13:03, Martin Pieuchot wrote:
> On 05/06/17(Mon) 16:52, Martin Pieuchot wrote:
>> On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
>>> Routing sockets are not protected by the NET_LOCK(). That's one of the
>>> boundaries of the network stack. That's whhy claudio@ sent some diffs
>>> to no longer require the KERNEL_LOCK() to protect them.
>>>
>>> But right now some rtm_* functions can be executed without
>>> KERNEL_LOCK(). That's wrong. Diff below fixes that and move the
>>> KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
>>> enough to protect the other data structures.
>>>
>>> The scariest bits come from default router advertisements, but I guess
>>> that slaacd(8) saved us ;)
>> Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
>> rt_{set,put}gwroute(). Theses can now be called without KERNEL_LOCK()
>> when inserting an ARP entry.
> Hrvoje Popovski found the hard way that rtrequest(RTM_RESOLVE...) still
> need the KERNEL_LOCK() for malloc(9)/free(9).
Hi,
with this patch i can't trigger panic as before.
Thank you.
Re: routing sockets vs KERNEL_LOCK()
On 05/06/17(Mon) 16:52, Martin Pieuchot wrote:
> On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
> > Routing sockets are not protected by the NET_LOCK(). That's one of the
> > boundaries of the network stack. That's whhy claudio@ sent some diffs
> > to no longer require the KERNEL_LOCK() to protect them.
> >
> > But right now some rtm_* functions can be executed without
> > KERNEL_LOCK(). That's wrong. Diff below fixes that and move the
> > KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
> > enough to protect the other data structures.
> >
> > The scariest bits come from default router advertisements, but I guess
> > that slaacd(8) saved us ;)
>
> Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
> rt_{set,put}gwroute(). Theses can now be called without KERNEL_LOCK()
> when inserting an ARP entry.
Hrvoje Popovski found the hard way that rtrequest(RTM_RESOLVE...) still
need the KERNEL_LOCK() for malloc(9)/free(9).
Here's a more conservative diff. ok?
Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.357
diff -u -p -r1.357 route.c
--- net/route.c 27 May 2017 09:51:18 - 1.357
+++ net/route.c 6 Jun 2017 11:01:05 -
@@ -385,7 +385,7 @@ rt_setgwroute(struct rtentry *rt, u_int
{
struct rtentry *nhrt;
- KERNEL_ASSERT_LOCKED();
+ NET_ASSERT_LOCKED();
KASSERT(ISSET(rt->rt_flags, RTF_GATEWAY));
@@ -442,7 +442,7 @@ rt_putgwroute(struct rtentry *rt)
{
struct rtentry *nhrt = rt->rt_gwroute;
- KERNEL_ASSERT_LOCKED();
+ NET_ASSERT_LOCKED();
if (!ISSET(rt->rt_flags, RTF_GATEWAY) || nhrt == NULL)
return;
@@ -624,7 +624,9 @@ out:
info.rti_info[RTAX_DST] = dst;
info.rti_info[RTAX_GATEWAY] = gateway;
info.rti_info[RTAX_AUTHOR] = src;
+ KERNEL_LOCK();
rtm_miss(RTM_REDIRECT, &info, flags, prio, ifidx, error, rdomain);
+ KERNEL_UNLOCK();
}
/*
@@ -653,8 +655,10 @@ rtdeletemsg(struct rtentry *rt, struct i
info.rti_flags = rt->rt_flags;
ifidx = rt->rt_ifidx;
error = rtrequest_delete(&info, rt->rt_priority, ifp, &rt, tableid);
+ KERNEL_LOCK();
rtm_miss(RTM_DELETE, &info, info.rti_flags, rt->rt_priority, ifidx,
error, tableid);
+ KERNEL_UNLOCK();
if (error == 0)
rtfree(rt);
return (error);
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.220
diff -u -p -r1.220 in_pcb.c
--- netinet/in_pcb.c7 Mar 2017 16:59:40 - 1.220
+++ netinet/in_pcb.c6 Jun 2017 10:56:17 -
@@ -716,8 +716,11 @@ in_losing(struct inpcb *inp)
info.rti_info[RTAX_DST] = &inp->inp_route.ro_dst;
info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, &sa_mask);
+
+ KERNEL_LOCK();
rtm_miss(RTM_LOSING, &info, rt->rt_flags, rt->rt_priority,
rt->rt_ifidx, 0, inp->inp_rtableid);
+ KERNEL_UNLOCK();
if (rt->rt_flags & RTF_DYNAMIC)
(void)rtrequest(RTM_DELETE, &info, rt->rt_priority,
NULL, inp->inp_rtableid);
Index: netinet6/nd6_rtr.c
===
RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
retrieving revision 1.159
diff -u -p -r1.159 nd6_rtr.c
--- netinet6/nd6_rtr.c 30 May 2017 08:58:34 - 1.159
+++ netinet6/nd6_rtr.c 6 Jun 2017 10:56:17 -
@@ -613,7 +613,9 @@ defrouter_addreq(struct nd_defrouter *ne
error = rtrequest(RTM_ADD, &info, RTP_DEFAULT, &rt,
new->ifp->if_rdomain);
if (error == 0) {
+ KERNEL_LOCK();
rtm_send(rt, RTM_ADD, new->ifp->if_rdomain);
+ KERNEL_UNLOCK();
rtfree(rt);
new->installed = 1;
}
@@ -717,7 +719,9 @@ defrouter_delreq(struct nd_defrouter *dr
error = rtrequest(RTM_DELETE, &info, RTP_DEFAULT, &rt,
dr->ifp->if_rdomain);
if (error == 0) {
+ KERNEL_LOCK();
rtm_send(rt, RTM_DELETE, dr->ifp->if_rdomain);
+ KERNEL_UNLOCK();
rtfree(rt);
}
Re: routing sockets vs KERNEL_LOCK()
On 05/06/17(Mon) 12:12, Martin Pieuchot wrote:
> Routing sockets are not protected by the NET_LOCK(). That's one of the
> boundaries of the network stack. That's whhy claudio@ sent some diffs
> to no longer require the KERNEL_LOCK() to protect them.
>
> But right now some rtm_* functions can be executed without
> KERNEL_LOCK(). That's wrong. Diff below fixes that and move the
> KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
> enough to protect the other data structures.
>
> The scariest bits come from default router advertisements, but I guess
> that slaacd(8) saved us ;)
Also change a KERNEL_ASSERT_LOCKED() into a NET_ASSERT_LOCK() in
rt_{set,put}gwroute(). Theses can now be called without KERNEL_LOCK()
when inserting an ARP entry.
Updated diff below.
Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.357
diff -u -p -r1.357 route.c
--- net/route.c 27 May 2017 09:51:18 - 1.357
+++ net/route.c 5 Jun 2017 14:48:49 -
@@ -253,7 +253,6 @@ rt_match(struct sockaddr *dst, uint32_t
memset(&info, 0, sizeof(info));
info.rti_info[RTAX_DST] = dst;
- KERNEL_LOCK();
/*
* The priority of cloned route should be different
* to avoid conflict with /32 cloning routes.
@@ -264,14 +263,17 @@ rt_match(struct sockaddr *dst, uint32_t
error = rtrequest(RTM_RESOLVE, &info,
rt->rt_priority - 1, &rt, tableid);
if (error) {
+ KERNEL_LOCK();
rtm_miss(RTM_MISS, &info, 0, RTP_NONE, 0,
error, tableid);
+ KERNEL_UNLOCK();
} else {
/* Inform listeners of the new route */
+ KERNEL_LOCK();
rtm_send(rt, RTM_ADD, tableid);
+ KERNEL_UNLOCK();
rtfree(rt0);
}
- KERNEL_UNLOCK();
}
rt->rt_use++;
} else
@@ -385,7 +387,7 @@ rt_setgwroute(struct rtentry *rt, u_int
{
struct rtentry *nhrt;
- KERNEL_ASSERT_LOCKED();
+ NET_ASSERT_LOCKED();
KASSERT(ISSET(rt->rt_flags, RTF_GATEWAY));
@@ -442,7 +444,7 @@ rt_putgwroute(struct rtentry *rt)
{
struct rtentry *nhrt = rt->rt_gwroute;
- KERNEL_ASSERT_LOCKED();
+ NET_ASSERT_LOCKED();
if (!ISSET(rt->rt_flags, RTF_GATEWAY) || nhrt == NULL)
return;
@@ -624,7 +626,9 @@ out:
info.rti_info[RTAX_DST] = dst;
info.rti_info[RTAX_GATEWAY] = gateway;
info.rti_info[RTAX_AUTHOR] = src;
+ KERNEL_LOCK();
rtm_miss(RTM_REDIRECT, &info, flags, prio, ifidx, error, rdomain);
+ KERNEL_UNLOCK();
}
/*
@@ -653,8 +657,10 @@ rtdeletemsg(struct rtentry *rt, struct i
info.rti_flags = rt->rt_flags;
ifidx = rt->rt_ifidx;
error = rtrequest_delete(&info, rt->rt_priority, ifp, &rt, tableid);
+ KERNEL_LOCK();
rtm_miss(RTM_DELETE, &info, info.rti_flags, rt->rt_priority, ifidx,
error, tableid);
+ KERNEL_UNLOCK();
if (error == 0)
rtfree(rt);
return (error);
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.220
diff -u -p -r1.220 in_pcb.c
--- netinet/in_pcb.c7 Mar 2017 16:59:40 - 1.220
+++ netinet/in_pcb.c5 Jun 2017 10:02:35 -
@@ -716,8 +716,11 @@ in_losing(struct inpcb *inp)
info.rti_info[RTAX_DST] = &inp->inp_route.ro_dst;
info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, &sa_mask);
+
+ KERNEL_LOCK();
rtm_miss(RTM_LOSING, &info, rt->rt_flags, rt->rt_priority,
rt->rt_ifidx, 0, inp->inp_rtableid);
+ KERNEL_UNLOCK();
if (rt->rt_flags & RTF_DYNAMIC)
(void)rtrequest(RTM_DELETE, &info, rt->rt_priority,
NULL, inp->inp_rtableid);
Index: netinet6/nd6_rtr.c
===
RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
retrieving revision 1.159
diff -u -p -r1.159 nd6_rtr.c
--- netinet6/nd6_rtr.c 30 May 2017 08:58:34 - 1.159
+++ netinet6/nd6_rtr.c 5 Jun 2017 10:03:16 -
@@ -613,7 +613,9 @@ defrouter_addreq(struct nd_defrouter *ne
error = rtrequest(RTM_ADD, &info, RTP_DEFAULT, &rt,
new->ifp->if_rdomain);
if (error == 0) {
+ KERNEL_LOCK();
rtm_se
routing sockets vs KERNEL_LOCK()
Routing sockets are not protected by the NET_LOCK(). That's one of the
boundaries of the network stack. That's whhy claudio@ sent some diffs
to no longer require the KERNEL_LOCK() to protect them.
But right now some rtm_* functions can be executed without
KERNEL_LOCK(). That's wrong. Diff below fixes that and move the
KERNEL_LOCK() further down in rtrequest(9) since the NET_LOCK() is
enough to protect the other data structures.
The scariest bits come from default router advertisements, but I guess
that slaacd(8) saved us ;)
ok?
Index: net/route.c
===
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.357
diff -u -p -r1.357 route.c
--- net/route.c 27 May 2017 09:51:18 - 1.357
+++ net/route.c 5 Jun 2017 10:02:11 -
@@ -253,7 +253,6 @@ rt_match(struct sockaddr *dst, uint32_t
memset(&info, 0, sizeof(info));
info.rti_info[RTAX_DST] = dst;
- KERNEL_LOCK();
/*
* The priority of cloned route should be different
* to avoid conflict with /32 cloning routes.
@@ -264,14 +263,17 @@ rt_match(struct sockaddr *dst, uint32_t
error = rtrequest(RTM_RESOLVE, &info,
rt->rt_priority - 1, &rt, tableid);
if (error) {
+ KERNEL_LOCK();
rtm_miss(RTM_MISS, &info, 0, RTP_NONE, 0,
error, tableid);
+ KERNEL_UNLOCK();
} else {
/* Inform listeners of the new route */
+ KERNEL_LOCK();
rtm_send(rt, RTM_ADD, tableid);
+ KERNEL_UNLOCK();
rtfree(rt0);
}
- KERNEL_UNLOCK();
}
rt->rt_use++;
} else
@@ -624,7 +626,9 @@ out:
info.rti_info[RTAX_DST] = dst;
info.rti_info[RTAX_GATEWAY] = gateway;
info.rti_info[RTAX_AUTHOR] = src;
+ KERNEL_LOCK();
rtm_miss(RTM_REDIRECT, &info, flags, prio, ifidx, error, rdomain);
+ KERNEL_UNLOCK();
}
/*
@@ -653,8 +657,10 @@ rtdeletemsg(struct rtentry *rt, struct i
info.rti_flags = rt->rt_flags;
ifidx = rt->rt_ifidx;
error = rtrequest_delete(&info, rt->rt_priority, ifp, &rt, tableid);
+ KERNEL_LOCK();
rtm_miss(RTM_DELETE, &info, info.rti_flags, rt->rt_priority, ifidx,
error, tableid);
+ KERNEL_UNLOCK();
if (error == 0)
rtfree(rt);
return (error);
Index: netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.220
diff -u -p -r1.220 in_pcb.c
--- netinet/in_pcb.c7 Mar 2017 16:59:40 - 1.220
+++ netinet/in_pcb.c5 Jun 2017 10:02:35 -
@@ -716,8 +716,11 @@ in_losing(struct inpcb *inp)
info.rti_info[RTAX_DST] = &inp->inp_route.ro_dst;
info.rti_info[RTAX_GATEWAY] = rt->rt_gateway;
info.rti_info[RTAX_NETMASK] = rt_plen2mask(rt, &sa_mask);
+
+ KERNEL_LOCK();
rtm_miss(RTM_LOSING, &info, rt->rt_flags, rt->rt_priority,
rt->rt_ifidx, 0, inp->inp_rtableid);
+ KERNEL_UNLOCK();
if (rt->rt_flags & RTF_DYNAMIC)
(void)rtrequest(RTM_DELETE, &info, rt->rt_priority,
NULL, inp->inp_rtableid);
Index: netinet6/nd6_rtr.c
===
RCS file: /cvs/src/sys/netinet6/nd6_rtr.c,v
retrieving revision 1.159
diff -u -p -r1.159 nd6_rtr.c
--- netinet6/nd6_rtr.c 30 May 2017 08:58:34 - 1.159
+++ netinet6/nd6_rtr.c 5 Jun 2017 10:03:16 -
@@ -613,7 +613,9 @@ defrouter_addreq(struct nd_defrouter *ne
error = rtrequest(RTM_ADD, &info, RTP_DEFAULT, &rt,
new->ifp->if_rdomain);
if (error == 0) {
+ KERNEL_LOCK();
rtm_send(rt, RTM_ADD, new->ifp->if_rdomain);
+ KERNEL_UNLOCK();
rtfree(rt);
new->installed = 1;
}
@@ -717,7 +719,9 @@ defrouter_delreq(struct nd_defrouter *dr
error = rtrequest(RTM_DELETE, &info, RTP_DEFAULT, &rt,
dr->ifp->if_rdomain);
if (error == 0) {
+ KERNEL_LOCK();
rtm_send(rt, RTM_DELETE, dr->ifp->if_rdomain);
+ KERNEL_UNLOCK();
rtfree(rt);
}
