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().
> To be honest, I'm a bit skeptical about the change this creates in
> if_umb.c where the NET_LOCK() used to protect two in_ioctl() calls
> in a row. After the diff we grab and release it twice. If this is
> innocent, I would love to have a better understanding of why.
Remember that the NET_LOCK() is here to prevent the 'softnet' taskq to
access a data structure, in this case `ifp->if_addrlist', while it is
being modified by another thread.
In the case below the only in_ioctl() call modifying a data structure
is the second one. So the first one should be taking a read version of
the NET_LOCK() which won't stop the 'softnet' task.
Plus the second ioctl is not always executed, it depends on the result
of the first one. However since we're currently taking the NET_LOCK()
before, we always stop the 'softnet' taskq. So moving the lock inside
in_ioctl() helps.
Please continue in that direction. Your diff is ok mpi@
> Index: sys/dev/usb/if_umb.c
> ===================================================================
> RCS file: /var/cvs/src/sys/dev/usb/if_umb.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 if_umb.c
> --- sys/dev/usb/if_umb.c 19 Feb 2018 08:59:52 -0000 1.18
> +++ sys/dev/usb/if_umb.c 28 Apr 2018 15:55:45 -0000
> @@ -965,7 +965,6 @@ umb_state_task(void *arg)
> */
> memset(sc->sc_info.ipv4dns, 0,
> sizeof (sc->sc_info.ipv4dns));
> - NET_LOCK();
> if (in_ioctl(SIOCGIFADDR, (caddr_t)&ifr, ifp, 1) == 0 &&
> satosin(&ifr.ifr_addr)->sin_addr.s_addr !=
> INADDR_ANY) {
> @@ -974,7 +973,6 @@ umb_state_task(void *arg)
> sizeof (ifra.ifra_addr));
> in_ioctl(SIOCDIFADDR, (caddr_t)&ifra, ifp, 1);
> }
> - NET_UNLOCK();
> }
> if_link_state_change(ifp);
> }
> @@ -1661,9 +1659,7 @@ umb_decode_ip_configuration(struct umb_s
> sin->sin_len = sizeof (ifra.ifra_mask);
> in_len2mask(&sin->sin_addr, ipv4elem.prefixlen);
>
> - NET_LOCK();
> rv = in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, 1);
> - NET_UNLOCK();
> if (rv == 0) {
> if (ifp->if_flags & IFF_DEBUG)
> log(LOG_INFO, "%s: IPv4 addr %s, mask %s, "
> Index: sys/netinet/in.c
> ===================================================================
> RCS file: /var/cvs/src/sys/netinet/in.c,v
> retrieving revision 1.149
> diff -u -p -r1.149 in.c
> --- sys/netinet/in.c 24 Apr 2018 19:53:38 -0000 1.149
> +++ sys/netinet/in.c 28 Apr 2018 16:44:23 -0000
> @@ -187,7 +187,6 @@ in_control(struct socket *so, u_long cmd
> int privileged;
> int error;
>
> - NET_LOCK();
> privileged = 0;
> if ((so->so_state & SS_PRIV) != 0)
> privileged++;
> @@ -204,7 +203,6 @@ in_control(struct socket *so, u_long cmd
> break;
> }
>
> - NET_UNLOCK();
> return error;
> }
>
> @@ -216,13 +214,13 @@ in_ioctl(u_long cmd, caddr_t data, struc
> struct in_ifaddr *ia = NULL;
> struct in_aliasreq *ifra = (struct in_aliasreq *)data;
> struct sockaddr_in oldaddr;
> - int error;
> + int error = 0;
> int newifaddr;
>
> if (ifp == NULL)
> return (ENXIO);
>
> - NET_ASSERT_LOCKED();
> + NET_LOCK();
>
> TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> if (ifa->ifa_addr->sa_family == AF_INET) {
> @@ -244,12 +242,16 @@ in_ioctl(u_long cmd, caddr_t data, struc
> }
> ia = ifatoia(ifa);
> }
> - if (cmd == SIOCDIFADDR && ia == NULL)
> - return (EADDRNOTAVAIL);
> + if (cmd == SIOCDIFADDR && ia == NULL) {
> + error = EADDRNOTAVAIL;
> + goto err;
> + }
> /* FALLTHROUGH */
> case SIOCSIFADDR:
> - if (!privileged)
> - return (EPERM);
> + if (!privileged) {
> + error = EPERM;
> + goto err;
> + }
>
> if (ia == NULL) {
> ia = malloc(sizeof *ia, M_IFADDR, M_WAITOK | M_ZERO);
> @@ -273,8 +275,10 @@ in_ioctl(u_long cmd, caddr_t data, struc
> case SIOCSIFNETMASK:
> case SIOCSIFDSTADDR:
> case SIOCSIFBRDADDR:
> - if (!privileged)
> - return (EPERM);
> + if (!privileged) {
> + error = EPERM;
> + goto err;
> + }
> /* FALLTHROUGH */
>
> case SIOCGIFADDR:
> @@ -291,8 +295,10 @@ in_ioctl(u_long cmd, caddr_t data, struc
> }
> }
> }
> - if (ia == NULL)
> - return (EADDRNOTAVAIL);
> + if (ia == NULL) {
> + error = EADDRNOTAVAIL;
> + goto err;
> + }
> break;
> }
> switch (cmd) {
> @@ -302,14 +308,18 @@ in_ioctl(u_long cmd, caddr_t data, struc
> break;
>
> case SIOCGIFBRDADDR:
> - if ((ifp->if_flags & IFF_BROADCAST) == 0)
> - return (EINVAL);
> + if ((ifp->if_flags & IFF_BROADCAST) == 0) {
> + error = EINVAL;
> + break;
> + }
> *satosin(&ifr->ifr_dstaddr) = ia->ia_broadaddr;
> break;
>
> case SIOCGIFDSTADDR:
> - if ((ifp->if_flags & IFF_POINTOPOINT) == 0)
> - return (EINVAL);
> + if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
> + error = EINVAL;
> + break;
> + }
> *satosin(&ifr->ifr_dstaddr) = ia->ia_dstaddr;
> break;
>
> @@ -318,31 +328,36 @@ in_ioctl(u_long cmd, caddr_t data, struc
> break;
>
> case SIOCSIFDSTADDR:
> - if ((ifp->if_flags & IFF_POINTOPOINT) == 0)
> - return (EINVAL);
> + if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
> + error = EINVAL;
> + break;
> + }
> oldaddr = ia->ia_dstaddr;
> ia->ia_dstaddr = *satosin(&ifr->ifr_dstaddr);
> error = (*ifp->if_ioctl)(ifp, SIOCSIFDSTADDR, (caddr_t)ia);
> if (error) {
> ia->ia_dstaddr = oldaddr;
> - return (error);
> + break;
> }
> in_scrubhost(ia, &oldaddr);
> in_addhost(ia, &ia->ia_dstaddr);
> break;
>
> case SIOCSIFBRDADDR:
> - if ((ifp->if_flags & IFF_BROADCAST) == 0)
> - return (EINVAL);
> + if ((ifp->if_flags & IFF_BROADCAST) == 0) {
> + error = EINVAL;
> + break;
> + }
> ifa_update_broadaddr(ifp, &ia->ia_ifa, &ifr->ifr_broadaddr);
> break;
>
> case SIOCSIFADDR:
> in_ifscrub(ifp, ia);
> error = in_ifinit(ifp, ia, satosin(&ifr->ifr_addr), newifaddr);
> - if (!error)
> - dohooks(ifp->if_addrhooks, 0);
> - return (error);
> + if (error)
> + break;
> + dohooks(ifp->if_addrhooks, 0);
> + break;
>
> case SIOCSIFNETMASK:
> ia->ia_netmask = ia->ia_sockmask.sin_addr.s_addr =
> @@ -352,8 +367,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
> case SIOCAIFADDR: {
> int needinit = 0;
>
> - error = 0;
> -
> if (ia->ia_addr.sin_family == AF_INET) {
> if (ifra->ifra_addr.sin_len == 0)
> ifra->ifra_addr = ia->ia_addr;
> @@ -384,9 +397,10 @@ in_ioctl(u_long cmd, caddr_t data, struc
> if (ifra->ifra_addr.sin_family == AF_INET && needinit) {
> error = in_ifinit(ifp, ia, &ifra->ifra_addr, newifaddr);
> }
> - if (!error)
> - dohooks(ifp->if_addrhooks, 0);
> - return (error);
> + if (error)
> + break;
> + dohooks(ifp->if_addrhooks, 0);
> + break;
> }
> case SIOCDIFADDR:
> /*
> @@ -400,9 +414,13 @@ in_ioctl(u_long cmd, caddr_t data, struc
> break;
>
> default:
> - return (EOPNOTSUPP);
> + error = EOPNOTSUPP;
> + break;
> }
> - return (0);
> +
> +err:
> + NET_UNLOCK();
> + return (error);
> }
> /*
> * Delete any existing route for an interface.
> Index: sys/netinet/ip_mroute.c
> ===================================================================
> RCS file: /var/cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.121
> diff -u -p -r1.121 ip_mroute.c
> --- sys/netinet/ip_mroute.c 1 Sep 2017 15:05:31 -0000 1.121
> +++ sys/netinet/ip_mroute.c 28 Apr 2018 15:45:32 -0000
> @@ -264,12 +264,16 @@ mrt_ioctl(struct socket *so, u_long cmd,
> else
> switch (cmd) {
> case SIOCGETVIFCNT:
> + NET_RLOCK();
> error = get_vif_cnt(inp->inp_rtableid,
> (struct sioc_vif_req *)data);
> + NET_RUNLOCK();
> break;
> case SIOCGETSGCNT:
> + NET_RLOCK();
> error = get_sg_cnt(inp->inp_rtableid,
> (struct sioc_sg_req *)data);
> + NET_RUNLOCK();
> break;
> default:
> error = ENOTTY;
