On Fri, Apr 29, 2016 at 06:48:39PM +0200, raf wrote:

Wow what a great update. thanks you !

...

o Static: Allow to specify attributes for static routes Do you consider the use of communauties on static routes (like on junos) ?

You could use any attributes for static routes, even BGP communities. It is essentially per-route import filter. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

On 29.4.2016 20:24, Evelio VILA wrote:

Thank you Ondrej! Unless I'm missing something, I thought the mrtdump branch was set to be merged on this release.

Hi! It was not. Ondrej

Best, Evelio

On Fri, Apr 29, 2016 at 9:35 AM, Ondrej Filip <feela@network.cz <mailto:feela@network.cz>> wrote:

Dear Bird users, the long expected version is out:

Version 1.6.0 (2016-04-29) o Major RIP protocol redesign o New Babel routing protocol o BGP multipath support o KRT: Add support for plenty of kernel route metrics o KRT: Allow more than 256 routing tables o Static: Allow to specify attributes for static routes o Static: Support for BFD controlled static routes o FreeBSD: Setup password for BGP MD5 authentication o IO: Remove socket number limit o Plenty of bug fixes

Upgrade notes:

For RIP, most protocol options were moved to interface blocks.

Enjoy!

Ondrej

Hi, On Fri, Apr 29, 2016 at 8:24 PM, Ondrej Zajicek <santiago@crfreenet.org> wrote:

I would like to thank Toke Høiland-Jørgensen for the Babel protocol implementation, which was finally merged despite my tardy code reviews.

sorry to ask, but your upstream git(-lab) is not responding to me: Did you also include our IPv6 ECMP fixes from <1456142494-3822-1-git-send-email-mikhail.sennikovskii@profitbricks.com> by chance? -- Arno Töll GnuPG Key-ID: 0x9D80F36D

Hi Ondrej, 2016-04-29 22:51 GMT+02:00 Ondrej Filip <feela@network.cz>:

...

you also include our IPv6 ECMP fixes from <1456142494-3822-1-git-send-email-mikhail.sennikovskii@profitbricks.com> by chance?

Not yet.

I can provide an updated set of patches against bird 1.6, and/or 2.0 if it speeds up the things a bit. Mikhail

On Sat, Apr 30, 2016 at 01:20:14AM +0200, Toke Høiland-Jørgensen wrote:

On 29 April 2016 20:24:43 CEST, Ondrej Zajicek <santiago@crfreenet.org> wrote:

...

I would like to thank Toke Høiland-Jørgensen for the Babel protocol implementation, which was finally merged despite my tardy code reviews.

Yay, awesome! You're very welcome, and congratulations on the release! :)

