inpcb: Split in_pcblookup_hash_locked() and clean up a bit

[markj]

Summary

Split the in_pcblookup_hash_locked() function into several independent
subroutine calls, each of which does some kind of hash table lookup.
This refactoring makes it easier to introduce different variants of the
lookup algorithm that behave differently depending on whether they are
synchronized by SMR or the hash lock.

While here, do some related cleanup:

• Remove the unused ifnet parameter from internal functions.
• Reorder the parameters to in_pcblookup_lbgroup() to be consistent with the other lookup functions.
• Remove an always-true check from in_pcblookup_lbgroup(): we can assume that we're performing a wildcard match.

No functional change intended.

[markj]

Add some missed initializations.

[glebius]

Do you agree that removal of unused ifnet parameter should be propagated up to protocols?

[markj]

In D38364#871924, @glebius wrote:

Do you agree that removal of unused ifnet parameter should be propagated up to protocols?

Yes, I just did not want to make the diff larger. I'll submit a follow-up review to do that.

[melifaro]

sys/netinet6/in6_pcb.c

1048

There is kind of a problem with the removal I'd like to sync on.
Currently, many places in IPv6 code, including this hash, use IPv6 addresses in "embedded" form - e.g. like-local addresses with the scopeid==ifindex being embedded in the two middle bytes of the IPv6 address.
This approach violates the standard and adds quite a lot of mess to the packet processing, as there is no clear way of checking if the address is in the embedded form by looking into the code.
I plan to eliminate the embedded form (preferably before 14.0) in favour of passing scopeid directly as an argument or filling in an appropriate 'scopeid' field of sockaddr_in6. This change removes the way to pass scopeid to the the hash lookup function, blocking the possibility to remove "embedding".
Love to discuss this further.

[markj]

sys/netinet6/in6_pcb.c

1048

How exactly does this "remove the way to pass scopeid to the hash lookup function"? Isn't it "just" a matter of plumbing an extra scopeid parameter per address through the lookup functions? I agree that with this revision it will take a bit more work (and actually I plan to make it worse :), but I can't see how I am preventing such refactoring from being done. What am I missing?

[melifaro]

sys/netinet6/in6_pcb.c

1048

Yep, you're right, I probably should have expressed it in a bit more precise way.
Technically it is indeed plumbing an extra scopid - and this scopeid is derived from the ifp argument.
This particular change may not be a blocker - if the ultimate goal is not minimising the number of passing arguments. What I'm trying to check is that we don't have clashing intents that spawns beyond this diff :-)
If there are no clashes with the potential changes of in6_pcblookup() using either a new scopeidfield directly or deriving it internally from the ifp and passing it to the locked/smr versions - no concerns here :-)

[markj]

sys/netinet6/in6_pcb.c

1048

What I'm trying to check is that we don't have clashing intents that spawns beyond this diff :-)

I don't think so. Here I wanted to try and make the high-level control flow easier to follow, in preparation for some further changes which split the lookup into SMR-protected and hash lock-protected variants. The larger goal is to avoid dereferencing inp_cred without either the PCB lock or the hash lock.

So long as the ifp parameter is preserved for external callers, it should be straightforward, if a bit tedious, to plumb a scopeid parameter for v6 when that time comes. So I will leave external callers alone.

(Presumably for v4 there is no problem with eliminating the ifp parameter from external callers? I will just leave it alone anyway.)

[melifaro]

sys/netinet6/in6_pcb.c

1048

Got it! Thank you for the prompt responce and clarification :-)
Makes total sense to me.
Re ipv4: well, technically folks did introduce IPv4 link-locals in https://www.rfc-editor.org/rfc/rfc3927 , but I don’t thinks it’s worth adding the complexity and implementing it, given the current IPv6 adoption status.

[markj]

Rebase.