On 23/06/20(Tue) 17:21, Jason A. Donenfeld wrote:
> 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.

Yes, that might be a better way.  If I understood your original mail the
issue is related to ifunit(), right?  ifunit() is not used in packet-
processing contexts, that's why we did not protect it by the NET_LOCK().

I'm not sure if all the ifunit() usages are necessary, many of them are
certainly exposing races with attach/destroy.

> 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?

Taking a rwlock for all ioctl(2) has a big impact.  The 'default' case
of the switch in ifioctl() goes into other subsystems and per-ifp ioctl
handler.

Many drivers sleep in their ioctl(2) handler, most USB and wireless one
to name a few.  Past experience showed that holding a rwlock around all
the handlers, led to (temporarily) "deadlock".  This is why vio(4) mailbox
sleep has been turned into a rwsleep releasing the `netlock'.

One can argue that such deadlocks happen because the scope of the lock is
huge.  This is easy to understand with the `netlock' and questionable,
at the time of the diff, with the proposed `if_list_lock'.  But saying
so would miss the point: throwing a lock around a huge part of code to
make symptoms disappear is a big hammer.

If the problem we're trying to fix is the reference counting of ifunit()
then I'd suggest to fix that and only that in all the tree.

If what we're after is serializing clone/destroy then let's do that.

The diff proposed did not dealt with all usages of any of the two
scenario.  I'd be glad to see the whole kernel has been considered and
the scope of any new lock is as small as possible.

Obviously I've been trying to reduce the scope of locks during years, so
I'm biased ;)

Reply via email to