Congrats, Toke! I have just tested your implementation (from a pure user perspective, I haven't looked at the code), and it looks great! Thanks to you, I will have to switch my test overlay network to IPv6-only... I have found a few issues while playing with Bird 1.6.0. I also have some general comments, notably about the documentation, but that will wait for another email :) === Configuration === When the quotes are missing around the interface name, the error message is misleading. This configuration: protocol babel { interface eth0; } gives: /etc/bird6.conf, line 240: IP address expected which is very strange, because an IP address does not make sense here (and something like "interface 2001:db8::42" obviously gives an error). === Filtering === When updating a filter that removes some routes (e.g. switching from "export all" to "export none"), the routes stay in the Babel table, even though they are no longer exported to the protocol: bird> show babel entries babel1 babel1: Prefix Router ID Metric Seqno Expires Sources 2001:db8:42::/64 <pending> 1 bird> show route export babel1 bird> === Hello interval range checking === The hello interval cannot be set below 1 second: the parser seems to expect an integer. The packet format encodes intervals in centiseconds, so it would make sense to allow any fractional Hello interval down to 0.01 seconds. I just checked, babeld's parser allows to go as low as 0.001s (which is a bit scary in itself, since it generates about 1 kpps / 1.5 Mbps of control traffic). On a related note, there is no overflow check on the hello interval. For instance, setting the hello interval to 800 seconds produces this in tcpdump: Hello seqno 1 interval 144.64s 80000 centiseconds is too large for 16 bits, and overflows. babeld does check for this, it only allows values up to 655.35 seconds in the configuration. There is a similar issue with the IHU interval and route update interval, which are set to 3 (resp. 4) times the Hello interval. When setting the hello interval so that 3*hello or 4*hello overflows, for instance hello interval = 225 seconds, both IHU and route updates packets contain an overflowed interval: Update 2001:db8:42::/64 metric 0 seqno 1 interval 244.64s IHU fe80::e8db:78ff:fe05:8a64 txcost 96 interval 19.64s This time, babeld seems to have the same issue: when setting the Hello interval too high, the IHU and route intervals do overflow. Looking at tcpdump with babeld and hello-interval set to 600s: Update 172.23.184.205/32 metric 0 seqno 6295 interval 433.92s Hello seqno 42182 interval 600.00s sub-timestamp 2159.389437s IHU fe80::78db:e7ff:fe98:4cde txcost 65535 interval 489.28s === Interaction between reconfiguration and timers === Changing the hello interval and telling bird to reconfigure itself leads to a somewhat inconsistent behaviour: - when decreasing the hello interval, the old interval is still used for the next Hello message, but the new interval is immediately taken into account for IHU and Update messages. I don't think this is a real interoperability issue, but it's just strange to see a lot of Updates and IHU without any Hello (when decreasing the hello interval substantially, for instance from one minute to 3 seconds) Increasing the hello interval works as expected (the old interval is still used for the next Hello, IHU and Update messages). This looks correct and consistent with the RFC, section 3.4.1: the interval change is actually taken into account when sending the next Hello message, because otherwise our neighbours could think we are dead. === TLV parsing error === When a neighbouring babeld starts or stop, Bird complains about an error parsing a TLV. When starting babeld: avril 30 12:23:38 lud bird6[17307]: babel1: Bad TLV from fe80::e8db:78ff:fe05:8a64 via tap-fastd type 8 pos 18 - parse error avril 30 12:23:38 lud bird6[17307]: babel1: Bad TLV from fe80::e8db:78ff:fe05:8a64 via tap-fastd type 8 pos 18 - parse error When stopping babeld: avril 30 12:23:59 lud bird6[17307]: babel1: Bad TLV from fe80::e8db:78ff:fe05:8a64 via tap-fastd type 8 pos 4 - parse error avril 30 12:23:59 lud bird6[17307]: babel2: Bad TLV from fe80::e8db:78ff:fe05:8a64 via tap-fastd type 8 pos 4 - parse error Packet captures are attached (babeld-start-parse-error.pcap and babeld-stop-parse-error.pcap). The local node running Bird is fe80::c07d:62ff:fea6:b8f4, all other speakers are babeld 1.5.0. Type 8 is the Update TLV, I think Bird complains about this particular message (here seen by tcpdump): Update/id ::/0 metric 65535 seqno 21902 interval infinity These are all minor issues, your implementation globally seems to work fine :) Thanks, Baptiste

On Sat, Apr 30, 2016 at 03:15:52PM +0200, Toke Høiland-Jørgensen wrote:

...

=== Configuration ===

When the quotes are missing around the interface name, the error message is misleading. This configuration:

protocol babel { interface eth0; }

gives:

/etc/bird6.conf, line 240: IP address expected

which is very strange, because an IP address does not make sense here (and something like "interface 2001:db8::42" obviously gives an error).

Not sure why this is. The inner workings of the configuration parsing still has some a good portion of black magic...

Ah, I thought that this "interface" statement was specific to Babel, but it's actually defined for all protocols. The syntax seems fairly complex: http://bird.network.cz/?get_doc&f=bird-3.html#ss3.3 IP prefixes are allowed for OSPF, which explains the error message.

...

=== Filtering ===

When updating a filter that removes some routes (e.g. switching from "export all" to "export none"), the routes stay in the Babel table, even though they are no longer exported to the protocol:

