Using an array to keep track of multicast memberships introduce multiple races if the APIs are exercised in parallell. Moving state structures around in memory when new members are removed makes debugging more difficult. By using a STAILQ() the memory location of the IPv4 and IPv6 multicast filters become fixed and use after free issues will be more easy to track.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
For those that review: The IPv4 changes are almost identical to the IPv6 changes! Only different structures with different name.
| sys/netinet/in_mcast.c | ||
|---|---|---|
| 343 ↗ | (On Diff #56996) | Extre newline. |
| sys/netinet/in_var.h | ||
| 270 ↗ | (On Diff #56762) | Indeed, I can't see why one of the _FOREACH macros cannot be used. |
| sys/netinet6/in6_mcast.c | ||
| 2299 ↗ | (On Diff #56996) | Is this supposed to be IN6_MULTI_LOCK()? Note that we lock it further below. |
| sys/netpfil/pf/if_pfsync.c | ||
| 2382 ↗ | (On Diff #56996) | We should probably assert that imo_head is empty following the removal? |
| 2404 ↗ | (On Diff #56996) | We should wrap this in a helper routine. |
| sys/netinet6/in6_mcast.c | ||
|---|---|---|
| 2299 ↗ | (On Diff #56996) | Yes, you're right. I'll update the patch tomorrow. |
| sys/netinet/in_mcast.c | ||
|---|---|---|
| 2426 ↗ | (On Diff #57887) | Might as well switch it to bool and write !is_final. |
| sys/netinet/in_var.h | ||
| 242 ↗ | (On Diff #57887) | Why did you make this a typedef? |
| 248 ↗ | (On Diff #57887) | I can't see why all of this stuff is here and not in ip_var.h. There are some consumers that do not include ip_var.h, but they can be fixed. At least, there should be a comment here or in in_var.h explaining the relationship. |
| sys/netpfil/pf/if_pfsync.c | ||
| 1397 ↗ | (On Diff #57887) | Missing space after the last comma. |
| sys/netinet/in_mcast.c | ||
|---|---|---|
| 1596 ↗ | (On Diff #57887) | There is a LOR here. We hold the non-sleepable inp lock at this point. |
| sys/netinet/in_mcast.c | ||
|---|---|---|
| 98 ↗ | (On Diff #57887) | I haven't tried to reproduce it yet. In the code I referenced, we acquire the inp wlock, followed by the in_multi lock. |
| sys/netinet/in_var.h | ||
|---|---|---|
| 242 ↗ | (On Diff #57887) | To make future changes easier, like changing the queue type. |
| sys/netinet/in_var.h | ||
|---|---|---|
| 248 ↗ | (On Diff #57887) | Can you explain a bit more? I believe I've already tried this and that is the reason for the #ifdef _NETINET_IN_VAR_H_ I've added. The structures must be defined in the correct order, because not always we use pointers. Else the order wouldn't matter. |
| sys/netinet/in_var.h | ||
|---|---|---|
| 242 ↗ | (On Diff #57887) | Just write STAILQ_HEAD(in_mfilter_head, in_mfilter). Then you get a struct in_mfilter_head and you can change the queue type easily. |
| sys/netinet/in_var.h | ||
|---|---|---|
| 244 ↗ | (On Diff #58312) | Style: extra space after '*'. |
| 245 ↗ | (On Diff #58312) | The naming is kind of weird IMO. Usually we would call these ip_mfilter_alloc(), ip_mfilter_free(), IP_MFILTER_FOREACH, etc.. |
| 248 ↗ | (On Diff #57887) | Doesn't this suggest that we should not expose the mfilter list accessors directly but instead define them in a C file as functions on a ip_moptions structure? |
| sys/netinet/ip_carp.c | ||
| 1476 ↗ | (On Diff #58312) | This line is too long, ditto below. |
| sys/netinet6/in6_var.h | ||
| 649 ↗ | (On Diff #58312) | I think it would be a bit simpler to just store a count of the filters in the moptions structure. |
| sys/netpfil/pf/if_pfsync.c | ||
| 2351 ↗ | (On Diff #58312) | This line is too long. |
Comment Actions
Address comments from @markj .
| sys/netinet/in_var.h | ||
|---|---|---|
| 248 ↗ | (On Diff #57887) | Can we leave this AS-IS for now, hence it is most compatible with the old way of doing things. And then later we can move things around which also will produce more diffs? |
| sys/netinet6/in6_var.h | ||
| 649 ↗ | (On Diff #58312) | The STAILQ() is already traversed several times, so there is no real benefit of keeping a separate counter. Can this change be made separately later on? |
| sys/netpfil/pf/if_pfsync.c | ||
| 2351 ↗ | (On Diff #58312) | Fixed. |
Comment Actions
I have no objection to the latest diff, but IMO the conversion from arrays to an STAILQ should be done in a way that does not require conditional definition of struct ip_moptions in ip_var.h.
| sys/netinet/in_var.h | ||
|---|---|---|
| 248 ↗ | (On Diff #57887) | I have no objection to the current diff aside from the one nit, but I would have found this diff easier to review if the conversion from an array to STAILQ was done in a separate diff. |
| sys/netinet6/in6_var.h | ||
| 645 ↗ | (On Diff #58759) | Should be IP6_MFILTER_FOREACH. |
| sys/netinet/in_var.h | ||
|---|---|---|
| 248 ↗ | (On Diff #57887) | OK |
Comment Actions
Please ACK. Patch will be submitted tomorrow after some additional build testing.
@markj: Thank you for spending time reviewing this patch.
| sys/netinet/ip_var.h | ||
|---|---|---|
| 98 ↗ | (On Diff #58806) | I meant to note earlier that imo_epoch_ctx appears to be unused. |