On Mon, Apr 30, 2018 at 02:55:21PM +0200, Martin Pieuchot wrote:
> On 30/04/18(Mon) 12:00, Theo Buehler wrote:
> > With mpi's encouragement and guidance, here's a diff that reduces the
> > scope of the NET_LOCK() a bit.
> >
> > in_control() is the only caller of mrt_ioctl() and the latter is a
> > simple function only requiring a read lock.
> >
> > There are only a handful callers of in_ioctl(). The two switches create
> > relatively tangled codepaths, so this will need some refactoring before
> > we can push the lock further down and split the read and write cases.
> > For now, establish a single exit point, grab the netlock at the
> > beginning and release it at the end.
>
> That makes sense, the same could be done for in6_ioctl().
Here's the corresponding diff for in6_control().
Push the diff down into mrt6_ioctl() and in6_ioctl(). Much like the IPv4
version, mrt6_ioctl() only needs a read lock.
in6_ioctl() is a bit messier. It starts off with a call to nd6_ioctl(),
which only needs a read lock. In the diff, I pushed the NET_RLOCK() down
into nd6_ioctl(), but perhaps it would be better to keep the locking
inside in6_ioctl(), like this:
switch (cmd) {
case SIOCGIFINFO_IN6:
case SIOCGNBRINFO_IN6:
NET_RLOCK();
error = nd6_ioctl(cmd, data, ifp);
NET_RUNLOCK();
return error;
}
I tested this as well as I could, but I don't usually use IPv6, so I
dabbled a bit with it on my home network and I tried to use the netinet6
regress tests, only with moderate success.
A test from people who actually use IPv6 would be welcome.
Index: sys/netinet6/in6.c
===================================================================
RCS file: /var/cvs/src/sys/netinet6/in6.c,v
retrieving revision 1.222
diff -u -p -r1.222 in6.c
--- sys/netinet6/in6.c 24 Apr 2018 19:53:38 -0000 1.222
+++ sys/netinet6/in6.c 1 May 2018 09:14:07 -0000
@@ -183,7 +183,6 @@ in6_control(struct socket *so, u_long cm
int privileged;
int error;
- NET_LOCK();
privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
privileged++;
@@ -200,7 +199,6 @@ in6_control(struct socket *so, u_long cm
break;
}
- NET_UNLOCK();
return error;
}
@@ -211,12 +209,11 @@ in6_ioctl(u_long cmd, caddr_t data, stru
struct in6_ifaddr *ia6 = NULL;
struct in6_aliasreq *ifra = (struct in6_aliasreq *)data;
struct sockaddr_in6 *sa6;
+ int error = 0;
if (ifp == NULL)
return (ENXIO);
- NET_ASSERT_LOCKED();
-
switch (cmd) {
case SIOCGIFINFO_IN6:
case SIOCGNBRINFO_IN6:
@@ -252,13 +249,16 @@ in6_ioctl(u_long cmd, caddr_t data, stru
case SIOCSIFNETMASK:
/*
* Do not pass those ioctl to driver handler since they are not
- * properly setup. Instead just error out.
+ * properly set up. Instead just error out.
*/
return (EINVAL);
default:
sa6 = NULL;
break;
}
+
+ NET_LOCK();
+
if (sa6 && sa6->sin6_family == AF_INET6) {
if (IN6_IS_ADDR_LINKLOCAL(&sa6->sin6_addr)) {
if (sa6->sin6_addr.s6_addr16[1] == 0) {
@@ -267,12 +267,15 @@ in6_ioctl(u_long cmd, caddr_t data, stru
htons(ifp->if_index);
} else if (sa6->sin6_addr.s6_addr16[1] !=
htons(ifp->if_index)) {
- return (EINVAL); /* link ID contradicts
*/
+ error = EINVAL; /* link ID contradicts */
+ goto err;
}
if (sa6->sin6_scope_id) {
if (sa6->sin6_scope_id !=
- (u_int32_t)ifp->if_index)
- return (EINVAL);
+ (u_int32_t)ifp->if_index) {
+ error = EINVAL;
+ goto err;
+ }
sa6->sin6_scope_id = 0; /* XXX: good way? */
}
}
@@ -289,8 +292,10 @@ in6_ioctl(u_long cmd, caddr_t data, stru
* interface address from the day one, we consider "remove the
* first one" semantics to be not preferable.
*/
- if (ia6 == NULL)
- return (EADDRNOTAVAIL);
+ if (ia6 == NULL) {
+ error = EADDRNOTAVAIL;
+ goto err;
+ }
/* FALLTHROUGH */
case SIOCAIFADDR_IN6:
/*
@@ -298,11 +303,14 @@ in6_ioctl(u_long cmd, caddr_t data, stru
* the corresponding operation.
*/
if (ifra->ifra_addr.sin6_family != AF_INET6 ||
- ifra->ifra_addr.sin6_len != sizeof(struct sockaddr_in6))
- return (EAFNOSUPPORT);
- if (!privileged)
- return (EPERM);
-
+ ifra->ifra_addr.sin6_len != sizeof(struct sockaddr_in6)) {
+ error = EAFNOSUPPORT;
+ goto err;
+ }
+ if (!privileged) {
+ error = EPERM;
+ goto err;
+ }
break;
case SIOCGIFAFLAG_IN6:
@@ -310,15 +318,19 @@ in6_ioctl(u_long cmd, caddr_t data, stru
case SIOCGIFDSTADDR_IN6:
case SIOCGIFALIFETIME_IN6:
/* must think again about its semantics */
- if (ia6 == NULL)
- return (EADDRNOTAVAIL);
+ if (ia6 == NULL) {
+ error = EADDRNOTAVAIL;
+ goto err;
+ }
break;
}
switch (cmd) {
case SIOCGIFDSTADDR_IN6:
- if ((ifp->if_flags & IFF_POINTOPOINT) == 0)
- return (EINVAL);
+ if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
+ error = EINVAL;
+ break;
+ }
/*
* XXX: should we check if ifa_dstaddr is NULL and return
* an error?
@@ -392,7 +404,8 @@ in6_ioctl(u_long cmd, caddr_t data, stru
if ((ifra->ifra_flags & IN6_IFF_DUPLICATED) != 0 ||
(ifra->ifra_flags & IN6_IFF_DETACHED) != 0 ||
(ifra->ifra_flags & IN6_IFF_DEPRECATED) != 0) {
- return (EINVAL);
+ error = EINVAL;
+ break;
}
if (ia6 == NULL)
@@ -413,10 +426,10 @@ in6_ioctl(u_long cmd, caddr_t data, stru
*/
error = in6_ifattach(ifp);
if (error != 0)
- return (error);
+ break;
error = in6_update_ifa(ifp, ifra, ia6);
if (error != 0)
- return (error);
+ break;
ia6 = in6ifa_ifpwithaddr(ifp, &ifra->ifra_addr.sin6_addr);
if (ia6 == NULL) {
@@ -446,7 +459,7 @@ in6_ioctl(u_long cmd, caddr_t data, stru
ia6->ia_ifa.ifa_addr);
if (error) {
in6_purgeaddr(&ia6->ia_ifa);
- return (error);
+ break;
}
dohooks(ifp->if_addrhooks, 0);
break;
@@ -458,10 +471,13 @@ in6_ioctl(u_long cmd, caddr_t data, stru
break;
default:
- return (EOPNOTSUPP);
+ error = EOPNOTSUPP;
+ break;
}
- return (0);
+err:
+ NET_UNLOCK();
+ return (error);
}
/*
Index: sys/netinet6/ip6_mroute.c
===================================================================
RCS file: /var/cvs/src/sys/netinet6/ip6_mroute.c,v
retrieving revision 1.114
diff -u -p -r1.114 ip6_mroute.c
--- sys/netinet6/ip6_mroute.c 26 Jun 2017 09:32:32 -0000 1.114
+++ sys/netinet6/ip6_mroute.c 1 May 2018 08:42:18 -0000
@@ -242,17 +242,26 @@ int
mrt6_ioctl(struct socket *so, u_long cmd, caddr_t data)
{
struct inpcb *inp = sotoinpcb(so);
+ int error;
switch (cmd) {
case SIOCGETSGCNT_IN6:
- return (get_sg6_cnt((struct sioc_sg_req6 *)data,
- inp->inp_rtableid));
+ NET_RLOCK();
+ error = get_sg6_cnt((struct sioc_sg_req6 *)data,
+ inp->inp_rtableid);
+ NET_RUNLOCK();
+ break;
case SIOCGETMIFCNT_IN6:
- return (get_mif6_cnt((struct sioc_mif_req6 *)data,
- inp->inp_rtableid));
+ NET_RLOCK();
+ error = get_mif6_cnt((struct sioc_mif_req6 *)data,
+ inp->inp_rtableid);
+ NET_RUNLOCK();
+ break;
default:
- return (ENOTTY);
+ error = ENOTTY;
+ break;
}
+ return error;
}
/*
Index: sys/netinet6/nd6.c
===================================================================
RCS file: /var/cvs/src/sys/netinet6/nd6.c,v
retrieving revision 1.223
diff -u -p -r1.223 nd6.c
--- sys/netinet6/nd6.c 15 Jan 2018 13:48:31 -0000 1.223
+++ sys/netinet6/nd6.c 30 Apr 2018 22:42:59 -0000
@@ -1014,20 +1014,20 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
struct in6_ndireq *ndi = (struct in6_ndireq *)data;
struct in6_nbrinfo *nbi = (struct in6_nbrinfo *)data;
struct rtentry *rt;
- int error = 0;
-
- NET_ASSERT_LOCKED();
switch (cmd) {
case SIOCGIFINFO_IN6:
+ NET_RLOCK();
ndi->ndi = *ND_IFINFO(ifp);
- break;
+ NET_RUNLOCK();
+ return (0);
case SIOCGNBRINFO_IN6:
{
struct llinfo_nd6 *ln;
struct in6_addr nb_addr = nbi->addr; /* make local for safety */
time_t expire;
+ NET_RLOCK();
/*
* XXX: KAME specific hack for scoped addresses
* XXXX: for other scopes than link-local?
@@ -1043,9 +1043,9 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
rt = nd6_lookup(&nb_addr, 0, ifp, ifp->if_rdomain);
if (rt == NULL ||
(ln = (struct llinfo_nd6 *)rt->rt_llinfo) == NULL) {
- error = EINVAL;
rtfree(rt);
- break;
+ NET_RUNLOCK();
+ return (EINVAL);
}
expire = ln->ln_rt->rt_expire;
if (expire != 0) {
@@ -1057,12 +1057,13 @@ nd6_ioctl(u_long cmd, caddr_t data, stru
nbi->asked = ln->ln_asked;
nbi->isrouter = ln->ln_router;
nbi->expire = expire;
- rtfree(rt);
- break;
+ rtfree(rt);
+ NET_RUNLOCK();
+ return (0);
}
}
- return (error);
+ return (0);
}
/*