bird> show babel entries babel1 babel1: Prefix Router ID Metric Seqno Expires Sources 2001:db8:42::/64 <pending> 1 bird> show route export babel1 bird>

These are routes coming from the box itself (not from peers)? They should expire after BABEL_HOLD_TIME (10 seconds)...

Yes, these are local routes, imported from the kernel. I do see a transient state of about 10 seconds, but this "pending" state happens *after* the transient state. Before changing the export filter: Prefix Router ID Metric Seqno Expires Sources 2001:db8:42::/64 <self> 0 1 0 1 Just after running "configure" (the route will expire in 9 seconds): Prefix Router ID Metric Seqno Expires Sources 2001:db8:42::/64 <self> 65535 1 9 1 After the route has expired: Prefix Router ID Metric Seqno Expires Sources 2001:db8:42::/64 <pending> 1 The route stays in this state for quite a while (around 5 minutes), then disappears.

...

=== Hello interval range checking ===

The hello interval cannot be set below 1 second: the parser seems to expect an integer. The packet format encodes intervals in centiseconds, so it would make sense to allow any fractional Hello interval down to 0.01 seconds.

I just checked, babeld's parser allows to go as low as 0.001s (which is a bit scary in itself, since it generates about 1 kpps / 1.5 Mbps of control traffic).

Hmm, yes. However, since the internal Bird timers run at a granularity of seconds only there's not much point in having the ability to configure smaller values.

Fair enough, it seems reasonable.

...

=== Interaction between reconfiguration and timers ===

Hmm, is this consistent? Reconfiguration simply replaces the struct with the values in the internal data structures, so the only reason I can see why you would get that behaviour is because there's a TLV that happens to be queued at the time you reconfigure. You can verify this by looking at the debug output when running with TRACE level debugging enabled; that outputs messages when the TLVs are generated, not when they are sent out.

Ok, I will try to have a closer look.

I do seem to have forgotten to generate a new hello when reconfiguring (RFC section 3.4.1: "Equivalently, a node SHOULD send an unscheduled Hello immediately after increasing its Hello interval."). I do believe just adding that should resolve the issues with inconsistent behaviour?

Oh, I thought you had already taken that into account (by applying the new interval only after the next Hello message, which is another way of ensuring neighbours don't forget us), but that's possibly the queuing issue you mention.

...

=== TLV parsing error === Type 8 is the Update TLV, I think Bird complains about this particular message (here seen by tcpdump):

Update/id ::/0 metric 65535 seqno 21902 interval infinity

I remember running into this. What happens here is that babeld sends an update without a preceding router_id TLV, with a wildcard address, but flag 0x40 set (meaning "infer the router ID from the address").

While I'm not sure what the purpose of this is (a null update with a null router ID with infinity metric and interval?) it *is* technically in spec.

Juliusz will certainly have an answer.

I think the reason why Bird complains is that Ondrej's cleanup of my (admittedly messy) packet parsing code inadvertently moved the check for the 0x40 flag inside the case branch for AE_IP6.

If you turn on debugging you should get a "No router ID seen before update" message which will confirm that this is indeed the issue.

Hmm, even with "debug all" on the protocol, it only shows this: avril 30 15:44:33 lud bird6[17966]: babel1: Packet received from fe80::e8db:78ff:fe05:8a64 via tap-fastd avril 30 15:44:33 lud bird6[17966]: babel1: Bad TLV from fe80::e8db:78ff:fe05:8a64 via tap-fastd type 8 pos 18 - parse error avril 30 15:44:33 lud bird6[17966]: babel1: Handling hello seqno 39103 interval 4

Most of these issues are fairly trivial fixes. I'll produce a patch once I'm done grokking Ondrej's code cleanup changes :)

Great :) Thanks, Baptiste

On Sat, Apr 30, 2016 at 04:15:46PM +0200, Toke Høiland-Jørgensen wrote:

Baptiste Jonglez <baptiste@bitsofnetworks.org> writes:

...

Ah, I thought that this "interface" statement was specific to Babel, but it's actually defined for all protocols. The syntax seems fairly complex:

http://bird.network.cz/?get_doc&f=bird-3.html#ss3.3

