On 14/05/18(Mon) 10:17, Theo Buehler wrote: > This was pointed out to me by mpi a while ago. > > The if_clone_list() function accesses &if_cloners, which is only > modified via if_clone_attach() and if_clone_detach(). > > The latter function is never called, as far as I can tell, while > if_clone_attach() is only called at boot time via the pdevinit[] > mechanism in ioconf.c. > > So, for the purposes of the NET_LOCK(), &if_cloners is immutable, hence > if_clone_list() does not need the NET_RLOCK().
Please add a KERNEL_ASSERT_LOCKED() in if_clone_list(), with that ok mpi@. It's important to document which locks are required in every function. Because at some point we'll remove remove the KERNEL_LOCK() from the ioctl path. When this will happen, we'll need to protect `if_cloners' and `if_cloners_count'. Another (better) alternative would be to introduce a rwlock to protect those two globals and use it where required. > Index: sys/net/if.c > =================================================================== > RCS file: /var/cvs/src/sys/net/if.c,v > retrieving revision 1.551 > diff -u -p -r1.551 if.c > --- sys/net/if.c 28 Apr 2018 15:44:59 -0000 1.551 > +++ sys/net/if.c 14 May 2018 07:37:01 -0000 > @@ -2164,9 +2164,7 @@ ifioctl_get(u_long cmd, caddr_t data) > NET_RUNLOCK(); > return (error); > case SIOCIFGCLONERS: > - NET_RLOCK(); > error = if_clone_list((struct if_clonereq *)data); > - NET_RUNLOCK(); > return (error); > case SIOCGIFGMEMB: > NET_RLOCK();
