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.

Reply via email to