IP prefixes are allowed for OSPF, which explains the error message.

Ah, right. Well, seems Babel wasn't mentioned in that part of the doc either. Putting that on my list of small fixes ;)

Since you mention documentation, I see a couple of changes you can make: - add RTS_BABEL in http://bird.network.cz/?get_doc&f=bird-5.html#ss5.5 - list babel-specific route attributes somewhere - add a few examples (interface without parameters, wildcard interface like "tun-*", multiple interfaces in the same interface block) - talk about filtering: right now, the only mention is very indirect: Babel supports no global configuration options apart from those common to all other protocols It's not that obvious that you can use import/export functions to filter routes. This is especially important because the default policy (import all, export none) is not very useful. For instance, the default behaviour of babeld, exporting only local addresses, can be approximated by using the "direct" protocol and setting this policy for babel: export where source = RTS_DEVICE; It's a bit different from babeld, though: if 2001:db8:42:42::1/64 is configured on an interface, babeld would advertise 2001:db8:42:42::1/128, while the above filter would advertise 2001:db8:42:42::/64. I think it's ok, because for a non-transitive ethernet network you should use a /128 on the interface anyway. Baptiste

On Sat, Apr 30, 2016 at 04:41:35PM +0200, Baptiste Jonglez wrote:

It's not that obvious that you can use import/export functions to filter routes. This is especially important because the default policy (import all, export none) is not very useful.

For instance, the default behaviour of babeld, exporting only local addresses, can be approximated by using the "direct" protocol and setting this policy for babel:

export where source = RTS_DEVICE;

Note that Babel in BIRD does not redistribute interally, so you have to use 'export where (source = RTS_DEVICE) || (source = RTS_BABEL)' to propagate Babel routes. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

Juliusz Chroboczek <jch@pps.univ-paris-diderot.fr> writes:

...

I remember running into this. What happens here is that babeld sends an update without a preceding router_id TLV, with a wildcard address, but flag 0x40 set (meaning "infer the router ID from the address"). While I'm not sure what the purpose of this is (a null update with a null router ID with infinity metric and interval?) it *is* technically in spec.

No, it isn't.

Earlier versions of babeld allowed a retraction (update with infinite metric) without a router-id. This is not allowed by RFC 6126. Current versions of babeld no longer send such retractions, but still honour them when received.

But those updates seem to set flag 0x40, so that's not "without a router ID" is it?

The plan is to explicitly allow such retractions in RFC 6126-bis, but they are clearly not allowed by RFC 6126.

Hmm, the RFC says this (which I seem to have previously missed): If the metric field is FFFF hexadecimal, this TLV specifies a retraction. In that case, the current router-id and the Seqno are not used. AE MAY then be 0, in which case this Update retracts all of the routes previously advertised on this interface. Doesn't that make them in spec? -Toke

Juliusz Chroboczek <jch@pps.univ-paris-diderot.fr> writes:

...

But those updates seem to set flag 0x40, so that's not "without a router ID" is it?

Yeah, it was meant to clear the router-id.

...

...

The plan is to explicitly allow such retractions in RFC 6126-bis, but they are clearly not allowed by RFC 6126.

...

Hmm, the RFC says this (which I seem to have previously missed):

...

If the metric field is FFFF hexadecimal, this TLV specifies a retraction. In that case, the current router-id and the Seqno are not used. AE MAY then be 0, in which case this Update retracts all of the routes previously advertised on this interface.

...

Doesn't that make them in spec?

It was certainly my intent to allow them, but Markus disagreed.

Well I would certainly argue that it was allowed; but apparently easy to miss. Will think about what to do about it in the Bird code. Why is babeld setting the 0x40 flag on those updates, though? -Toke

Juliusz Chroboczek <jch@pps.univ-paris-diderot.fr> writes:

...

Why is babeld setting the 0x40 flag on those updates, though?

It was supposed to clear the router-id. Recent versions no longer do.

Ah, I see. But surely, having an update with AE 0 and the flag set would not be out of spec either, as long as router ID 0 is not disallowed? -Toke

