Page MenuHomeFreeBSD

Fix mutual exclusion issues in multicast socket option handling.
ClosedPublic

Authored by markj on Apr 26 2019, 7:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 21 2024, 7:19 PM
Unknown Object (File)
Jan 15 2024, 7:33 AM
Unknown Object (File)
Dec 20 2023, 3:09 AM
Unknown Object (File)
Nov 30 2023, 1:37 AM
Unknown Object (File)
Nov 10 2023, 1:12 AM
Unknown Object (File)
Oct 19 2023, 3:20 AM
Unknown Object (File)
Sep 16 2023, 3:52 PM
Unknown Object (File)
Aug 21 2023, 12:28 PM

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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 marked an inline comment as done.
  • Remove unintended changes.
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.

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. :)

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.

Is anyone interested in reviewing this further?

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.