Fix mutual exclusion issues in multicast socket option handling.

[markj]

Summary

r333175 converted the global multicast lock to a sleepable sx lock, so
the lock order wrt the (non-sleepable) inp lock changed. To handle this,
r333175 and r333505 introduced a pattern of

in_pcbref(inp);
INP_WUNLOCK(inp);
IN_MULTI_LOCK();

However, this means that inp_join_group() and inp_leave_group() aren't
properly serialized against each other. Concurrent attempts to add a
souce address to a group and leave a group can leave things in an
inconsistent state and trigger panics. Also, the handling for the case
where we release the last inp ref was broken.

Fix the problem by simply acquiring the global multicast lock earlier.
I also fixed some lingering LORs that were not covered by r333505. I am
not convinced that the global lock is actually required anymore
(especially since it is used differently between IPv4 and IPv6), but I
need to study the code further. In the meantime, the change fixes
several races, present in 12.0, found by syzkaller.

[ae]

I'm not quite familiar with this code. Is it safe enough to make INP_WUNLOCK(); /* some code */ INP_WLOCK(); without holding extra reference to PCB? Is is it impossible, that another thread can destroy PCB when we release lock?

sys/netinet/in_mcast.c

2674

Is this change intentional?

[markj]

• Remove unintended changes.

[markj]

In D20070#431564, @ae wrote:

I'm not quite familiar with this code. Is it safe enough to make INP_WUNLOCK(); /* some code */ INP_WLOCK(); without holding extra reference to PCB? Is is it impossible, that another thread can destroy PCB when we release lock?

Sorry, I don't see where the question is coming from. In general, no, you have to acquire a reference before dropping the lock. That's what the old version of the diff did, in order to acquire the sleepable IN_MULTI lock. But dropping the PCB lock introduces races. I changed the code to acquire the IN_MULTI lock first, so we don't have to drop the PCB lock anymore.

sys/netinet/in_mcast.c

2674

No, I uploaded a stale diff by mistake.

[ae]

In D20070#432259, @markj wrote:

I'm not quite familiar with this code. Is it safe enough to make INP_WUNLOCK(); /* some code */ INP_WLOCK(); without holding extra reference to PCB? Is is it impossible, that another thread can destroy PCB when we release lock?

Sorry, I don't see where the question is coming from. In general, no, you have to acquire a reference before dropping the lock. That's what the old version of the diff did, in order to acquire the sleepable IN_MULTI lock. But dropping the PCB lock introduces races. I changed the code to acquire the IN_MULTI lock first, so we don't have to drop the PCB lock anymore.

The question is not related to your changes, but these files contains several places where such trick happen. :)

[markj]

In D20070#432274, @ae wrote:

In D20070#432259, @markj wrote:

I'm not quite familiar with this code. Is it safe enough to make INP_WUNLOCK(); /* some code */ INP_WLOCK(); without holding extra reference to PCB? Is is it impossible, that another thread can destroy PCB when we release lock?

Sorry, I don't see where the question is coming from. In general, no, you have to acquire a reference before dropping the lock. That's what the old version of the diff did, in order to acquire the sleepable IN_MULTI lock. But dropping the PCB lock introduces races. I changed the code to acquire the IN_MULTI lock first, so we don't have to drop the PCB lock anymore.

The question is not related to your changes, but these files contains several places where such trick happen. :)

Ah. My knowledge of the area is limited, but I believe the socket reference is sufficient to ensure that this is not a problem for e.g., inp_findmoptions(). In other words, so long as a thread is in inp_setmoptions(), a reference is held on the socket, and the socket references prevents the inp from being detached and freed. I did not do an exhaustive audit though.

[markj]

Is anyone interested in reviewing this further?

[ae]

I have no objection. This subsystem is currently broken, but nobody wants to fix it. So, if you tested this patch and it helps to solve your problem, I'm ok, since description looks reasonable.