Toke Høiland-Jørgensen <toke@toke.dk> writes:

...

The plan is to explicitly allow such retractions in RFC 6126-bis, but they are clearly not allowed by RFC 6126.

Hmm, the RFC says this (which I seem to have previously missed):

If the metric field is FFFF hexadecimal, this TLV specifies a retraction. In that case, the current router-id and the Seqno are not used. AE MAY then be 0, in which case this Update retracts all of the routes previously advertised on this interface.

Okay, actually trying to put this into code: Is the intention here that a null-router ID update is acceptable only on *wildcard* retractions or on *all* retractions? -Toke

Juliusz Chroboczek <jch@pps.univ-paris-diderot.fr> writes:

...

Okay, actually trying to put this into code: Is the intention here that a null-router ID update is acceptable only on *wildcard* retractions or on *all* retractions?

In RFC 6126, there's nothing special about a null router-ID: it's just a router ID.

I didn't actually mean a 'null router ID'. I meant an *unset* router ID. I.e., if flag 0x40 is set or the update is preceded by a router ID TLV, the router ID is *set*. It may or may not be set to all-zeroes, but that is orthogonal. So I was referring to the text stating "the current router-id and seqno is not used" - does that refer to all retractions or just wildcard ones? (I suspect the answer to be the former, and that the fact that this poses problems is an artifact of the current update handling flow in the Bird code; but want to be sure before I change it).

However, for AEs 0 and 1, the address is too short to carry a router-ID (it's 0 and 4 octets respectively, while a router-ID is 8 octets). The intention was that a shorter address should be stored in the right side of a router-ID, and padded with zeroes; e.g. the IPv4 address (AE 1) 1.2.3.4 maps to the router-ID 0:0:0:0:1:2:3:4, and the zero-length address (AE 0) maps to 0:0:0:0:0:0:0:0. However, I don't think this is spelled out in RFC 6126.

Well, I've always thought about 0x40 as specifying that the router ID be the 64 bits from the address that is semantically encoded by the TLV, not the literal bytes in the TLV itself. I.e. Bird does this: if (tlv->flags & BABEL_FLAG_ROUTER_ID) { state->router_id = ((u64) _I2(msg->prefix)) << 32 | _I3(msg->prefix); state->router_id_seen = 1; } where msg is the internal data structure containing the parsed values. This means there's no problem in combining flag 0x40 with AE 0; but for IPv4 addresses it needs to be specified whether the addresses should be padded to the right or the left.

So my current thinking is:

- if a Babel speaker receives an update with AE 0 or 1 and bit 0x40 set, it MUST set the router-ID to the address in the update, right justified and padded with zeroes;

Yes, this seems reasonable, and should go into a -bis I guess.

- a Babel speaker SHOULD NOT set bit 0x40 in updates with AE 0 or 1, lest the author meet the wrath of Markus.

This is "for the time being", or? Surely Markus can be appeased by the time a new draft is written? ;) -Toke

On Sat, Apr 30, 2016 at 03:15:52PM +0200, Toke Høiland-Jørgensen wrote:

Baptiste Jonglez <baptiste@bitsofnetworks.org> writes:

While I'm not sure what the purpose of this is (a null update with a null router ID with infinity metric and interval?) it *is* technically in spec. I think the reason why Bird complains is that Ondrej's cleanup of my (admittedly messy) packet parsing code inadvertently moved the check for the 0x40 flag inside the case branch for AE_IP6.

Hi Well, the move was not inadvertent. My understanding of RFC 6126: o if the bit with value 40 hexadecimal is set, then the low-order 8 octets of the advertised prefix establish a new default router-id for this TLV and subsequent Update TLVs in the same packet. is that such flag clearly cannot be used with address families with prefix lengths shorter than 8 octets. Also now it seems to me that even the current code is not valid as it implicitly assumes that the prefix length is 128 with this flag. As the spec says 'low-order 8 octets of the advertised prefix' then if one advertise 2001:DB8:1020:3040:5060:7080::/96, then low-order 8 octets of this *prefix* are 1020:3040:5060:7080 and not 5060:7080:0000:0000. It seems that when used outside of the scope of obvious application (full IPv6 address), this flag is not really well specified (and does not make much sense). -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

