Page MenuHomeFreeBSD

Fix mutual exclusion issues in multicast socket option handling.
ClosedPublic

Authored by markj on Apr 26 2019, 7:19 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Apr 26 2019, 7:19 AM
ae added a comment.Apr 26 2019, 10:20 AM

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 ↗(On Diff #56690)

Is this change intentional?

markj updated this revision to Diff 56803.Apr 29 2019, 1:03 PM
markj marked an inline comment as done.
  • Remove unintended changes.
markj added a comment.Apr 29 2019, 1:05 PM
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 ↗(On Diff #56690)

No, I uploaded a stale diff by mistake.

ae added a comment.Apr 29 2019, 1:28 PM

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 added a comment.Apr 29 2019, 1:45 PM
In D20070#432274, @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.

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.

mmacy added a subscriber: mmacy.Apr 29 2019, 7:05 PM
emaste added a subscriber: emaste.Apr 29 2019, 8:50 PM
markj added a comment.May 8 2019, 12:35 AM

Is anyone interested in reviewing this further?

ae accepted this revision as: ae.May 8 2019, 5:16 PM

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.

This revision is now accepted and ready to land.May 8 2019, 5:16 PM
This revision was automatically updated to reflect the committed changes.