IP6 addresses

Development
1

Bro's code currently has a number of "#ifdef IPv6" blocks that I'd
really like to get rid of. It's a pain to support two different
compile-time modes, and setups where IPv6 isn't used at all are
becoming more and more rare.

I think the main internal changes for that would be (1) making
addresses generally 128-bit wide, and (2) encapsulating the logic for
manipulating addresses in its own class, something like IPAddr, or so.

I'd like to hear if folks agree with that? (1) clearly has impact on
memory usage, but I'd say that's ok these days. Alternatively, we
could use (2) to do some clever scheme that stores less bytes for IPv4
addresses than for IPv6; but that would then quite likely have in turn
an impact on CPU performance and I don't see that that'd be worth it.

Thoughts?

Robin

2

I'd like to hear if folks agree with that? (1) clearly has impact on
memory usage, but I'd say that's ok these days. Alternatively, we
could use (2) to do some clever scheme that stores less bytes for
IPv4 addresses than for IPv6; but that would then quite likely have in
turn an impact on CPU performance and I don't see that that'd be worth it.

If there's any demand for this alternative, an idea I had was that there could probably be a compile-time flag to switch between the new IPAddr implementations. But generally I think I'm with you on just doing the consistent 128-bit width at cost of memory.

- Jon

3

I'd like to avoid any compile-time option. That just makes it harder
to maintain and test the code as there are always two versions to
consider.

Robin

4

Bro's code currently has a number of "#ifdef IPv6" blocks that I'd
really like to get rid of. It's a pain to support two different
compile-time modes, and setups where IPv6 isn't used at all are
becoming more and more rare.

I agree. I think IPv6 should be enabled by default.

I think the main internal changes for that would be (1) making
addresses generally 128-bit wide, and (2) encapsulating the logic for
manipulating addresses in its own class, something like IPAddr, or so.

I think that the memory impact for (1) is going to be a lot worse than
you anticipate. A long time ago I did a benchmark and found that
enabling IPv6 doubles the memory footprint. See:
http://tracker.bro-ids.org/bro/ticket/68

Note, that the Analyzer tree already uses a scheme similar to (2) for
parsing IP headers and making the fields available (see class IP_Hdr)

I'd like to hear if folks agree with that? (1) clearly has impact on
memory usage, but I'd say that's ok these days. Alternatively, we
could use (2) to do some clever scheme that stores less bytes for IPv4
addresses than for IPv6; but that would then quite likely have in turn
an impact on CPU performance and I don't see that that'd be worth it.

Therefore, I would opt for using some "clever scheme" that stores less
bytes for IPv4. If we are going to use a class for manipulating IP
addresses, then this alone is going to cause CPU overhead. I don't think
that the additional overhead of implementing the scheme to save bytes
for IPv4-only addresses is going to be significant.

OTOH, using such an IPAddr class is already going to increase the memory
footprint (now we have to allocate a class) significantly. Possibly more
overhead then just enabling IPv6 (*)

Also note, that most IP addresses are probably going to be used on the
policy layer. Maybe we find a clever way of making those as efficient as
possible (memory and space wise), while always using 128-bits in the C++
layer.

cu
Gregor

(*) when IPv6 is enabled AddrVal stores a uint32 *. When it's
instantiated a array with 4 uint32's is allocated. When IPv4-only is
used, AddrVal stores the address as a uint32.

5

I wouldn't do any compile time flags. That's what we have right now and
it isn't pretty at all.

cu
gregor

6

> If there's any demand for this alternative, an idea I had was that
> there could probably be a compile-time flag to switch between the
> new IPAddr implementations. But generally I think I'm with you on
> just doing the consistent 128-bit width at cost of memory.

I wouldn't do any compile time flags. That's what we have right now
and it isn't pretty at all.

I probably wasn't clear, so restating in case: I meant that I agreed with getting rid of the compile-time flag to toggle IPv6 support and just supporting it by default. But a potential new compile-time flag could be offered to let users choose between the two different approaches Robin was talking about if we can't decide on one that's clearly better (CPU vs. memory tradeoff). i.e. maybe it has to be implemented and profiled both ways anyway before a decision is made.

- Jon

7

I wouldn't do any compile time flags. That's what we have right now
and it isn't pretty at all.

I probably wasn't clear, so restating in case: I meant that I agreed with getting rid of the compile-time flag to toggle IPv6 support and just supporting it by default. But a potential new compile-time flag could be offered to let users choose between the two different approaches Robin was talking about if we can't decide on one that's clearly better (CPU vs. memory tradeoff). i.e. maybe it has to be implemented and profiled both ways anyway before a decision is made.

Yeah. That's how I understood it. My concern is that we replace one
compile time flag with another. While we would have IPv6 support for all
builds then, we still have #ifdefs in the code and two different code
paths that need to be tested and maintained. So I don't see a real
improvement over the current situation there.

OTOH, I agrees that we might want to / have to implement both approach
and profile them to see what kind of overhead we'll actually have.

cu
gregor

8

My concern is that we replace one
compile time flag with another. While we would have IPv6 support for
all builds then, we still have #ifdefs in the code and two different code
paths that need to be tested and maintained.

Yeah, since you mention the burden of testing, I'm with you: we should arrive at a single implementation choice.

- Jon

9

http://tracker.bro-ids.org/bro/ticket/68

Hmm ... Ok, I didn't remember it to be *that* bad.

Therefore, I would opt for using some "clever scheme" that stores less
bytes for IPv4. If we are going to use a class for manipulating IP
addresses, then this alone is going to cause CPU overhead. I don't think
that the additional overhead of implementing the scheme to save bytes
for IPv4-only addresses is going to be significant.

OTOH, using such an IPAddr class is already going to increase the memory
footprint (now we have to allocate a class) significantly. Possibly more
overhead then just enabling IPv6 (*)

This is all hard to predict and also depends on how we implement it
(using a class by itself doesn't necessarily mean more CPU and/or
memory; it may though). So seems we indeed need measurements with
different appraoches here.

Robin

10

That looks pretty similar to my experience recently running on live traffic with v6 enabled and then disabling it and running again.

  .Seth