On Sat, Apr 30, 2016 at 03:15:52PM +0200, Toke Høiland-Jørgensen wrote:

...

=== Hello interval range checking ===

The hello interval cannot be set below 1 second: the parser seems to expect an integer. The packet format encodes intervals in centiseconds, so it would make sense to allow any fractional Hello interval down to 0.01 seconds.

I just checked, babeld's parser allows to go as low as 0.001s (which is a bit scary in itself, since it generates about 1 kpps / 1.5 Mbps of control traffic).

Hmm, yes. However, since the internal Bird timers run at a granularity of seconds only there's not much point in having the ability to configure smaller values.

...

On a related note, there is no overflow check on the hello interval. For instance, setting the hello interval to 800 seconds produces this in tcpdump:

Hello seqno 1 interval 144.64s

Right, yeah, the overflow check is done on the specified values not their centisecond equivalents. Oops.

IMHO the reasonable way would be to keep such values (intervals) internally in Babel native time units (centiseconds) and only convert to BIRD units when appropriate (when mixed with bird_clock_t values). -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

Do you have plans in near feature create ipv4 support?in this case I can drop bird and use only bird6 for ipv4/ipv6 routes inside my network. 1 Май 2016 г. 16:14 пользователь "Toke Høiland-Jørgensen" <toke@toke.dk> написал:

Ondrej Zajicek <santiago@crfreenet.org> writes:

...

...

Right, yeah, the overflow check is done on the specified values not their centisecond equivalents. Oops.

IMHO the reasonable way would be to keep such values (intervals) internally in Babel native time units (centiseconds) and only convert to BIRD units when appropriate (when mixed with bird_clock_t values).

That would work. Also thought about using the expr_us mechanism in the config parser code. However, both of those would break existing configurations (not that there are many at this point; but the principle remains). What is your policy on that?

Also, since we can't actually schedule things at a granularity finer than seconds, setting a lower value would just result in announcing deadlines we then cannot meet, which is bound to break things. So for the time being I think keeping it the way it is would be best, no?

-Toke

On Sat, Apr 30, 2016 at 01:20:14AM +0200, Toke Høiland-Jørgensen wrote:

...

Hi

I would like to thank Toke Høiland-Jørgensen for the Babel protocol implementation, which was finally merged despite my tardy code reviews.

Yay, awesome! You're very welcome, and congratulations on the release! :)

Hi Some comments to my changes. I originally thought that i would just do some minor formatting changes, but i end with some heavy code changes and plenty of bugfixes. Hopefully i did not make much more errors. Some issues i found and fixed (and noted down): - modified ge_mod64k() still does not work: static inline u16 ge_mod64k(u16 a, u16 b) { return (a == b || b - a > 0x8000); } Due to C integer promotion rules, b - a is computed in (signed) integers and not in u16s, so if b = 1 and a = 2, you get -1, so > is false. The solution is to use uint as the type for a, b, so it is computed in unsigned integers and result can be safely typecasted to u16: static inline int ge_mod64k(uint a, uint b) { return (u16)(a - b) < 0x8000; } - in some cases (e.g. babel_handle_route_request()), check for plen == 0 or check if prefix is IPA_NONE is used where check for AE 0 should be used instead. Note that these are two different cases - there is the default route ::/0 (with AE 2), which is a regular route, and there is 'wildcard' AE 0, which represent all routes. - p->entry_slab allocated but not used - struct babel_tlv_header - should also be packed (AFAIK there are some archs that has structures padded to alignment of at least int). - babel_get_attr() - router-id is printed from a->u.data, but it is stored in a->u.ptr->data instead (one more indirection) - babel_process_packet() - check for framing error accesses tlv->length without checking that it can be accessed (i.e., if there is only one remaining byte, the tlv->length is out of bounds). - babel_process_packet() - Pad1 TLV is not correctly handled. - babel_process_packet() - memory leak in sl_alloc() / sl_free() for 'cur' variable - if the TLV parsing loop ends correctly, it is not freed. - babel_send_unicast() - if there is a packet waiting in the socket and another TLVs in the queue, the packet is overwritten and TX sequence for multicast packets in queue is interrupted. - log(L_ERR, ...) should be usually either log(L_REMOTE, ...), or in some cases TRACE(). - babel_read_router_id(), babel_read_next_hop() changed to PARSE_IGNORE as these functions just modify the state and do not generate parsed data. - babel_read_router_id() - endianity problem for router_id (the value is read directly without using get_u64()). - babel_read_ihu() - supposes that addr field is zeroed, but nobody zeroes it. - babel_read_route_request() - non-wildcard may have plen 0 (route ::/0) - babel_read_update() - may read after buffer if BABEL_AE_IP6_LL and omitted are set - babel_read_update() - does BABEL_AE_IP6_LL make sense? - babel_write_queue() - there is a queue argument, but the function processes ifa->tlv_queue anyways. So i guess unicast messages did not work. Also the queue argument uses call by value, should use call by reference. - babel_handle_update() - in 'if (msg->router_id == s->router_id) return;' should be r->router_id instead of s->router_id. (as the source and msg have always the same router-id value). Feel free to comment any of my changes. I found also some problems that were hard to fix and not severe, mainly related to route recalculation and triggered updates. These were left in the code. I will comment them later. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

