Add hash table lookup for IPv6 raw sockets.
Needs ReviewPublic
Actions

Authored by nc on Oct 13 2020, 1:50 AM.

Details

Reviewers

ae
bz
melifaro
mav

Group Reviewers

network

Summary

Add hash table lookup for IPv6 raw sockets. This is akin to r180828 for IPv4.

Submitted by: Neel Chauhan <neel AT neelc DOT org>
PR: 232346

Test Plan

Use an application that creates raw IPv6 sockets.

I have tried this patch on IPv6 dhcpcd, but don't know if dhcpcd uses raw IPv6 sockets.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

nc created this revision.Oct 13 2020, 1:50 AM

Herald added a subscriber: imp. · View Herald TranscriptOct 13 2020, 1:50 AM

nc requested review of this revision.Oct 13 2020, 1:50 AM

It's quite a lot of code. So the question is, why this effort?

sys/netinet6/raw_ip6.c
143–147	You should compare the whole struct against INET6_ANY. The compiler is smart.

The reason for doing this is that the performance of raw IPv6 sockets is improved, like it was for IPv4 here: https://svnweb.freebsd.org/base?view=revision&revision=180828

sys/netinet6/raw_ip6.c
143–147	There is no INET6_ANY. The closest I found, IN6ADDR_ANY_INIT did not compare.

nc marked an inline comment as done.Oct 14 2020, 7:16 PM

afedorov added a reviewer: mav.Oct 14 2020, 7:31 PM

bz added inline comments.Oct 14 2020, 7:43 PM

sys/netinet6/raw_ip6.c
143–147	IN6_IS_ADDR_UNSPECIFIED()?

Thanks for working on this!
It would be nice to have some tests attached to the diff to improve our continuous testing efforts :-)
Should be relatively easy to do with python sockets.

sys/netinet6/raw_ip6.c
274–278	Now this function is 240 LoC, which makes it pretty hard to read. Would it be possible to move hash lookup(s) into a separate functions?

This revision now requires changes to proceed.Oct 14 2020, 7:56 PM

Sure, done. Thanks for the pointers, @bz and @melifaro!

However, the hash lookup function has a lot of arguments.

I also have a test and it passes. It's just a simple ICMPv6 ping. If you need something more complex, please tell me.

Herald added a subscriber: asomers. · View Herald TranscriptOct 15 2020, 3:15 AM

nc marked 2 inline comments as done.Oct 15 2020, 3:15 AM

I do not understand, why you do not need to calculate the hash during the delete operation.

sys/netinet6/raw_ip6.c
141	I do not understand the hash function. Which part of the address is used in this calculation? How strong is the influence of the protocol, does is vary between entries?
192–195	I'm not satisfied with this function. It should only return the found entry (or NULL), but it does a lot of extra work, incl. resetting external locks, modifying data.
227	Where is the corresponding LOCK call?

melifaro added inline comments.Oct 16 2020, 7:54 PM

sys/netinet6/raw_ip6.c
225	There was `opts = NULL;` in the original code somewhere here
278	Could you please clarify why do we need to iterate through the hash here given we (supposedly) found the needed inp?
tests/sys/netinet6/rawsocket.py
48	Well, as we're altering rip6_input(), which deals with the packets received by the kernel, I'd expect the test to cover receiving raw packets :-)

nc marked 5 inline comments as done.Oct 16 2020, 8:53 PM

nc added inline comments.

sys/netinet6/raw_ip6.c
225	It was in the caller, which I am moving this code block back to per your other comment.
227	I think this code locks earlier in the execution than here.
278	Thanks for the clarification. I did a port of the IPv4 code so some things are similar.
tests/sys/netinet6/rawsocket.py
48	Sure, will do.