inpcb: resort to wildcard hash lookup when checking for TCP port conflict
Needs ReviewPublic
Actions

Authored by glebius on Wed, Jul 1, 5:50 AM.

Details

Reviewers

markj

Group Reviewers

network

Summary

On bind(2) TCP sockets do not conflict with existing connected sockets
using the same port. We check only wildcard bound sockets. Instead of
looking through the all connections ports hash and then checking that
foreign address of an inpcb found is a wildcard, just short circuit to wild
hash lookup. Provide a better comment with a reference to the change that
introduced this logic.

Should be no behavior change.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 74447
Build 71330: arc lint + arc unit

Event Timeline

glebius created this revision.Wed, Jul 1, 5:50 AM

Herald added subscribers: pouria, melifaro, ae, imp. · View Herald TranscriptWed, Jul 1, 5:50 AM

glebius requested review of this revision.Wed, Jul 1, 5:50 AM

Harbormaster completed remote builds in B74447: Diff 181150.Wed, Jul 1, 5:50 AM

markj added inline comments.Thu, Jul 2, 2:19 PM

sys/netinet/in_pcb.c
956	Code comments should be self-contained, we generally avoid these kinds of external references.

glebius added inline comments.Thu, Jul 2, 3:26 PM

sys/netinet/in_pcb.c
956	That's something new to me! Where is this policy? The amount of information on the problem is quite large to put in a comment that justifies just one line. The bug numbers are stable for the lifetime of the project and git hashes are stable just in principle.

markj added inline comments.Thu, Jul 2, 3:34 PM

sys/netinet/in_pcb.c
956	It's not a formal policy AFAIK. But where else do we have such references? In test code it makes some sense, but IMO it's wrong to do this in the kernel.

glebius added inline comments.Thu, Jul 2, 3:44 PM

sys/netinet/in_pcb.c
956	`grep 'bug [0-9]+'` gives some precedents. I tried `PR [0-9]+]`, but too many false positives. This particular case takes many many iterations of `git annotate` to understand the logic behind the SOCK_STREAM check. With my refactoring there will be one step more. I want to save people's time and immediately link them to investigation that lead to this special treatment. P.S. In a couple major release I hope this will fully go away, as we aim at TCP sockets always behave as if SO_REUSEADDR is set.