If this is not the case, I think the RFC needs to specify what, exactly, is meant by a "wildcard address". I've always thought of ::/0 as the wildcard address; and doesn't "default route" also mean "wildcard route"?

Wow, no. The main purpose of a routing protocol is to carry prefixes. Now prefixes do represent sets of addresses, but that's none of the routing protocol's business -- from the point of view of the protocol, they are just opaque constructs. For example, a request for 2000::/3 requests an update for 2000::/3. If a router has routers for 2000::/4 and 3000::/4 but no route for 2000::/3, it must not use the longer prefixes to satisfy a request for 2000::/3 -- from the router's point of view, there's no relation between the prefixes. AE 0 is used to mean "any" in the following circumstances: - IHU (where it represents "any" interface identifier); - non-seqno request (where it represents "any" prefix); - retraction (where it represents "any" prefix). It is not allowed in any other place -- in other places, RFC 6126 says that AE MUST NOT be 0. (There's an omission in Section 4.4.9, where it only says in what case AE MAY be 0; the implication is that it MUST NOT be 0 in other cases.) If we decide to make an incompatible revision of Babel, I might decide to remove AE 0. While it seemed like a good idea at the time, it's turned out to only be moderately useful -- the retraction case can be handled by announcing a Hell interval of 10ms (which causes the neighbour to drop after 0.16s), and the other uses are not useful. -- Juliusz

...

It is not allowed in any other place -- in other places, RFC 6126 says that AE MUST NOT be 0. (There's an omission in Section 4.4.9, where it only says in what case AE MAY be 0; the implication is that it MUST NOT be 0 in other cases.)

Hmm, maybe this should be spelled out somewhere?

Could you please clarify what exactly needs to be spelled out? I have the impression that the meaning of AE 0 is spelled out pretty clearly in all three pleaces where it is allowed. The one missing bit is Updates, where it doesn't state clearly that AE 0 is only allowed for retractions, but I think it's pretty clear from context. -- Juliusz

On Sun, May 01, 2016 at 03:32:58PM +0200, Toke Høiland-Jørgensen wrote:

Ondrej Zajicek <santiago@crfreenet.org> writes:

...

- in some cases (e.g. babel_handle_route_request()), check for plen == 0 or check if prefix is IPA_NONE is used where check for AE 0 should be used instead. Note that these are two different cases - there is the default route ::/0 (with AE 2), which is a regular route, and there is 'wildcard' AE 0, which represent all routes.

This was actually deliberate on my part: I have considered the different AEs as solely a matter of encoding on the wire, and not carrying any significance as to the semantics of the prefix. I.e. and update with AE 0 and no prefix and one with AE 2 encoding ::/0 are identical in my reading.

There is an implied semantics of address family: The address family of an address is either IPv4 or IPv6; it is undefined for AE 0, IPv4 for AE 1, and IPv6 for AE 2 and 3. Obviously, there is a difference between AE 1 encoding 0.0.0.0/0 and AE 2 encoding ::/0, therefore AE 0 cannot be identical to both of them.

If this is not the case, I think the RFC needs to specify what, exactly, is meant by a "wildcard address". I've always thought of ::/0 as the wildcard address; and doesn't "default route" also mean "wildcard route"?

'default route' is just a name for the least specific route, i.e. 0.0.0.0/0 or ::/0, but it is a route like any other. While meaning of AE 0 is more like unspecified/any route. Its exact meaning depends on specific TLV: IHU TLV (address implied/irrelevant): it MAY be 0 if the TLV is sent to a unicast address, if the association is over a point-to-point link, or when bidirectional reachability is ascertained by means outside of the Babel protocol. Update TLV (all routes): AE MAY then be 0, in which case this Update retracts all of the routes previously advertised on this interface. Route Request TLV (all routes): The (AE) value 0 specifies that this is a request for a full routing table dump (a wildcard request)

...

- babel_read_update() - does BABEL_AE_IP6_LL make sense?

Not sure it makes much sense to announce a route for a link-local address, but I suppose it could be used to set a router ID? (Not sure why anyone would do that, but it would not technically be out of spec).

Well, most routing protocols explicitly forbid to propagate link-local routes. So this may be omission in RFC 6126, OTOH it might make sense on ad-hoc wireless networks, where everybody is in one network but there is no full mutual visibily. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

On Sat, Apr 30, 2016 at 09:19:25PM +0200, Ondrej Zajicek wrote:

On Sat, Apr 30, 2016 at 09:24:12AM +0200, Job Snijders wrote:

...

Any hint on the schedule for the next release, which hopefully includes mrt dump file rotation?

I hope we could establish quarterly releases.

Is there a way we can help to convert "hope" into something more tangible? :) Kind regards, Job

It'll be good to run bird under valgrind for testing - I catched some bugs in 1.4.x in that way. 30.04.2016 14:08, Vladimir Lettiev пишет:

On Fri, Apr 29, 2016 at 06:35:33PM +0200, Ondrej Filip wrote:

...

Dear Bird users, the long expected version is out:

Version 1.6.0 (2016-04-29) o Major RIP protocol redesign Looks like the same issue with -D_FORTIFY_SOURCE=2 raised again

http://bird.network.cz/pipermail/bird-users/2015-October/009965.html

In function 'strncpy', inlined from 'rip_fill_authentication.isra.9' at ../../../proto/rip/packets.c:215:5: /usr/include/bits/string3.h:126:10: error: call to __builtin___strncpy_chk will always overflow destination buffer return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); ^ make[1]: *** [packets.o] Error 1

On Mon, May 02, 2016 at 10:44:28AM +0200, Jan Matejka wrote:

...

In function 'strncpy', inlined from 'rip_fill_authentication.isra.9' at ../../../proto/rip/packets.c:215:5: /usr/include/bits/string3.h:126:10: error: call to __builtin___strncpy_chk will always overflow destination buffer return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); ^ make[1]: *** [packets.o] Error 1

Thanks for your report, we will address it, it seems to be a real bug in auth.

Well, IMHO it is not a bug but a false positive, see: http://bird.network.cz/pipermail/bird-users/2015-October/009966.html Although we could use there some less tricky construct. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."

On Mon, May 02, 2016 at 12:18:30PM +0200, Jan Matejka wrote:

...

...

Thanks for your report, we will address it, it seems to be a real bug in auth.

Well, IMHO it is not a bug but a false positive, see:

http://bird.network.cz/pipermail/bird-users/2015-October/009966.html

Although we could use there some less tricky construct.

... This is possibly another place where strict aliasing could fail ...

IMHO no, because aliasing rules has an exception for (char *). -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."