From: Matthew Dillon Date: Tue, 7 Jun 2011 20:50:40 +0000 (-0700) Subject: kernel - Serialize ifioctl() with a mutex X-Git-Tag: v2.12.0~496 X-Git-Url: https://gitweb.dragonflybsd.org/dragonfly.git/commitdiff_plain/9683f229e6862b5d21b610cac390a826ba6b51de kernel - Serialize ifioctl() with a mutex * ifioctl() calls (aka via ifconfig) make various assumptions about the stability of certain data structures and can panic otherwise. Concurrent calls to ifioctl via ifconfig can create inconsistencies in the ifnet structure. * Rearrange the ifioctl() code into a fall-through modem, add a struct mtx to the ifnet structure, and acquire and release the mutex in ifioctl() to enforce stability relative to concurrent ifconfig/ioctl commands issued on the interface. * Fixes a panic reproduced via a while(1): while (1) echo -n x ifconfig re0 inet6 fe80::201:2eff:fe31:5469%re0 & ifconfig re0 inet6 fe80::201:2eff:fe31:5469%re0 delete & sleep 0.1 end * May fix other related panics. NOTE: This mutex does not protect internal data structures related to network operation. It is currently being used strictly to serialize ifconfig style operations on the interface. Reported-by: Francois Tigeot --- diff --git a/sys/net/if.c b/sys/net/if.c index f29aedefb6..7ad51ab935 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -53,16 +53,19 @@ #include #include #include +#include #include #include #include #include #include -#include #include -#include #include +#include +#include +#include + #include #include #include @@ -525,6 +528,9 @@ if_attach(struct ifnet *ifp, lwkt_serialize_t serializer) ifp->if_start_nmsg[i].lmsg.u.ms_resultp = ifp; } + mtx_init(&ifp->if_ioctl_mtx); + mtx_lock(&ifp->if_ioctl_mtx); + TAILQ_INSERT_TAIL(&ifnet, ifp, if_link); ifp->if_index = ++if_index; @@ -612,6 +618,8 @@ if_attach(struct ifnet *ifp, lwkt_serialize_t serializer) /* Announce the interface. */ rt_ifannouncemsg(ifp, IFAN_ARRIVAL); + + mtx_unlock(&ifp->if_ioctl_mtx); } static void @@ -1449,17 +1457,22 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) int error; short oif_flags; int new_flags; +#ifdef COMPAT_43 + int ocmd; +#endif size_t namelen, onamelen; char new_name[IFNAMSIZ]; struct ifaddr *ifa; struct sockaddr_dl *sdl; switch (cmd) { - case SIOCGIFCONF: case OSIOCGIFCONF: return (ifconf(cmd, data, cred)); + default: + break; } + ifr = (struct ifreq *)data; switch (cmd) { @@ -1473,16 +1486,23 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) if ((error = priv_check_cred(cred, PRIV_ROOT, 0)) != 0) return (error); return (if_clone_destroy(ifr->ifr_name)); - case SIOCIFGCLONERS: return (if_clone_list((struct if_clonereq *)data)); + default: + break; } + /* + * Nominal ioctl through interface, lookup the ifp and obtain a + * lock to serialize the ifconfig ioctl operation. + */ ifp = ifunit(ifr->ifr_name); - if (ifp == 0) + if (ifp == NULL) return (ENXIO); - switch (cmd) { + error = 0; + mtx_lock(&ifp->if_ioctl_mtx); + switch (cmd) { case SIOCGIFINDEX: ifr->ifr_index = ifp->if_index; break; @@ -1507,10 +1527,9 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCGIFDATA: error = copyout((caddr_t)&ifp->if_data, ifr->ifr_data, - sizeof(ifp->if_data)); + sizeof(ifp->if_data)); break; - case SIOCGIFPHYS: ifr->ifr_phys = ifp->if_physical; break; @@ -1533,7 +1552,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCSIFFLAGS: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); + break; new_flags = (ifr->ifr_flags & 0xffff) | (ifr->ifr_flagshigh << 16); if (ifp->if_flags & IFF_SMART) { @@ -1587,9 +1606,11 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCSIFCAP: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); - if (ifr->ifr_reqcap & ~ifp->if_capabilities) - return (EINVAL); + break; + if (ifr->ifr_reqcap & ~ifp->if_capabilities) { + error = EINVAL; + break; + } ifnet_serialize_all(ifp); ifp->if_ioctl(ifp, cmd, data, cred); ifnet_deserialize_all(ifp); @@ -1597,15 +1618,19 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCSIFNAME: error = priv_check_cred(cred, PRIV_ROOT, 0); - if (error != 0) - return (error); + if (error) + break; error = copyinstr(ifr->ifr_data, new_name, IFNAMSIZ, NULL); - if (error != 0) - return (error); - if (new_name[0] == '\0') - return (EINVAL); - if (ifunit(new_name) != NULL) - return (EEXIST); + if (error) + break; + if (new_name[0] == '\0') { + error = EINVAL; + break; + } + if (ifunit(new_name) != NULL) { + error = EEXIST; + break; + } EVENTHANDLER_INVOKE(ifnet_detach_event, ifp); @@ -1644,7 +1669,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCSIFMETRIC: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); + break; ifp->if_metric = ifr->ifr_metric; getmicrotime(&ifp->if_lastchange); break; @@ -1652,15 +1677,17 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCSIFPHYS: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return error; - if (!ifp->if_ioctl) - return EOPNOTSUPP; + break; + if (ifp->if_ioctl == NULL) { + error = EOPNOTSUPP; + break; + } ifnet_serialize_all(ifp); error = ifp->if_ioctl(ifp, cmd, data, cred); ifnet_deserialize_all(ifp); if (error == 0) getmicrotime(&ifp->if_lastchange); - return (error); + break; case SIOCSIFMTU: { @@ -1668,11 +1695,15 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); - if (ifp->if_ioctl == NULL) - return (EOPNOTSUPP); - if (ifr->ifr_mtu < IF_MINMTU || ifr->ifr_mtu > IF_MAXMTU) - return (EINVAL); + break; + if (ifp->if_ioctl == NULL) { + error = EOPNOTSUPP; + break; + } + if (ifr->ifr_mtu < IF_MINMTU || ifr->ifr_mtu > IF_MAXMTU) { + error = EINVAL; + break; + } ifnet_serialize_all(ifp); error = ifp->if_ioctl(ifp, cmd, data, cred); ifnet_deserialize_all(ifp); @@ -1688,22 +1719,26 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) nd6_setmtu(ifp); #endif } - return (error); + break; } case SIOCADDMULTI: case SIOCDELMULTI: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); + break; /* Don't allow group membership on non-multicast interfaces. */ - if ((ifp->if_flags & IFF_MULTICAST) == 0) - return EOPNOTSUPP; + if ((ifp->if_flags & IFF_MULTICAST) == 0) { + error = EOPNOTSUPP; + break; + } /* Don't let users screw up protocols' entries. */ - if (ifr->ifr_addr.sa_family != AF_LINK) - return EINVAL; + if (ifr->ifr_addr.sa_family != AF_LINK) { + error = EINVAL; + break; + } if (cmd == SIOCADDMULTI) { struct ifmultiaddr *ifma; @@ -1713,7 +1748,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) } if (error == 0) getmicrotime(&ifp->if_lastchange); - return error; + break; case SIOCSIFPHYADDR: case SIOCDIFPHYADDR: @@ -1725,53 +1760,57 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) case SIOCSIFGENERIC: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); - if (ifp->if_ioctl == 0) - return (EOPNOTSUPP); + break; + if (ifp->if_ioctl == 0) { + error = EOPNOTSUPP; + break; + } ifnet_serialize_all(ifp); error = ifp->if_ioctl(ifp, cmd, data, cred); ifnet_deserialize_all(ifp); if (error == 0) getmicrotime(&ifp->if_lastchange); - return error; + break; case SIOCGIFSTATUS: ifs = (struct ifstat *)data; ifs->ascii[0] = '\0'; - + /* fall through */ case SIOCGIFPSRCADDR: case SIOCGIFPDSTADDR: case SIOCGLIFPHYADDR: case SIOCGIFMEDIA: case SIOCGIFGENERIC: - if (ifp->if_ioctl == NULL) - return (EOPNOTSUPP); + if (ifp->if_ioctl == NULL) { + error = EOPNOTSUPP; + break; + } ifnet_serialize_all(ifp); error = ifp->if_ioctl(ifp, cmd, data, cred); ifnet_deserialize_all(ifp); - return (error); + break; case SIOCSIFLLADDR: error = priv_check_cred(cred, PRIV_ROOT, 0); if (error) - return (error); - error = if_setlladdr(ifp, - ifr->ifr_addr.sa_data, ifr->ifr_addr.sa_len); + break; + error = if_setlladdr(ifp, ifr->ifr_addr.sa_data, + ifr->ifr_addr.sa_len); EVENTHANDLER_INVOKE(iflladdr_event, ifp); - return (error); + break; default: oif_flags = ifp->if_flags; - if (so->so_proto == 0) - return (EOPNOTSUPP); + if (so->so_proto == 0) { + error = EOPNOTSUPP; + break; + } #ifndef COMPAT_43 error = so_pru_control_direct(so, cmd, data, ifp); #else - { - int ocmd = cmd; + ocmd = cmd; switch (cmd) { - case SIOCSIFDSTADDR: case SIOCSIFADDR: case SIOCSIFBRDADDR: @@ -1787,21 +1826,20 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) ifr->ifr_addr.sa_len = 16; #endif break; - case OSIOCGIFADDR: cmd = SIOCGIFADDR; break; - case OSIOCGIFDSTADDR: cmd = SIOCGIFDSTADDR; break; - case OSIOCGIFBRDADDR: cmd = SIOCGIFBRDADDR; break; - case OSIOCGIFNETMASK: cmd = SIOCGIFNETMASK; + break; + default: + break; } error = so_pru_control_direct(so, cmd, data, ifp); @@ -1814,7 +1852,6 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) *(u_short *)&ifr->ifr_addr = ifr->ifr_addr.sa_family; break; } - } #endif /* COMPAT_43 */ if ((oif_flags ^ ifp->if_flags) & IFF_UP) { @@ -1827,10 +1864,11 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct ucred *cred) } #endif } - return (error); - + break; } - return (0); + + mtx_unlock(&ifp->if_ioctl_mtx); + return (error); } /* diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 6cd1c82116..5f63cd8736 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -44,6 +44,9 @@ #ifndef _NET_IF_H_ #include #endif +#ifndef _SYS_MUTEX_H_ +#include +#endif /* * Structures defining a network interface, providing a packet @@ -234,7 +237,6 @@ struct ifnet { int (*if_start_cpuid) /* cpuid to run if_start */ (struct ifnet *); TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */ - /* protected by if_addr_mtx */ #ifdef DEVICE_POLLING void (*if_poll) /* IFF_POLLING support */ (struct ifnet *, enum poll_cmd, int); @@ -272,6 +274,7 @@ struct ifnet { struct ifaddr *if_lladdr; struct lwkt_serialize *if_serializer; /* serializer or MP lock */ struct lwkt_serialize if_default_serializer; /* if not supplied */ + struct mtx if_ioctl_mtx; /* high-level ioctl serializing mutex */ int if_cpuid; struct netmsg_base *if_start_nmsg; /* percpu msgs to sched if_start */ void *if_pf_kif; /* pf interface abstraction */