bgpd: flowspec: remove sizelimit check applied to the wrong length field (issue 18557)

[spoignant-proton]

See issue 18557 for detailed description of the problem. When announcing flowspec routes, frr currently sends NLRIs up to max_packet_size. However, the maximum size of flowspec NLRIs is limited to a much lower value here.

Because of this, past as certain amount of flowspec routes, the peer will drop the session.

The proposed change reduces the size of the buffer for the NLRI to the maximum value between nlri_max_length and either FLOWSPEC_NLRI_SIZELIMIT_EXTENDED (if the peer advertised support for extended messages) or FLOWSPEC_NLRI_SIZELIMIT (if it did not).

Needs further review and testing.

[mwinter-osr]

CI:rerun Rerunning the CI after fix on "[CI] Verify Source" incorrectly reporting bad status

[RodrigoNardi]

CI:rerun Rerunning the CI after fix on "[CI] Verify Source" incorrectly reporting bad status

[mwinter-osr]

CI:rerun Rerunning the CI after fix on "[CI] Verify Source" incorrectly reporting bad status (again - sorry CI issues)

[ton31337]

I don't understand why we should limit to FLOWSPEC_NLRI_SIZELIMIT_EXTENDED and not to BGP_EXTENDED_MESSAGE_MAX_PACKET_SIZE?

IMO, this check should be relaxed depending if we support extended message support or not:

	if (packet->length >= FLOWSPEC_NLRI_SIZELIMIT_EXTENDED) { <-------------
		flog_err(EC_BGP_FLOWSPEC_PACKET,
			 "BGP flowspec nlri length maximum reached (%u)",
			 packet->length);
		return BGP_NLRI_PARSE_ERROR_FLOWSPEC_NLRI_SIZELIMIT;
	}

[spoignant-proton]

I don't know the rationale for this part of the code, but what i did observe, is that even with non-FRR peers (nokia sr, junos) the session was flapping when we were exceeding that length.

[ton31337]

Exceeding the limit because it was using 65k which should be fine, but that check hard-coding packet length FLOWSPEC_NLRI_SIZELIMIT_EXTENDED is wrong, IMO. Unless it is defined somewhere in RFCs (before extended message support was introduced).

[ton31337]

@pguibert6WIND any comments?

[spoignant-proton]

@ton31337 did you check this by any chance?
https://datatracker.ietf.org/doc/html/rfc8955#section-4.1

[ton31337]

Thanks point out the actual RFC, that makes sense then. But why do we need to rely on extended message support capability? I'm trying to understand this.

[spoignant-proton]

There might be a confusion between the length field of the MP_REACH_NLRI Path Attribute (which bgp_flowspec.c#L108 is considering) and the length field of the entries in the NLRI field of that attribute (which uses the variable length encoding described in this section of the RFC).

Not sure tho, as it would not explain why the longer messages were not accepted by the nokia and sros peers either. Maybe vendor got it wrong too??? Would be surprising at least.

[ton31337]

Everything is fine, but I don't understand why we need to rely on extended message support? Why can't we do:

if (safi == SAFI_FLOWSPEC)
  subgrp->scratch = stream_new(FLOWSPEC_NLRI_SIZELIMIT_EXTENDED);

[spoignant-proton]

Fair point, as FLOWSPEC_NLRI_SIZELIMIT_EXTENDED is lower than the max length allowed when extended message capability was not negotiated (4095 < 4096).
I'm just checking first whether we can't simply have the check in bgp_flowspec.c pertains to the correct length field in accordance with the RFC. Testing atm and i'll update the PR accordingly.

[spoignant-proton]

@ton31337 following your suggestion, and after confirming on the test bed that the modified version no longer reproduces the issue (all peers remaining table, including non-FRR ones), i've modified the PR accordingly.

Based on my understanding of the RFC, i think you were right pointing that the check in bgp_flowspec.c#L108 is the actual issue. My initial approach would prevent running into the issue with FRR peers, but not other types of peers, and also would reduce efficiency.

Simply put, the 4095 limit defined in the RFC pertains to individual flowspec NLRI records included in the NLRI field of the MP_REACH_NLRI path attribute, whereas the check considers the total length of all NLRIs.

I'm therefore suggesting removing that check entirely.
As a matter of fact, the length field of individual NLRI records do not need to be checked against this value, because the encoding method defined in the RFC makes it impossible for the length to be greater than 4095.
And whether the peer advertised extended message capability is relevant to the total message length, not just the NLRI fields. Since FRR announces extended message capability unconditionally, it should always accept message up to 65535 bytes (per section 4 of RFC8654). And even though we wanted to check the total message length, bgp_flowspec.c isn't probably the place where that should be done.

[spoignant-proton]

not sure if we need to assign -12 to the next in the list or so on, or if it is better to leave codes unchanged not to break compatibility