summaryrefslogtreecommitdiffstats
path: root/bgpd/bgp_packet.c
Commit message (Collapse)AuthorAgeFilesLines
* bgpd: Remove the double-pass parsing of NLRIsPaul Jakma2016-03-081-18/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * bgpd parses NLRIs twice, a first pass "sanity check" and then a second pass that changes actual state. For most AFI/SAFIs this is done by bgp_nlri_sanity_check and bgp_nlri_parse, which are almost identical. As the required action on a syntactic error in an NLRI is to NOTIFY and shut down the session, it should be acceptable to just do a one pass parse. There is no need to atomically handle the NLRIs. * bgp_route.h: (bgp_nlri_sanity_check) Delete * bgp_route.c: (bgp_nlri_parse) Make the prefixlen size check more general and don't hard-code AFI/SAFI details, e.g. use prefix_blen library function. Add error logs consistent with bgp_nlri_sanity_check as much as possible. Add a "defense in depth" type check of the prefixlen against the sizeof the (struct prefix) storage - ala bgp_nlri_parse_vpn. Update standards text from draft RFC4271 to the actual RFC4271 text. Extend the semantic consistency test of IPv6. E.g. it should skip mcast NLRIs for unicast safi as v4 does. * bgp_mplsvpn.{c,h}: Delete bgp_nlri_sanity_check_vpn and make bgp_nlri_parse_vpn_body the bgp_nlri_parse_vpn function again. (bgp_nlri_parse_vpn) Remove the notifies. The sanity checks were responsible for this, but bgp_update_receive handles sending NOTIFY generically for bgp_nlri_parse. * bgp_attr.c: (bgp_mp_reach_parse,bgp_mp_unreach_parse) Delete sanity check. NLRI parsing done after attr parsing by bgp_update_receive. Arising out of discussions on the need for two-pass NLRI parse with: Lou Berger <lberger@labn.net> Donald Sharp <sharpd@cumulusnetworks.com>
* bgpd: Regularise bgp_update_receive, add missing notifies and checksPaul Jakma2016-03-081-234/+152
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * bgp_packet.c: (bgp_update_receive) Lots of repeated code, doing same thing for each AFI/SAFI. Except when it doesn't, e.g. the IPv4/VPN case was missing the EoR bgp_clear_stale_route call - the only action really needed for EoR. Make this function a lot more regular, using common, AFI/SAFI independent blocks so far as possible. Replace the 4 separate bgp_nlris with an array, indexed by an enum. The distinct blocks that handle calling bgp_nlri_parse for each different AFI/SAFI can now be replaced with a loop. Transmogrify the nlri SAFI from the SAFI_MPLS_LABELED_VPN code-point used on the wire, to the SAFI_MPLS_VPN safi_t enum we use internally as early as possible. The existing code was not necessarily sending a NOTIFY for NLRI parsing errors, if they arose via bgp_nlri_sanity_check. Send the correct NOTIFY - INVAL_NETWORK for the classic NLRIs and OPT_ATTR_ERR for the MP ones. EoR can now be handled in one block. The existing code seemed broken for EoR recognition in a number of ways: 1. A v4/unicast EoR should be an empty UPDATE. However, it seemed to be treating an UPDATE with attributes, inc. MP REACH/UNREACH, but no classic NLRIs, as a v4/uni EoR. 2. For other AFI/SAFIs, it was treating UPDATEs with no classic withraw and with a zero-length MP withdraw as EoRs. However, that would mean an UPDATE packet _with_ update NLRIs and a 0-len MP withdraw could be classed as an EoR. This seems to be loose coding leading to ambiguous protocol situations and likely incorrect behaviour, rather than simply being liberal. Be more strict about checking that an UPDATE really is an EoR and definitely is not trying to update any NLRIs. This same loose EoR parsing was noted by Chris Hall previously on list. (bgp_nlri_parse) Front end NLRI parse function, to fan-out to the correct parser for the AFI/SAFI. * bgp_route.c: (bgp_nlri_sanity_check) We try convert NLRI safi to internal code-point ASAP, adjust switch for that. Leave the wire code point in for defensive coding. (bgp_nlri_parse) rename to bgp_nlri_parse_ip. * tests/bgp_mp_attr_test.c: Can just use bgp_nlri_parse frontend.
* bgpd: Regularise BGP NLRI sanity checks a bitPaul Jakma2016-03-081-11/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * bgp_route.h: (bgp_nlri_sanity_check) The bulk of the args are equivalent to a (struct bgp_nlri), consolidate. * bgp_route.c: (bgp_nlri_sanity_check) Make this a frontend for all afi/safis. Including SAFI_MPLS_LABELED_VPN. (bgp_nlri_sanity_check_ip) Regular IP NLRI sanity check based on the existing code, and adjusted for (struct bgp_nlri *) arg. * bgp_attr.c: (bgp_mp_reach_parse) Adjust for passing (struct bgp_nlri *) to bgp_nlri_sanity_check. Get rid of special-casing to not sanity check VPN. (bgp_mp_unreach_parse) Ditto. * bgp_mplsvpn.c: Use the same VPN parsing code for both the sanity check and the actual parse. (bgp_nlri_parse_vpn) renamed to bgp_nlri_parse_vpn_body and made internal. (bgp_nlri_parse_vpn_body) Added (bool) argument to control whether it is sanity checking or whether it should update routing state for each NLRI. Send a NOTIFY and reset the session, if there's a parsing error, as bgp_nlri_sanity_check_ip does, and as is required by the RFC. (bgp_nlri_parse_vpn) now a wrapper to call _body with update. (bgp_nlri_sanity_check_vpn) wrapper to call parser without updating. * bgp_mplsvpn.h: (bgp_nlri_sanity_check_vpn) export for bgp_nlri_sanity_check. * bgp_packet.c: (bgp_update_receive) Adjust for bgp_nlri_sanity_check argument changes. * test/bgp_mp_attr_test.c: Extend to also test the NLRI parsing functions, if the initial MP-attr parsing has succeeded. Fix the NLRI in the VPN cases. Add further VPN tests. * tests/bgpd.tests/testbgpmpattr.exp: Add the new test cases. This commit a joint effort of: Lou Berger <lberger@labn.net> Donald Sharp <sharpd@cumulusnetworks.com> Paul Jakma <paul.jakma@hpe.com> / <paul@jakma.org>
* bgpd: make bgp_nlri_parse_encap conform with other nlri_parse funcsPaul Jakma2016-02-261-4/+4
| | | | | | | | | * bgp_encap.{c,h} (bgp_nlri_parse_encap) afi is already in the NLRI argument. update or withdraw is signalled by attr being non-NULL or NULL. * bgp_packet.c: (update_receive) fixup to match, and also make the attr argument conform with NLRI_ATTR_ARG for correct error handling on optional, transitive, partial, attributes.
* bgpd: remove HAVE_IPV6 conditionalsLou Berger2016-02-261-4/+0
| | | | | Signed-off-by: Lou Berger <lberger@labn.net> Tested-by: NetDEF CI System <cisystem@netdef.org>
* bgpd: encap: add encap SAFI (RFC5512)Lou Berger2016-02-261-1/+52
| | | | | | Adds RFC5512 and Encapsulation Attribute. Signed-off-by: Lou Berger <lberger@labn.net>
* bgpd: general MP/SAFI improvementsLou Berger2016-02-261-6/+17
| | | | | | | | This fixes some minor mixups particularly in MPLS-related SAFIs, as well as doing some stylistic changes & adding comments. Signed-off-by: Lou Berger <lberger@labn.net> Reviewed-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: wire up VPNv6 protocol processingLou Berger2016-02-261-4/+28
| | | | | | | | | | | | There wasn't much missing for VPNv6 to begin with; just a few bits of de- & encoding and a few lists to be updated. Signed-off-by: Lou Berger <lberger@labn.net> Signed-off-by: David Lamparter <equinox@opensourcerouting.org> [Editorial note: Signed-off-by may imply an authorship claim, but need not] Edited-by: Paul Jakma <paul.jakma@hpe.com> / <paul@jakma.org>
* lib, bgpd, tests: Refactor FILTER_X in zebra.hDonald Sharp2016-02-261-0/+1
| | | | | | | lib/zebra.h has FILTER_X #define's. These do not belong there. Put them in lib/filter.h where they belong. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
* bgpd: OPEN parse errors should send OPEN_ERR and UNSPECIFIC subcode.Paul Jakma2016-02-101-1/+1
| | | | | | | | | | | | | | | | | | | | | CEASE NOTIFICATION for OPEN parsing errors seems, to my reading of RFC4271 §6.2 to be incorrect. * bgp_packet.c: (bgp_open_receive) OPEN/UNACEP_HOLDTIME is not an appropriate error subcode if bgp_open_option_parse returns an error. Set it to "Unspecific". Where a more specific subcode is appropriate, then lower level should send that. * bgp_open.c: (bgp_open_option_parse) Malformed OPENs should result in NOTIFICATION with OPEN error, and OPEN/UNSPECIFIC sub-code - not CEASE. (bgp_capability_{parse,orf_entry}) ditto. * bgpd.h: Add BGP_NOTIFY_OPEN_UNSPECIFIC for 0. Note that IANA lists 0 as reserved in the OPEN error sub-code registry, but RFC4271 page 32 says 0 is the "Unspecific" OPEN error subcode. Have emailed IANA, they says it's a known errate to 4271 under review. Some inspiration from Cumulus' bgpd-capability-cleanup.patch, though v different result.
* bgpd: check rtt later after the real peer is knownTimo Teräs2015-12-081-1/+1
| | | | | | | | | | | OPEN message handler moves the connection from the temporary "struct peer" (used to accept it) to the real "struct peer" based on the configuration. RTT needs to be updated only to the real struct peer, and this patch moves the RTT query to point where realpeer is known. Fixes: ef757700d0 "bgpd: allow using rtt in route-map's set metric" Signed-off-by: Timo Teräs <timo.teras@iki.fi>
* bgpd: crash from not NULLing freed pointersDaniel Walton2015-12-081-2/+11
| | | | | | | | Signed-off-by: Daniel Walton <dwalton@cumulusnetworks.com> Reviewed-by: Donald Sharp <sharpd@cumulusnetworks.com> There was a crash from not NULLing out peer->hostname but I cleaned up a bunch of other suspect ones as well.
* bgpd: Remove unnecessary stream_dup callsDonald Sharp2015-09-241-26/+6
| | | | | | | | | BGP packet handling creates a stream to handle data for an outgoing packet. In some cases the code would create a new stream, add data, then duplicate the stream and then free the original stream. This change removes the unnecessary duplication, which entails a malloc and memcpy. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
* bgpd: allow using rtt in route-map's set metricTimo Teräs2015-05-201-0/+2
| | | | | | | | | | | | | | | | | | | | | | Useful when the BGP neighbors are over tunnels that have large differences in geographic distances and RTTs. Especially useful for DMVPN setups to allow preferring closes hub. The parameter is added as new alias command as otherwise it seems the command parser is not able to match it properly (it seems merging is done for the various 'set metric' route-map objects in different routing engines). For same reason also they are listed as three separate options: optional +/- seems not possibly easily. Related research papers: http://www.pps.univ-paris-diderot.fr/~jch/research/delay-based.pdf http://arxiv.org/pdf/1309.0632.pdf Paper on similar extension to Babel: http://www.pps.univ-paris-diderot.fr/~jch/research/rapport-jonglez-2013.pdf Signed-off-by: Timo Teräs <timo.teras@iki.fi> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* lib: straighten out ORF prefix list supportDavid Lamparter2015-05-051-3/+3
| | | | | | | | | BGP ORF prefix lists are in a separate namespace; this was previously hooked up with a special-purpose AFI value. This is a little kludgy for extension, hence this splits it off. Signed-off-by: David Lamparter <equinox@opensourcerouting.org> Acked-by: Paul Jakma <paul@jakma.org>
* Fix BGP's use of restart bit.Vipin Kumar2014-11-111-0/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | bgpd-restart-bit-fix.patch ISSUE: Quagga BGP doesn't send or use the restart-bit via the Graceful-Restart(GR) capability. GR capability implementation isn't complete as per the RFC. PATCH: Patch uses BGP instance creation as the beginning of the startup period, and 'restart_time' is taken as the startup period. As a result, BGP will set the restart bit in the GR capability of the OPEN messages during the startup period. As an indication of quagga implementation's capability of sending End-Of-RIB, helping a restarting neighbor, quagga BGP will now send global GR capability irrespective of the graceful-restart config in BGP and the address-family specific GR capability will be sent only if the GR config is present. Forwarding bit is not set assuming its not preserved. Incorporated feedback from David Lamparter via the quagga-dev mailing list. Signed-off-by: Vipin Kumar <vipin@cumulusnetworks.com> Reviewed-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com> Reviewed-by: Paul Jakma <paul@opensourcerouting.org>
* bgpd: well-known attr check only run for v4/uni, which could cause a crash.Paul Jakma2014-10-171-11/+0
| | | | | | | | | | | | | | | | | | | | | | * ANVL testing by Martin Winter threw up a crash in bgpd in aspath_dup called from bgp_packet_attribute, if attr->aspath was NULL, on an IPv6 UPDATE. This root cause is that the checks for well-known, mandatory attributes were being applied only if an UPDATE contained the IPv4 NLRI and the peer was configured for v4/unicast (i.e. not deconfigured). This is something inherited from GNU Zebra, and never noticed before. * bgp_attr.c: (bgp_attr_parse) Move the well-known mandatory attribute check to here, so that it can be run immediately after all attributes are parsed, and before any further processing of attributes that might assume the existence of WK/M attributes (e.g. AS4-Path). (bgp_attr_munge_as4_attrs) Missing AS_PATH shouldn't happen here anymore, but retain a check anyway for robustness - it's definitely a hard error though. * bgp_attr.h: (bgp_attr_check) No longer needs to be exported, make static. * bgp_packet.c: (bgp_update_receive) Responsibility for well-known check now in bgp_attr_parse.
* Fix most compiler warnings in default GCC build.Paul Jakma2014-09-231-15/+12
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix lots of warnings. Some const and type-pun breaks strict-aliasing warnings left but much reduced. * bgp_advertise.h: (struct bgp_advertise_fifo) is functionally identical to (struct fifo), so just use that. Makes it clearer the beginning of (struct bgp_advertise) is compatible with with (struct fifo), which seems to be enough for gcc. Add a BGP_ADV_FIFO_HEAD macro to contain the right cast to try shut up type-punning breaks strict aliasing warnings. * bgp_packet.c: Use BGP_ADV_FIFO_HEAD. (bgp_route_refresh_receive) fix an interesting logic error in (!ok || (ret != BLAH)) where ret is only well-defined if ok. * bgp_vty.c: Peer commands should use bgp_vty_return to set their return. * jhash.{c,h}: Can take const on * args without adding issues & fix warnings. * libospf.h: LSA sequence numbers use the unsigned range of values, and constants need to be set to unsigned, or it causes warnings in ospf6d. * md5.h: signedness of caddr_t is implementation specific, change to an explicit (uint_8 *), fix sign/unsigned comparison warnings. * vty.c: (vty_log_fixed) const on level is well-intentioned, but not going to fly given iov_base. * workqueue.c: ALL_LIST_ELEMENTS_RO tests for null pointer, which is always true for address of static variable. Correct but pointless warning in this case, but use a 2nd pointer to shut it up. * ospf6_route.h: Add a comment about the use of (struct prefix) to stuff 2 different 32 bit IDs into in (struct ospf6_route), and the resulting type-pun strict-alias breakage warnings this causes. Need to use 2 different fields to fix that warning? general: * remove unused variables, other than a few cases where they serve a sufficiently useful documentary purpose (e.g. for code that needs fixing), or they're required dummies. In those cases, try mark them as unused. * Remove dead code that can't be reached. * Quite a few 'no ...' forms of vty commands take arguments, but do not check the argument matches the command being negated. E.g., should 'distance X <prefix>' succeed if previously 'distance Y <prefix>' was set? Or should it be required that the distance match the previously configured distance for the prefix? Ultimately, probably better to be strict about this. However, changing from slack to strict might expose problems in command aliases and tools. * Fix uninitialised use of variables. * Fix sign/unsigned comparison warnings by making signedness of types consistent. * Mark functions as static where their use is restricted to the same compilation unit. * Add required headers * Move constants defined in headers into code. * remove dead, unused functions that have no debug purpose.
* bgpd: fix memory leak on malformed attributeDavid Lamparter2014-06-291-1/+4
| | | | | | | | When bgp_attr_parse returns BGP_ATTR_PARSE_ERROR, it may already have parsed and allocated some attributes before hitting that error. Free the attr's data before returning. Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* *: nuke ^L (page feed)David Lamparter2014-06-041-3/+3
| | | | | | | | | | | | | | Quagga sources have inherited a slew of Page Feed (^L, \xC) characters from ancient history. Among other things, these break patchwork's XML-RPC API because \xC is not a valid character in XML documents. Nuke them from high orbit. Patches can be adapted simply by: sed -e 's%^L%%' -i filename.patch (you can type page feeds in some environments with Ctrl-V Ctrl-L) Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: efficient NLRI packing for AFs != ipv4-unicastPradosh Mohapatra2014-06-031-45/+115
| | | | | | | | | | | | | | | | | | | | | ISSUE: Currently, for non-ipv4-unicast address families where prefixes are encoded in MP_REACH/MP_UNREACH attributes, BGP ends up sending one prefix per UPDATE message. This is quite inefficient. The patch addresses the issue. PATCH: We introduce a scratch buffer in the peer structure that stores the MP_REACH/MP_UNREACH attributes for non-ipv4-unicast families. This enables us to encode multiple prefixes. In the end, the two buffers are merged to create the UPDATE packet. Signed-off-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com> Reviewed-by: Daniel Walton <dwalton@cumulusnetworks.com> [DL: removed no longer existing bgp_packet_withdraw prototype] Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: write NOTIFY non-blockinglyDavid Lamparter2013-07-311-4/+5
| | | | | | | | | | | | | | | switching the socket to blocking may well block the entire bgpd process for some time if our peer is overloaded (which may well be the original reason for the NOTIFY) The error handling is slightly different from the previous ML discussion on this; buffer exhaustion isn't technically a fatal TCP error, and we should probably proceed with FSM actions according to a sent NOTIFY (adjusting timers) even if we didn't manage to get the NOTIFY onto the wire. Acked-by: Leonid Rosenboim <lrosenbo@wrs.com> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: uncork/nagle socket when sending BGP NOTIFYLeonid Rosenboim2013-01-161-1/+9
| | | | | | | | | | | | This pushes out the NOTIFY message before closing a connection. Previously, the TCP_CORK bandwidth optimization code caused NOTIFY messages to disappear prior to when the connection is closed. * bgpd/bgp_packet.c: unset CORK, set NODELAY, and replace writen() by more correct write() Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: uncork after each writeStephen Hemminger2013-01-151-4/+5
| | | | | | | | | Keep data flowing, uncork after each BGP_WRITE_PACKET_MAX. This makes TCP send data sooner, since thread may not be scheduled again for a a longish time because of new UPDATE's coming in. Signed-off-by: Stephen Hemminger <shemminger@vyatta.org> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: use recent monotonic time for readtimeStephen Hemminger2013-01-151-2/+11
| | | | | | | | The readtime value is for diagnostic, and doesn't have to be highly accurate. This also fixes a problem where the readtime was being measured with system clock, but the peer_uptime() was comparing with bgp_clock. Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: fix error response to invalid BGP version numberLeonid Rosenboim2013-01-141-2/+4
| | | | | | | | | | | BGP4-ANVL 20.1 ANVL tries to open BGP with version 5 and expects correct notification in response. Quagga sends notification, but with incorrect information in it. The data needs to be a 2-byte value, and for now we respond with 0004 for any peer version other than 4. Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: Fix for commit 6a4677b7, fixes BZ#729Jorge Boncompte [DTI2]2012-06-211-1/+2
| | | | | | | | | | | The timers are rearmed after events processing. After 6a4677b7 we do not generate events that can rearm the holdtime timer. Fix it's to call bgp_timer_set() directly as it's done from bgp_event(). Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net> Tested-by: Martin Winter <mwinter@opensourcerouting.org> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: optimize holdtime timer cancellingJorge Boncompte [DTI2]2012-05-221-2/+2
| | | | | | | | | | | | | * bgp_packet.c: (bgp_update_receive) for every update received we queue an event just to cancel the holdtime timer, done in bgp_fsm_update(). Instead cancel the timer directly an avoid a scheduling pass. This incidently fixes another problem found on a slow box, where thousands of events threads were queued, and run, but never freed, because they are moved to the unused list that grows without bounds. Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: use on stack struct attr_extra in bgp_update_receive()Jorge Boncompte [DTI2]2012-05-221-7/+4
| | | | | | | Reduce memory heap fragmentation and pressure on the memory allocator. Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: debug buffers cleanup and optimizationJorge Boncompte [DTI2]2012-05-221-28/+44
| | | | | | | | | | | | | | | Just the first change pushes bgp_update_receive() from 6th to ~14th on a full internet table load profiling session. * bgp_debug.c: (bgp_update_receive) The attrstr initialization is expensive, moved under the debug conditional where it is used and just initialize the first char to NULL. (bgp_update_default_send) Initialize attrstr needed for bgp_dump_attr(). Moved some buffers used for printing IP[4|6] addresses under the debug conditionals that use them and reduced its size. Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net> Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgpd: fix issue in capability negotiation (BZ#700)Avneesh Sachdev2012-04-081-5/+10
| | | | | | | | | | | | | | | | | | Address problem where bgpd would reject a session if a peer sent some capabilities in its Open message, but did not include a Multiprotocol extensions capability. Note that the session would come up if there were no capabilities at all in the Open message. * Add the 'mp_capability' out parameter to bgp_capability_parse(). Set it to '1' if a Multiprotocol extensions capability is encountered. * Switch on 'mp_capability' instead of 'capability' in the calling functions to determine if the peer indicated the set of AFI/SAFIs it supports. The net result is that when a peer does not send an MP capability, it is assumed to support the AFI/SAFIs configured for it locally.
* bgpd: Open option parse errors don't NOTIFY, resulting in abort & DoSPaul Jakma2012-03-041-3/+7
| | | | | | | | | | | | | | | | | | * bgp_packet.c: (bgp_open_receive) Errors from bgp_open_option_parse are detected, and the code will stop processing the OPEN and return. However it does so without calling bgp_notify_send to send a NOTIFY - which means the peer FSM doesn't get stopped, and bgp_read will be called again later. Because it returns, it doesn't go through the code near the end of the function that removes the current message from the peer input streaam. Thus the next call to bgp_read will try to parse a half-parsed stream as if it were a new BGP message, leading to an assert later in the code when it tries to read stuff that isn't there. Add the required call to bgp_notify_send before returning. * bgp_open.c: (bgp_capability_as4) Be a bit stricter, check the length field corresponds to the only value it can be, which is the amount we're going to read off the stream. And make sure the capability flag gets set, so callers can know this capability was read, regardless. (peek_for_as4_capability) Let bgp_capability_as4 do the length check.
* bgpd: justify checks for IPv4 class D/EDenis Ovsienko2012-01-021-1/+1
| | | | | | | | | | * lib/prefix.h * IPV4_CLASS_DE(): make consistent with counterpart macros * bgp_packet.c * bgp_open_receive(): test using macro instead of ">=" * bgp_route.c * bgp_update_rsclient(): idem * bgp_update_main(): idem
* bgpd: fix regression in ORF procesing (BZ#688)Denis Ovsienko2011-12-131-1/+1
| | | | | | | | This issue has been pointed out by Lou Berger and Tim Browski. * bgp_packet.c * bgp_route_refresh_receive(): restore if() condition, which was broken by commit fdbc8e77c88f751924299d0bc752371d5cc31116
* bgpd: more SAFI fixesDenis Ovsienko2011-09-291-7/+7
| | | | | | | | | | | | | | | | | | | | | (with resolved conflict in bgpd/bgp_packet.c) Two macros resolving to the same integer constant broke a case block and a more thorough merge of BGP_SAFI_VPNV4 and BGP_SAFI_VPNV6 was performed. * bgpd.h: MPLS-labeled VPN SAFI is AFI-independent, switch to single * macro * bgp_capability_test.c: update test data * bgp_mp_attr_test.c: idem * bgp_route.c: (bgp_maximum_prefix_overflow, bgp_table_stats_vty) update macro and check conditions (where appropriate) * bgp_packet.c: (bgp_route_refresh_send, bgp_capability_send, bgp_update_receive, bgp_route_refresh_receive): idem * bgp_open.c: (bgp_capability_vty_out, bgp_afi_safi_valid_indices, bgp_open_capability_orf, bgp_open_capability): idem * bgp_attr.c: (bgp_mp_reach_parse, bgp_packet_attribute, bgp_packet_withdraw): idem
* bgpd: spellingDenis Ovsienko2011-09-271-1/+1
|
* bgpd: add useful notification logs (BZ#616)heasley2011-09-271-3/+16
| | | | | * bgp_packet.c * bgp_notify_send_with_data(): add calls to zlog_info()
* Merge branch 'attr-errors'Paul Jakma2011-07-291-29/+49
|\ | | | | | | | | | | | | | | Contains BGP fixes: - set extcommunity crash: tihs patch tries to make the refcounting more robust but does not fully solve the problem, sadly. - BGP attribute error handling: Little testing.
| * bgpd: Implement revised error handling for partial optional/trans. attributesPaul Jakma2011-03-211-28/+48
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | * BGP error handling generally boils down to "reset session". This was fine when all BGP speakers pretty much understood all BGP messages. However the increasing deployment of new attribute types has shown this approach to cause problems, in particular where a new attribute type is "tunneled" over some speakers which do not understand it, and then arrives at a speaker which does but considers it malformed (e.g. corruption along the way, or because of early implementation bugs/interop issues). To mitigate this drafts before the IDR (likely to be adopted) propose to treat errors in partial (i.e. not understood by neighbour), optional transitive attributes, when received from eBGP peers, as withdrawing only the NLRIs in the affected UPDATE, rather than causing the entire session to be reset. See: http://tools.ietf.org/html/draft-scudder-idr-optional-transitive * bgp_aspath.c: (assegments_parse) Replace the "NULL means valid, 0-length OR an error" return value with an error code - instead taking pointer to result structure as arg. (aspath_parse) adjust to suit previous change, but here NULL really does mean error in the external interface. * bgp_attr.h (bgp_attr_parse) use an explictly typed and enumerated value to indicate return result. (bgp_attr_unintern_sub) cleans up just the members of an attr, but not the attr itself, for benefit of those who use a stack-local attr. * bgp_attr.c: (bgp_attr_unintern_sub) split out from bgp_attr_unintern (bgp_attr_unintern) as previous. (bgp_attr_malformed) helper function to centralise decisions on how to handle errors in attributes. (bgp_attr_{aspathlimit,origin,etc..}) Use bgp_attr_malformed. (bgp_attr_aspathlimit) Subcode for error specifc to this attr should be BGP_NOTIFY_UPDATE_OPT_ATTR_ERR. (bgp_attr_as4_path) be more rigorous about checks, ala bgp_attr_as_path. (bgp_attr_parse) Adjust to deal with the additional error level that bgp_attr_ parsers can raise, and also similarly return appropriate error back up to (bgp_update_receive). Try to avoid leaking as4_path. * bgp_packet.c: (bgp_update_receive) Adjust to deal with BGP_ATTR_PARSE_WITHDRAW error level from bgp_attr_parse, which should lead to a withdraw, by making the attribute parameter in call to (bgp_nlri_parse) conditional on the error, so the update case morphs also into a withdraw. Use bgp_attr_unintern_sub from above, instead of doing this itself. Fix error case returns which were not calling bgp_attr_unintern_sub and probably leaking memory. * tests/aspath_test.c: Fix to work for null return with bad segments
| * bgpd: Try fix extcommunity resource allocation probs, particularly with 'set ↵Paul Jakma2011-03-211-4/+4
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | extcom..' * Extended communities has some kind of resource allocation problem which causes a double-free if the 'set extcommunity ...' command is used. Try fix by properly interning extcommunities. Also, more generally, make unintern functions take a double pointer so they can NULL out callers references - a usefully defensive programming pattern for functions which make refs invalid. Sadly, this patch doesn't fix the problem entirely - crashes still occur on session clear. * bgp_ecommunity.h: (ecommunity_{free,unintern}) take double pointer args. * bgp_community.h: (community_unintern) ditto * bgp_attr.h: (bgp_attr_intern) ditto * bgp_aspath.h: (bgp_aspath.h) ditto * (general) update all callers of above * bgp_routemap.c: (route_set_ecommunity_{rt,soo}) intern the new extcom added to the attr, and unintern any old one. (route_set_ecommunity_{rt,soo}_compile) intern the extcom to be used for the route-map set. (route_set_ecommunity_*_free) unintern to match, instead of free (route_set_ecommunity_soo) Do as _rt does and don't just leak any pre-existing community, add to it (is additive right though?)
* | bgpd: trivial format / warning fixesPaul Jakma2011-04-131-10/+11
| | | | | | | | | | | | | | * bgp_aspath.c: (assegments_parse) just bail early if length doesn't match and fix the formatting. * bgp_network.c: add include needed for set_nonblocking * bgp_packet.c: formatting
* | bgpd: enable TCP corkingStephen Hemminger2011-03-211-14/+18
| | | | | | | | | | | | | | * bgp_packet.c: (bgp_write) On BGP write, use TCP_CORK to provide hints to kernel about TCP buffering. This will cause BGP packets to occur in bigger chunks (full size MTU), improving performance and getting rid of one of the problems reported in the UNH BGP conformance test.
* | bgpd: leave peer socket in non-blocking mode (mostly)Stephen Hemminger2011-03-211-23/+14
|/ | | | | | | * bgpd: Rather than toggling socket in/out of non-block mode, just leave it in nonblocking mode. One exception is in bgp_notify which only happens just before close.
* bgpd: fix handling of "Unsupported Capability"Dmitrij Tejblum2011-01-141-6/+0
| | | | | | | | | | | | | | | | | | | | | | | | * bgp_packet.c: (bgp_notify_receive) justify the difference between BGP_NOTIFY_OPEN_UNSUP_PARAM and BGP_NOTIFY_OPEN_UNSUP_CAPBL cases, as it is explained in RFC5492, page 3, paragraph 1. "Unsupported Capability" error does not mean, that the peer doesn't support capabilities advertisement -- quite the opposite (if the peer would not support capabilities advertisement, the code would be "Unsupported Optional Parameter"). Thus there is no reason to mark the peer as one non-supporting capabilities advertisement. Example: suppose the peer is in fact IPv6-only, but we didn't configure anything address-family specific for it. Then, the peer would refuse the session with "Unsupported Capability" code. If we internally set the peer as non-supporting capabilities advertisement after that, we will not be able to establish the session with it ever, even with a fixed configuration -- IPv6-only BGP session cannot be established without capabilities. In practice an edge case would be seen as the same IPv6 peer working with its "neighbor" block read from bgpd.conf, but not working, when slowly input in "conf t" mode.
* Set from even if binfo->extra is NULL.Greg Troxel2010-09-171-2/+3
| | | | | | | | | | bgpd/bgp_packet.c:bgp_update_packet(): When extracting the peer, don't fail to extract it because "binfo->extra" is NULL. While one should certainly avoid dereferencing binfo->extra, that's not a good reason not to use binfo->peer. Fixes https://bugzilla.quagga.net/show_bug.cgi?id=497. Patch by Eric Sobocinksi.
* bgpd: tighten bounds checking in RR ORF msg readerChris Hall2010-05-141-9/+40
| | | | | | * bgp_packet.c: (bgp_route_refresh_receive) add validation of "Length" (RFC5292) field value, check input stream bounds each time bytes are pulled from it
* bgp: use monotonic clock for time of dayStephen Hemminger2010-01-151-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | BGP uses time() to get system time of day; but that value fluctuates with time adjustments from NTP. This can cause premature flapping of peer sessions and other failures. Use the system monotonic clock supported by Quagga thread library to avoid issue. See: http://bugzilla.vyatta.com/show_bug.cgi?id=4467 * bgpd/bgp_fsm.c * bgp_uptime_reset(): dismiss function * bgpd/bgpd.c * bgp_clock(): new function * bgpd/bgp_damp.c * bgp_reuse_timer(): employ bgp_clock() instead of time(NULL) * bgp_damp_withdraw(): idem * bgp_damp_update(): idem * bgp_damp_scan(): idem * bgp_damp_info_vty(): idem * bgp_damp_reuse_time_vty(): idem * bgpd/bgp_fsm.c * bgp_routeadv_timer(): idem * bgp_stop(): idem * bgp_establish(): idem * bgpd/bgp_packet.c * bgp_update_receive(): idem * bgpd/bgp_route.c * bgp_update_rsclient(): idem * bgp_update_main(): idem * bgp_static_update_rsclient(): idem * bgp_static_update_main(): idem * bgp_static_update_vpnv4(): idem * bgp_aggregate_route(): idem * bgp_aggregate_add(): idem * bgp_redistribute_add(): idem * bgpd/bgp_snmp.c * bgpPeerTable(): idem * bgpTrapEstablished(): idem * bgpTrapBackwardTransition(): idem * bgpd/bgpd.c * peer_create(): idem * peer_uptime(): idem * bgp_master_init(): idem
* bgpd: compile warnings cleanupStephen Hemminger2009-12-101-24/+0
| | | | | | | | | | | | * bgpd/bgp_fsm.c * bgp_clearing_completed(): only used in one file, can be static * bgpd/bgp_packet.c * afi2str(): sayonara * safi2str(): sayonara * bgpd/bgp_route.c * bgp_distance_reset(): sayonara * bgpd/bgp_zebra.c * bgp_ifindex_by_nexthop(): sayonara
* [bgpd] Stability fixes including bugs 397, 492Chris Caputo2009-07-191-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | I've spent the last several weeks working on stability fixes to bgpd. These patches fix all of the numerous crashes, assertion failures, memory leaks and memory stomping I could find. Valgrind was used extensively. Added new function bgp_exit() to help catch problems. If "debug bgp" is configured and bgpd exits with status of 0, statistics on remaining lib/memory.c allocations are printed to stderr. It is my hope that other developers will use this to stay on top of memory issues. Example questionable exit: bgpd: memstats: Current memory utilization in module LIB: bgpd: memstats: Link List : 6 bgpd: memstats: Link Node : 5 bgpd: memstats: Hash : 8 bgpd: memstats: Hash Bucket : 2 bgpd: memstats: Hash Index : 8 bgpd: memstats: Work queue : 3 bgpd: memstats: Work queue item : 2 bgpd: memstats: Work queue name string : 3 bgpd: memstats: Current memory utilization in module BGP: bgpd: memstats: BGP instance : 1 bgpd: memstats: BGP peer : 1 bgpd: memstats: BGP peer hostname : 1 bgpd: memstats: BGP attribute : 1 bgpd: memstats: BGP extra attributes : 1 bgpd: memstats: BGP aspath : 1 bgpd: memstats: BGP aspath str : 1 bgpd: memstats: BGP table : 24 bgpd: memstats: BGP node : 1 bgpd: memstats: BGP route : 1 bgpd: memstats: BGP synchronise : 8 bgpd: memstats: BGP Process queue : 1 bgpd: memstats: BGP node clear queue : 1 bgpd: memstats: NOTE: If configuration exists, utilization may be expected. Example clean exit: bgpd: memstats: No remaining tracked memory utilization. This patch fixes bug #397: "Invalid free in bgp_announce_check()". This patch fixes bug #492: "SIGBUS in bgpd/bgp_route.c: bgp_clear_route_node()". My apologies for not separating out these changes into individual patches. The complexity of doing so boggled what is left of my brain. I hope this is all still useful to the community. This code has been production tested, in non-route-server-client mode, on a linux 32-bit box and a 64-bit box. Release/reset functions, used by bgp_exit(), added to: bgpd/bgp_attr.c,h bgpd/bgp_community.c,h bgpd/bgp_dump.c,h bgpd/bgp_ecommunity.c,h bgpd/bgp_filter.c,h bgpd/bgp_nexthop.c,h bgpd/bgp_route.c,h lib/routemap.c,h File by file analysis: * bgpd/bgp_aspath.c: Prevent re-use of ashash after it is released. * bgpd/bgp_attr.c: #if removed uncalled cluster_dup(). * bgpd/bgp_clist.c,h: Allow community_list_terminate() to be called from bgp_exit(). * bgpd/bgp_filter.c: Fix aslist->name use without allocation check, and also fix memory leak. * bgpd/bgp_main.c: Created bgp_exit() exit routine. This function frees allocations made as part of bgpd initialization and, to some extent, configuration. If "debug bgp" is configured, memory stats are printed as described above. * bgpd/bgp_nexthop.c: zclient_new() already allocates stream for ibuf/obuf, so bgp_scan_init() shouldn't do it too. Also, made it so zlookup is global so bgp_exit() can use it. * bgpd/bgp_packet.c: bgp_capability_msg_parse() call to bgp_clear_route() adjusted to use new BGP_CLEAR_ROUTE_NORMAL flag. * bgpd/bgp_route.h: Correct reference counter "lock" to be signed. bgp_clear_route() now accepts a bgp_clear_route_type of either BGP_CLEAR_ROUTE_NORMAL or BGP_CLEAR_ROUTE_MY_RSCLIENT. * bgpd/bgp_route.c: - bgp_process_rsclient(): attr was being zero'ed and then bgp_attr_extra_free() was being called with it, even though it was never filled with valid data. - bgp_process_rsclient(): Make sure rsclient->group is not NULL before use. - bgp_processq_del(): Add call to bgp_table_unlock(). - bgp_process(): Add call to bgp_table_lock(). - bgp_update_rsclient(): memset clearing of new_attr not needed since declarationw with "= { 0 }" does it. memset was already commented out. - bgp_update_rsclient(): Fix screwed up misleading indentation. - bgp_withdraw_rsclient(): Fix screwed up misleading indentation. - bgp_clear_route_node(): Support BGP_CLEAR_ROUTE_MY_RSCLIENT. - bgp_clear_node_queue_del(): Add call to bgp_table_unlock() and also free struct bgp_clear_node_queue used for work item. - bgp_clear_node_complete(): Do peer_unlock() after BGP_EVENT_ADD() in case peer is released by peer_unlock() call. - bgp_clear_route_table(): Support BGP_CLEAR_ROUTE_MY_RSCLIENT. Use struct bgp_clear_node_queue to supply data to worker. Add call to bgp_table_lock(). - bgp_clear_route(): Add support for BGP_CLEAR_ROUTE_NORMAL or BGP_CLEAR_ROUTE_MY_RSCLIENT. - bgp_clear_route_all(): Use BGP_CLEAR_ROUTE_NORMAL. Bug 397 fixes: - bgp_default_originate() - bgp_announce_table() * bgpd/bgp_table.h: - struct bgp_table: Added reference count. Changed type of owner to be "struct peer *" rather than "void *". - struct bgp_node: Correct reference counter "lock" to be signed. * bgpd/bgp_table.c: - Added bgp_table reference counting. - bgp_table_free(): Fixed cleanup code. Call peer_unlock() on owner if set. - bgp_unlock_node(): Added assertion. - bgp_node_get(): Added call to bgp_lock_node() to code path that it was missing from. * bgpd/bgp_vty.c: - peer_rsclient_set_vty(): Call peer_lock() as part of peer assignment to owner. Handle failure gracefully. - peer_rsclient_unset_vty(): Add call to bgp_clear_route() with BGP_CLEAR_ROUTE_MY_RSCLIENT purpose. * bgpd/bgp_zebra.c: Made it so zclient is global so bgp_exit() can use it. * bgpd/bgpd.c: - peer_lock(): Allow to be called when status is "Deleted". - peer_deactivate(): Supply BGP_CLEAR_ROUTE_NORMAL purpose to bgp_clear_route() call. - peer_delete(): Common variable listnode pn. Fix bug in which rsclient was only dealt with if not part of a peer group. Call bgp_clear_route() for rsclient, if appropriate, and do so with BGP_CLEAR_ROUTE_MY_RSCLIENT purpose. - peer_group_get(): Use XSTRDUP() instead of strdup() for conf->host. - peer_group_bind(): Call bgp_clear_route() for rsclient, and do so with BGP_CLEAR_ROUTE_MY_RSCLIENT purpose. - bgp_create(): Use XSTRDUP() instead of strdup() for peer_self->host. - bgp_delete(): Delete peers before groups, rather than after. And then rather than deleting rsclients, verify that there are none at this point. - bgp_unlock(): Add assertion. - bgp_free(): Call bgp_table_finish() rather than doing XFREE() itself. * lib/command.c,h: Compiler warning fixes. Add cmd_terminate(). Fixed massive leak in install_element() in which cmd_make_descvec() was being called more than once for the same cmd->strvec/string/doc. * lib/log.c: Make closezlog() check fp before calling fclose(). * lib/memory.c: Catch when alloc count goes negative by using signed counts. Correct #endif comment. Add log_memstats_stderr(). * lib/memory.h: Add log_memstats_stderr(). * lib/thread.c: thread->funcname was being accessed in thread_call() after it had been freed. Rearranged things so that thread_call() frees funcname. Also made it so thread_master_free() cleans up cpu_record. * lib/vty.c,h: Use global command_cr. Add vty_terminate(). * lib/zclient.c,h: Re-enable zclient_free().
* [bgpd] add comment that bgp_capability_receive is exported for unit-testsPaul Jakma2009-06-181-1/+4
|
generated by cgit v1.2.3 (git 2.25.1) at 2025-08-02 12:48:41 +0000