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

[nc]

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.

[emaste]

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.

[nc]

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

I added a description.

[donner]

sys/netinet/udp_usrreq.c

560–566

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

563

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

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

361

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?

[nc]

sys/netinet/udp_usrreq.c

560–566

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

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

[donner]

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.

[nc]

Fixed the issue with full context and paths.