On 19/12/17(Tue) 19:24, Alexander Bluhm wrote: > On Mon, Dec 18, 2017 at 04:01:12PM +0100, Martin Pieuchot wrote: > > Diff below moves the NET_LOCK() into every switch case that need it and > > document locking for most of the fields of 'struct ifnet'. > > > > case SIOCSIFXFLAGS: > > if ((error = suser(p, 0)) != 0) > > break; > > > > + NET_LOCK(); > > #ifdef INET6 > > if (ISSET(ifr->ifr_flags, IFXF_AUTOCONF6)) { > > error = in6_ifattach(ifp); > > Just a line below is an "if (error != 0) break;". There the unlock > is missing.
Indeed, fixed. > > > case SIOCSIFMTU: > > if ((error = suser(p, 0)) != 0) > > break; > > + NET_LOCK(); > > error = (*ifp->if_ioctl)(ifp, cmd, data); > > + NET_UNLOCK(); > > if (!error) > > rtm_ifchg(ifp); > > break; > > rtm_ifchg() calls route_input() which needs the net lock. Does it? What needs the lock in route_input()? I thought routing sockets are still protected by the KERNEL_LOCK(). > > @@ -2103,8 +2118,6 @@ ifioctl(struct socket *so, u_long cmd, c > > > > if (((oif_flags ^ ifp->if_flags) & IFF_UP) != 0) > > getmicrotime(&ifp->if_lastchange); > > - > > - NET_UNLOCK(); > > > > return (error); > > } > > May we access the ifp->if_flags without net lock? As we want to > compare them to their value before we changed them, would it be > even better to put the "oif_flags = ifp->if_flags;" under the lock? Flags are modified by the driver, not by the stack. Some of them are event immutable and might be better moved to a different field. > What is the advantage to move the net lock into the cases? The goal is to stop grabbing the lock for per-driver ioctl(2). This would fix the problem that sleeping in if_ioctl() might block network traffic for a long time. That means the limit between driver and stack fields have to be clear. You just pointed out that `if_flags' has some issues.