Page MenuHomeFreeBSD

netinet: In multicast/broadcast udp(6)_input(), compare the IP details after we lock inp as well
Needs ReviewPublic

Authored by neel_neelc.org on Mon, Oct 7, 7:40 PM.

Details

Reviewers
None
Group Reviewers
transport
Summary

In r222488, we introduced ipi_hash_lock alongside ipi_lock, which stopped using ipi_lock for stabilizing four-tuple source/destination matches. This patch adds a second check once the inpcb lock is held in udp_input() and udp6_input().

Also, compare the IP details as a separate function to prevent code duplication.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

neel_neelc.org created this revision.Mon, Oct 7, 7:40 PM
neel_neelc.org retitled this revision from netinet: In multicast/broadcast udp_input(), compare the IP details after we lock inp as well to netinet: In multicast/broadcast udp(6)_input(), compare the IP details after we lock inp as well.
neel_neelc.org edited the summary of this revision. (Show Details)
emaste added a subscriber: emaste.Thu, Oct 17, 3:55 PM

Two points - first, please upload with full context (see https://wiki.freebsd.org/action/show/Phabricator for commandline examples). Second, please more details about the reason for the patch - e.g. a description of the problem that this solves, a link to the PR, a reproduction case, etc.

neel_neelc.org edited the summary of this revision. (Show Details)Thu, Oct 17, 7:48 PM

Sorry for the issues, I'm new to kernel hacking, despite doing Ports for many years.

I added a description.

lutz_donnerhacke.de added inline comments.
sys/netinet/udp_usrreq.c
560–566 ↗(On Diff #63234)

Why is this remark solved?
I do see no code for handling the concern nor any documentation why it is not justified.

563 ↗(On Diff #63234)

I do not get the second condition.
This was already evaluated in line 558 above and caused a "continue".
So in which case will this condition ever be effective?

sys/netinet6/udp6_usrreq.c
343–349 ↗(On Diff #63234)

Why is this remark solved?
I do see no code for handling the concern nor any documentation why it is not justified.

361 ↗(On Diff #63234)

I do not get the second condition.
This was already evaluated in line 348 above and caused a "continue".
So in which case will this condition ever be effective?

neel_neelc.org added inline comments.Fri, Oct 18, 3:58 PM
sys/netinet/udp_usrreq.c
560–566 ↗(On Diff #63234)

The code is handled by the condition:

!udp_inpcb_eq_ip_uh(inp, ip, uh)

So if we don't get a match, we unlock and continue.

This is based on r222488 around the "In the UDPv4 and UDPv6 multicast cases" lins: https://freshbsd.org/commit/freebsd/src/222488

563 ↗(On Diff #63234)

The second condition checks for a match, related to the comment I removed below.

The ip/uh data is fixed in this function, so only the inp structure might be modified.
My question is: How can the "ip/port" information of an pcb entry (inp) get changed?

Is there a syscall, so that the application can rebind those parameters of a listening socket?
Normally, new entries will occur for new bindings and old entries will be freed.
After obtaining the lock, the "freed" state is ruled out, so only living sockets survive.

If there is not way to change those values in a found inp, the secondary test is not necessary at all.
The old comment points to such thoughts of the author, who was unsure if there is such a possibility or not.

BTW: Can you please resubmit the diff with full context an correct paths? Clicking on the path names in the diff results in "missing file" errors, because the branch name is missing.

May you even provide a diff against HEAD for correct line numbers and the new epoch locking environment? Thanks.

Fixed the issue with full context and paths.