looks reasonable to me.
OK
On Tue, May 01, 2018 at 07:08:59PM +0200, Theo Buehler wrote:
> 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);
> }
>
> /*
>
--
I'm not entirely sure you are real.
