Fix IPv6 implementation for bridge statistics
Yesterday I found that the current IPv6 implementation in Tor doesn't count IPv6 connections at all. See connection_or.c lines 1717--1719:
/*XXXX IP6 support ipv6 geoip.*/
uint32_t a = tor_addr_to_ipv4h(&TO_CONN(conn)->addr);
geoip_note_client_seen(GEOIP_CLIENT_CONNECT, a, now);That's actually pretty bad. If we don't count IPv6 connections at all, our bridge statistics will be even more broken than they already are. What if IPv6 becomes popular for bridges and people start using them instead of IPv4 bridges? We'll conclude that all our bridge users have disappeared. (Entry statistics are also affected, but they're not as important as bridge statistics.)
The long-term goal will be to include a IPv6 GeoIP database in Tor and resolve IPv6 addresses to countries, too. That's something for 0.2.4.x.
But the short-term goal must be to count IPv6 connections as unresolvable addresses using country code "??", so that we get the absolute numbers right. Or if we wanted, we could use country code "?6" for unresolved IPv6 addresses.
I think we have to fix this in 0.2.3.x, despite the feature freeze. If required, I can start writing a patch today.
Activity
changed milestone to %Tor: 0.2.3.x-final
I wouldn't mind trying to squeeze this into 0.2.3. I agree that having ipv6 support for users connecting to bridges, but lacking stats, will make us sad later.
Looks like the patch would be to make geoip_note_client_seen() take a tor_addr_t, and change clientmap_entry_t to use a tor_addr_t, and then change a bunch of the clientmap functions so they handle tor_addr_t rather than uint32_t, and change geoip_get_country_by_ip() so it returns 0 if it's not a v4 address? Could get messy, but not totally crazy.
I wonder if there's a less invasive approach.
I took pretty much the approach arma described. Please see branch bug5053 in my public repository.
Note that this code has seen no testing other than
make testyet. ln5 offered to help testing tomorrow. So, please don't merge yet. Code review would be much appreciated though.Trac:
Status: new to needs_review-
+ if (tor_addr_family(addr) != AF_INET) + /*XXXX IP6 support ipv6 geoip.*/ + return -1;While technically correct, because there's one line of code, there are two lines of stuff, and it will make C programmers twitch to see it. They'll twitch because somebody will go add another line someday and think he changed it from two lines to three, so there's no need to add braces, when actually he changed it from one line to two and there is a need.
-
The
"?6"idea in the ticket description makes no sense. I opened #5055 (moved) for that. -
Ok, and now for the actual substance:
- if (tor_inet_aton((TO_CONN(conn))->address, &in)) { ... + geoip_note_client_seen(act, &TO_CONN(conn)->addr, time(NULL));Unfortunately, you've just been bitten by the "addr might be different from address" bug. "address" is where we store where we think the client is. "addr" is what IP address the connection came from.
See for example http_set_address_origin() for how they can become different.
The other case that comes to mind for how they can become different is for begindir requests. See
address = tor_dup_addr(&or_circ->p_conn->real_addr);and
if (or_circ->p_conn && !tor_addr_is_null(&or_circ->p_conn->real_addr)) tor_addr_copy(&n_stream->_base.addr, &or_circ->p_conn->real_addr);in connection_exit_begin_conn().
I think that the return value for geop_get_country_by_addr() is wrong: -1 means "we have no geoip file loaded", not "country unknown." The unknown country is 0, right?
Note to self: When I merge this, I should also change clientmap_entry_hash so that it considers the 'action' field as well.
Trac:
Status: needs_review to needs_revision-
Replying to nickm:
I think that the return value for geop_get_country_by_addr() is wrong: -1 means "we have no geoip file loaded", not "country unknown." The unknown country is 0, right?
Well, we don't have any geoip information about ipv6 addresses loaded. So I think it could count as ambiguous.
-
Replying to arma:
Replying to nickm:
I think that the return value for geop_get_country_by_addr() is wrong: -1 means "we have no geoip file loaded", not "country unknown." The unknown country is 0, right?
Well, we don't have any geoip information about ipv6 addresses loaded. So I think it could count as ambiguous.
If we want to count ipv6 as "??", then the answer must be "unknown country". If we return -1, then we won't count it.
-
Replying to nickm:
Replying to arma:
Replying to nickm:
I think that the return value for geop_get_country_by_addr() is wrong: -1 means "we have no geoip file loaded", not "country unknown." The unknown country is 0, right?
Well, we don't have any geoip information about ipv6 addresses loaded. So I think it could count as ambiguous.
If we want to count ipv6 as "??", then the answer must be "unknown country". If we return -1, then we won't count it.
I was wondering about -1 vs. 0, too. I picked -1 here because "we have no geoip file loaded" is the correct answer and because all places calling geoip_get_country_by_ip() check for country_idx < 0 and set it to 0 if they want to treat both cases as "country unknown". Now, I'm not sure if that's the cleanest approach, but I didn't want to touch it.
Linus and I are going to test this patch today. We're going to check whether IPv6 addresses are counted as "??" with the -1 result.
But if you like 0 better, I don't mind at all changing that.
-
Linus and I tested this patch. Here are the results:
- w/o GeoIP database, 1 IPv6 connection:
"bridge-stats" - w/o GeoIP database, 1 IPv4 connection:
"bridge-stats" - w/ GeoIP database, 1 IPv6 connection:
"bridge-stats ??=8" - w/ GeoIP database, 1 IPv4 connection:
"bridge-stats se=8"
We'd expect
"bridge-stats ??=8"for the first two cases, too. That's a separate bug that we should fix, too. We should count unique IP addresses even if we cannot resolve them. I'm going to look into that problem today and open a separate ticket for it. - w/o GeoIP database, 1 IPv6 connection:
Running karsten's bug5053 branch (4741aa48) plus 59271d3e ("Reduce bridge-stats interval to 1 minute for testing.")
- w/o GeoIP database, 1 IPv6 connection: "bridge-ips"
- w/o GeoIP database, 1 IPv4 connection: "bridge-ips"
- w GeoIP database, 1 IPv6 connection: "bridge-ips ??=8"
- w GeoIP database, 1 IPv4 connection: "bridge-ips se=8"
(And se=8 is reasonable given the client IP address.)
Maxmind actually has a GeoIP database for IPv6; I'm not sure how well it works since I don't have a lot of ipv6 traffic to work from.
I've added support for the IPv6 GeoIP database (including test cases) to the "nils-bug5053" branch on git@github.com:shkoo/tor.git; how does that look?
Trac:
Username: nils
Status: needs_revision to needs_review-
Trac:
Username: nils
Cc: nickm, arma, ln5 to nickm, arma, ln5, nils@shkoo.com Looks very good to me. With tests and all!
A few comments:
-
I would've collapsed more of the functions into one too, f.ex. geoip_get_country_by_ip() by adding a 'family' argument.
-
I'd renamed data types, lists and functions specific to IPv4 (like struct geoip_entry_t) but I'd like to hear others view on this.
-
There's tor_addr_compare() which I think should be used instead of memcmp().
-
A comment or two in geoip_get_client_history() explaining how this works for both IPv4 and IPv6 would be nice but not necessary.
-
Remove prototype for geoip_load_ipv6_file() in geoip.h.
Coding style:
-
I don't think we do 'if (foo) bar;' without a newline.
-
A few whitespace changes should go away (geoip.c:346, geoip.c:497, test.c:1479, test.c:1516).
I think Nick should take a look at this too before merging.
I will run this on my IPv6 bridge and see how it works.
Thanks!
-
-
Replying to ln5:
A few comments:
- I would've collapsed more of the functions into one too, f.ex. geoip_get_country_by_ip() by adding a 'family' argument.
This is a bit tricky seeing as the arguments and data structures are all different types. geoip_get_country_by_addr already does the selection on address family and dispatches to the proper routine. Or maybe I'm not understanding what you're suggesting here.
- I'd renamed data types, lists and functions specific to IPv4 (like struct geoip_entry_t) but I'd like to hear others view on this.
I would like to hear others views on this as well. I went for having a smaller changeset rather than renaming all of them, but it's easy enough to rename them.
- There's tor_addr_compare() which I think should be used instead of memcmp().
Unless I'm missing some, all of the memcmp cases are when we have specifically a in6_addr. I guess we could use tor_addr_from_in6 to make a tor_addr_t and then use tor_addr_compare, but that seems like a lot of overhead and isn't nearly as clear.
I chose not to have a combined geoip database using tor_addr_t because of the additional memory usage (there are about 150,000 entries in the ipv4 geoip database) and it seemed a bit wasteful, but if we want to waste the extra 4 MB RAM we could use generic tor_addr_t addresses for the in-memory structure.
I committed the cosmetic changes you suggested onto my branch.
Trac:
Username: nils Replying to nils:
- I would've collapsed more of the functions into one too, f.ex. geoip_get_country_by_ip() by adding a 'family' argument.
This is a bit tricky seeing as the arguments and data structures are all different types. geoip_get_country_by_addr already does the selection on address family and dispatches to the proper routine. Or maybe I'm not understanding what you're suggesting here.
Ah, I missed geoip_get_country_by_addr(). It indeed does what I suggest. It'd be nice to hide geoip_get_country_by_ip() and geoip_get_country_by_ipv6() by making them static. Unless callers will get inefficient or ugly by calling tor_addr_from_ipv4n()?
- I'd renamed data types, lists and functions specific to IPv4 (like struct geoip_entry_t) but I'd like to hear others view on this.
I would like to hear others views on this as well. I went for having a smaller changeset rather than renaming all of them, but it's easy enough to rename them.
That change would go in a separate commit, preferrably predating your large commit with the real implementation.
- There's tor_addr_compare() which I think should be used instead of memcmp().
Unless I'm missing some, all of the memcmp cases are when we have specifically a in6_addr. I guess we could use tor_addr_from_in6 to make a tor_addr_t and then use tor_addr_compare, but that seems like a lot of overhead and isn't nearly as clear.
Aha! struct geoip_ipv6_entry_t has in6_addr's, not tor_addr_t's. Makes sense then.
I chose not to have a combined geoip database using tor_addr_t because of the additional memory usage (there are about 150,000 entries in the ipv4 geoip database) and it seemed a bit wasteful, but if we want to waste the extra 4 MB RAM we could use generic tor_addr_t addresses for the in-memory structure.
No, separate structures is the right thing IMO.
-
So to be clear , we have two branches; karsten's "bug5058" and nils's bug5058-nils?
It appears that karsten's thing is suitable for 0.2.3.x (since it's minimal) and the Nils version is suitable for 0.2.4.x (since it's a new feature).
Karsten, I am not seeing any change to the return value of geoip_get_country_by_addr() for ipv6 addresses?
-
Replying to nickm:
Karsten, I am not seeing any change to the return value of geoip_get_country_by_addr() for ipv6 addresses?
Right. Did you want me to change it?
-1 is the correct answer if we don't have geoip information available to resolve an address, which is the case for IPv6 addresses. Note how all current callers of geoip_get_country_by_addr() check whether the response is < 0 and then set it to 0. They don't want to distinguish between "no geoip information available" and "not resolved to any country." But future callers might want to distinguish, which is why we're returning -1, not 0.
If you think this implementation is too confusing, we should change both geoip_get_country_by_ip() and geoip_get_country_by_addr() and dump the -1 return value entirely. I can do that.
-
Replying to nickm:
Merged karsten/bug5053 , saving nils's version for 0.2.4.x. Thanks!
Thanks for merging!
nils, I reviewed your bug5053-nils branch. Looks pretty neat to me!
I fixed a few minor things and added some comments in branch nils-bug5053-bug5055 in my public repo (that branch has 1 commit for #5053 comments/fixes and 1 commit for #5055 comments/fixes; maybe we should split up the branch into two). Two other issues are that a changes/ file is missing and that
make check-spacesis unhappy about tabs, long lines, and trailing whitespace.Want to tackle the remaining issues, or shall I give them a try?
Trac:
Status: needs_review to needs_revision Karsten, wanna finish this or should I?
Tor 0.2.4.1-alpha was tagged the other day, with 'bridge-ip-versions' in stats/bridge-stats (#5055 (moved)) but without support for GeoIPv6File.
-
Replying to ln5:
Karsten, wanna finish this or should I?
I'm afraid I cannot work on this anytime soon. If you can handle it, please do. Thanks!
Branch bug5053-bug5055 in my public repo contains a changes file for this enhancement. Please see #5055 (moved) for more info about that branch.
Trac:
Status: assigned to needs_review-
okay, I'll try to review the whole branch here...
Stuff we could fix after merge:
- The duplicated code in the new geoip*ipv6() functions is scary. Gotta refactor that out. Cut-and-pasting in programming is begging for bloat and bugs.
- Passing in6_addr by value? it is a 16-byte structure; that kind of thing one usually passes by reference.
- Comparing in6_addrs with memcmp is iffy; there's no guarantee that they can't contain padding or uninitialized fields. If we're going to use memcmp, we should probably be comparing the s8_addr fields.
- The comment that says that the in6_addrs are in "host order" seems wrong.
- The comment /* No alignment issue here, since _key really is a pointer to uint32_t */ seems wrong.
- The clear_geoip_db(); that got removed from geoip_load_file() : where did it go? It looks like we just removed it entirely, which means that reloading a geoip file will wind up with entries both from the old file and the new one. That's a big bug.
- Gotta fix all the XXX5053; are there any left?
Trac:
Status: needs_review to needs_revision Replying to nickm:
okay, I'll try to review the whole branch here...
Stuff we could fix after merge: Or before I guess. Please see branch bug5053-bug5055 in my public repo.
- The duplicated code in the new geoipipv6() functions is scary. Gotta refactor that out. Cut-and-pasting in programming is begging for bloat and bugs. Agreed. Commit 6a241ff3 merges geoip_ipv_add_entry() and geoip_ipv*_parse_entry().
- Passing in6_addr by value? it is a 16-byte structure; that kind of thing one usually passes by reference. Gone with 6a241ff3.
- Comparing in6_addrs with memcmp is iffy; there's no guarantee that they can't contain padding or uninitialized fields. If we're going to use memcmp, we should probably be comparing the s8_addr fields. Fixed in commit e7e68b80. Or do you prefer comparing the s6_addr32's without memcmp?
- The comment that says that the in6_addrs are in "host order" seems wrong. Gone with e7e68b80.
- The comment /* No alignment issue here, since _key really is a pointer to uint32_t */ seems wrong. Gone with e7e68b80.
- The clear_geoip_db(); that got removed from geoip_load_file() : where did it go? It looks like we just removed it entirely, which means that reloading a geoip file will wind up with entries both from the old file and the new one. That's a big bug. geoip_load_file() frees geoip_ipv*_entries before adding new data. geoip_ipv*_add_entry() only adds to geoip_countries data not already present.
- Gotta fix all the XXX5053; are there any left? No.
Trac:
Status: needs_revision to needs_review-
Okay; I think this might be ready! I'm going to ask andrea if she wants to review it before we merge, and if not, go ahead and merge it in; thanks!
Trac:
Cc: nickm, arma, ln5, nils@shkoo.com to nickm, arma, ln5, nils@shkoo.com, andrea I've reviewed this and then read nickm's review and found he had about the same set of observations I came up with. The clear_geoip_db() thing and the alignment issue particularly stood out to me. I'm satisfied that the clear_geoip_db() isn't failing to clear the database when reloading, but some of the code in geoip_load_file() seems a bit klunky to me still. That function starts off with a call to clear_geoip_db(), then does an init_geoip_countries() if it isn't NULL (isn't it always NULL after clear_geoip_db()?), then frees geoip_entries and replaces it with a new, empty smartlist_t. Seems rather redundant.
Replying to andrea:
I've reviewed this and then read nickm's review and found he had about the same set of observations I came up with. The clear_geoip_db() thing and the alignment issue particularly stood out to me. I'm satisfied that the clear_geoip_db() isn't failing to clear the database when reloading, but some of the code in geoip_load_file() seems a bit klunky to me still. That function starts off with a call to clear_geoip_db(), then does an init_geoip_countries() if it isn't NULL (isn't it always NULL after clear_geoip_db()?), then frees geoip_entries and replaces it with a new, empty smartlist_t. Seems rather redundant.
Apparently I have the stupid today. I was looking at the old one when I wrote that. The new geoip_load_file() seems much less silly.
As for the alignment issue, the original comment is flat wrong; it isn't safe to assume that a pointer aligned to uint32_t's alignment can be dereferenced as any type. There exist platforms where uint32_t has 4-byte alignment but pointers and uint64_t have 8-byte alignment. However, since the keys are really struct in6_addr * and just passed as void for genericity of the sort code, as long as they were originally allocated with the proper alignment then casting back and forth to void within the same type should always be safe.
mentioned in issue #5055 (moved)
mentioned in issue #7354 (moved)
moved to tpo/core/tor#5053 (closed)
mentioned in issue tpo/core/tor#5055 (closed)
mentioned in issue tpo/core/tor#7354 (closed)