Page MenuHomeFreeBSD

Add hash table lookup for IPv6 raw sockets.
Needs ReviewPublic

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
Unit Tests Skipped

Event Timeline

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
142–146

You should compare the whole struct against INET6_ANY. The compiler is smart.

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

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
142–146

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
sys/netinet6/raw_ip6.c
142–146

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
216–220

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.

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
140

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?

191–194

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.

226

Where is the corresponding LOCK call?

sys/netinet6/raw_ip6.c
219

Could you please clarify why do we need to iterate through the hash here given we (supposedly) found the needed inp?

224

There was opts = NULL; in the original code somewhere here

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
219

Thanks for the clarification.

I did a port of the IPv4 code so some things are similar.

224

It was in the caller, which I am moving this code block back to per your other comment.

226

I think this code locks earlier in the execution than here.

tests/sys/netinet6/rawsocket.py
48

Sure, will do.

nc marked 3 inline comments as done.

Made the changes, here.

Don't invent my own IPv6 hash when one already exists.

nc marked an inline comment as done.Oct 20 2020, 5:38 PM