bgpd: accept extended-size BGP messages on receive (RFC 8654)#22423
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
|
Full packet capture illustrating the issue: https://pkg.rdem-systems.com/ippi-bgp.pcap |
The receive path (validate_header() / read_ibuf_work() in bgp_io.c) caps an incoming message at peer->max_packet_size and, when that is exceeded, drops it with NOTIFICATION Message Header Error / Bad Message Length. peer->max_packet_size is the negotiated *send* limit: it is set to the extended size (65535) only when the Extended Message capability (RFC 8654) was both advertised and received, it is computed once while parsing the peer's OPEN in bgp_open_option_parse(), and it is never refreshed. It is therefore left at the standard size (4096) on any connection where our own EXTENDED_MESSAGE_ADV flag is not yet set when the peer's OPEN is parsed -- notably a passively accepted connection, which sends its own OPEN only from bgp_fsm_open() after the peer's OPEN has been parsed. The value is then preserved across connection-collision resolution by peer_xfer_conn(), so it sticks for the life of the session. RFC 8654 makes the limit asymmetric: a speaker that has advertised the capability MUST be able to *receive* messages up to 65535 octets; the peer's advertisement governs only what we may *send*. FRR advertises the capability unconditionally, so the receive limit must allow extended-size messages regardless of peer->max_packet_size. Two FRR speakers that both advertised and received the capability could thus still reset each other in a loop as soon as one packed an UPDATE larger than 4096 octets (seen with a 7795-octet add-path UPDATE: NOTIFICATION data 0x1E73). Use a dedicated, type-aware receive limit: the extended size for every message except OPEN and KEEPALIVE (which RFC 8654 never extends), leaving peer->max_packet_size as the send / update-group sizing field. Signed-off-by: Richard Demongeot <richard@rdem-systems.com>
1cb5267 to
d2b19a1
Compare
|
Just tested the build, and session works now, while non-upgraded router does not accept the same session. IPPI Router <-> Rdem Systems 1 (patched) : OK |
|
We must fix the problem in the resetting of the peer connection and before we construct a open message. That is the bug. I do not want to refigure the value every time we receive a packet that makes no sense. |
|
Dear @donaldsharp ; Thanks for your comment. Regards, |
|
P.S : Why the OSPF check fails with BGP patch? :/ i don't see any OSPF patch in my patch :( |
|
Let's fix frrbot (styling) yet. |
Commit d2b19a1 ("bgpd: accept extended-size BGP messages on receive") worked around the bug on the receive path by recomputing a type-aware size limit for every incoming packet. As noted in review, that is the wrong place: the limit should be wrong nowhere, so it must be fixed where peer->max_packet_size is established, not re-derived per packet. This reverts the bgp_io.c change and fixes the root cause. peer->max_packet_size is set to the extended size (65535) only when the Extended Message capability (RFC 8654) was both advertised by us (PEER_CAP_EXTENDED_MESSAGE_ADV) and received from the peer (PEER_CAP_EXTENDED_MESSAGE_RCV). It was computed once, while parsing the peer's OPEN in bgp_open_option_parse(). On a passively accepted connection our own OPEN is built (and the ADV flag set) only after the peer's OPEN has been parsed, so at parse time ADV is not yet set and the value is left at the standard size (4096). It is then preserved across connection-collision resolution by peer_xfer_conn() and never refreshed, so two FRR speakers that both support the capability could reset each other in a loop once one sent an UPDATE larger than 4096 octets (seen with a 7795-octet add-path UPDATE). Recompute the limit at every point where its inputs can change: - bgp_open_capability(): when we set the ADV flag while building our OPEN. This is the point a passive connection was previously missing. - bgp_open_option_parse(): when we set the RCV flag (unchanged, now via the shared helper). - bgp_stop() / bgp_start(): when the peer's capabilities are cleared on connection reset, revert to the standard size so no stale extended value carries into a session that does not negotiate the capability. The shared computation lives in bgp_peer_set_max_packet_size(). The receive path in bgp_io.c again simply trusts peer->max_packet_size. Signed-off-by: Richard Demongeot <richard@rdem-systems.com>
…ve (RFC 8654) RFC 8654 makes the Extended Message capability apply to every message type except OPEN and KEEPALIVE, which "continue to use a maximum message size of 4096 octets". Once a session has negotiated the extended size, peer->max_packet_size is 65535, and the generic receive guard in bgp_io.c -- which is deliberately type-blind so it never has to refigure a value per packet -- would then accept an oversized OPEN or KEEPALIVE. Enforce the per-type upper bound in the dedicated receive handlers, where the message type is already known by construction, so no type logic leaks into the bgp_io.c receive path: bgp_open_receive() and bgp_keepalive_receive() now drop any message whose total length exceeds the standard maximum with NOTIFICATION Message Header Error / Bad Message Length. Signed-off-by: Richard Demongeot <richard@rdem-systems.com>
b76d1be to
9431add
Compare
Dear, Style applied. |
|
I'm not an expert in Github, must i create the same merge request for stable/10.5 and stable/10.6 ? |
The receive path (validate_header() / read_ibuf_work() in bgp_io.c) caps an incoming message at peer->max_packet_size and, when exceeded, drops it with NOTIFICATION Message Header Error / Bad Message Length.
peer->max_packet_size is the negotiated send limit: it is set to the extended size (65535) only when the Extended Message capability (RFC 8654) was both advertised and received, it is computed once while parsing the peer's OPEN (bgp_open_option_parse()), and it is never refreshed. If that value is left at the standard size (4096) on a connection -- e.g. our OPEN had not been built yet when the peer's OPEN was parsed, a state then preserved across collision resolution by peer_xfer_conn() -- it sticks for the life of the session.
RFC 8654 makes the limit asymmetric: a speaker that has advertised the capability MUST be able to receive messages up to 65535 octets; the peer's advertisement governs only what we may send. FRR advertises the capability unconditionally, so the receive limit must allow extended-size messages regardless of peer->max_packet_size. Two FRR speakers that both advertised and received the capability could therefore still tear each other down in a loop once one packed an UPDATE larger than 4096 octets (observed with a 7795-octet add-path UPDATE: NOTIFICATION data 0x1E73).
Use a dedicated, type-aware receive limit: the extended size for every message except OPEN and KEEPALIVE (which RFC 8654 never extends), independent of peer->max_packet_size which remains the send/update-group sizing field.