On 21/02/19(Thu) 07:35, David Gwynne wrote: > > On 20 Feb 2019, at 11:21 pm, Martin Pieuchot <m...@openbsd.org> wrote: > > > > On 20/02/19(Wed) 14:44, David Gwynne wrote: > >> Index: sys/net/if.c > >> =================================================================== > >> RCS file: /cvs/src/sys/net/if.c,v > >> retrieving revision 1.571 > >> diff -u -p -r1.571 if.c > >> --- sys/net/if.c 9 Jan 2019 01:14:21 -0000 1.571 > >> +++ sys/net/if.c 20 Feb 2019 04:35:42 -0000 > >> @@ -2143,6 +2143,25 @@ ifioctl(struct socket *so, u_long cmd, c > >> NET_UNLOCK(); > >> break; > >> > >> + case SIOCSETMPWCFG: > >> + case SIOCSPWE3CTRLWORD: > >> + case SIOCSPWE3FAT: > >> + case SIOCSPWE3NEIGHBOR: > >> + case SIOCDPWE3NEIGHBOR: > >> + if ((error = suser(p)) != 0) > >> + break; > >> + /* FALLTHROUGH */ > >> + case SIOCGETMPWCFG: > >> + case SIOCGPWE3CTRLWORD: > >> + case SIOCGPWE3FAT: > >> + case SIOCGPWE3NEIGHBOR: > >> + if_ref(ifp); > >> + KERNEL_UNLOCK(); > >> + error = ((*ifp->if_ioctl)(ifp, cmd, data)); > >> + KERNEL_LOCK(); > >> + if_put(ifp); > > > > Why are you referencing the `ifp' and grabbing the KERNEL_LOCK() > > (recursively)? > > ifioctl gets the ifp pointer from ifunit, which doesn't increase the ref > count for you. I'm giving up kernel lock around the pwe3 ioctl calls into the > driver, not taking them harder. Taking the ifp ref there guarantees the > interface will stay alive^Wallocated over those calls.
It feels premature to me, well I'm confused. None of the other ioctl handlers do that. The KERNEL_LOCK() is still held in ifioctl() which guarantees serialization. If any of the ioctl(2) calls is going to sleep, thus releasing the lock, then I'd suggest to grab the NET_RLOCK() here instead. It still guarantees serialization of network ioctls w/ regard to detach. Note that I'll be delighted if you want to remove/push down the NET_LOCK() from this code path, but can we keep the handlers coherent? Even if we're using refcounting, don't we want to serialize all network ioctl(2)s? If we're not using the NET_LOCK() for this, can this new lock guarantee that that `ifp' isn't going away? Or do you have a better idea?
