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.
