On Tue, Jun 23, 2020 at 8:21 AM Martin Pieuchot <m...@openbsd.org> wrote: > I'd argue this is a related problem but a different one. The diff I > sent serializes cloning/destroying pseudo-interfaces. It has value on > its own because *all* if_clone_*() operations are now serialized. > > Now you correctly points out that it doesn't fix all the races in the > ioctl path. That's a fact. However without the informations of the > crashes it is hard to reason about the uncounted reference returned by > ifunit(). > > Taking a rwlock around all ioctl(2) can have effects that are hard to > debug.
We're talking about the same resource and lookup structure, so generally it makes sense to protect that the same way, right? Adding and removing ifps, and adding and them and removing them from the list of ifps, all need to be synchronized with the read-only usage of those ifps. The other way to fix it would be avoiding a critical section entirely by incrementing a refcount during the if_list lookup. Either way, it sounds like the big problem you're pointing out with my patch is that you fear some of those ioctls need to be callable from contexts that cannot sleep, which means using an rwlock is wrong. It doesn't look like the mutex spinlock has a r/w variant though. Or am I missing that? Jason