In bgpd/bgp_community_del_val memcpy is used for potentially overlapping
regions which is *not* safe. It may "work" in some cases but is not
guaranteed to work in all cases. The case that I saw fail was on an
x86_64 architecture with the number of bytes being moved/copied equal to
8.
The way the code is written the uint32_t pointers will always differ by
1, which is equivalent to a memcpy/memmove of regions that are 4 bytes
away from one another. So the code failed while copying an 8 byte region
to an address that is 4 bytes lower i.e. overlapping regions.
Interestingly, the same architecture had no problems with a 12 byte
copy.
When the code failed the communities were [200,300,400] and a call was
made to delete the 200 community. The result of this was an array that
looked like [400,400] which was uniquified to [400]. Of course the
expected result should have been [300, 400].
One additional point - in our production environment memmove would not
*link* without including <string.h> but in an isolated quagga git repo
this #include does not seem to be required and I see memmove is used in
vtysh.c without this #include either.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Most of the attribute parsing functions were already sending a notify,
let's clean up the code to make it happen only once.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
The route-map extcommunity set code was incorrectly assuming that it
owns the intern'd struct ecommunity reference. In reality, the intern'd
reference belongs to bgp_update_receive() and we're not supposed to
touch it in the route-map code.
Instead, like all the other set commands, we use a on-heap but
non-intern'd ecommunity to set the new value. This is then either
intern'd in bgp_update_main/_rsclient() through bgp_attr_intern(), or
free'd through bgp_attr_flush().
This fixes Bugzilla #799, which is that bgpd otherwise crashes with a
double free. The ecommunity got unintern'd first in the route-map set
command, then in bgp_update_receive().
Debugged-by: Milan Kocian <milon@wq.cz>
Reported-by: Florian S <florian@herrenlohe.de>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
route_set_ecommunity_rt and _soo share almost all of their code.
Let's remove one of the redundant copies.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
bgp_update_main() wasn't doing anything to release attribute values
set from route maps for two of its error paths. To fix, pull up the
appropriate cleanup from further down and apply it here.
bgp_update_rsclient() doesn't have the issue since it immediately
does bgp_attr_intern() on the results from bgp_{export,import}_modifier.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
When allowas-in is changed on a peer that is not up, BGP would crash
trying to do route_refresh. If peer is not up, there is no need
to do notification or send.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Feng Lu <lu.feng@6wind.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
While announcing a path to a peer, the code currently compares the path's
next-hop with the peer's router-id. This can lead to problems as the router
IDs are unique only within an AS. Suppose AS 1 sends route with next-hop
10.1.1.1. It is possible that the speaker has an established BGP peering
with a router in AS 2 with router ID 10.1.1.1. The route will not be
advertised to that peer in AS 2.
The patch removes this check.
Signed-off-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
Reviewed-by: Dinesh G Dutt <ddutt@cumulusnetworks.com>
Reviewed-by: Shrijeet Mukherjee <shm@cumulusnetworks.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Currently when you set neighbour's 'allowas-in' option on route server side
you get redistribution of the prefixes from this neighbour's table into all
neighbour's tables which have the same AS number. I think that wanted behaviour
is to allow import prefixes from neighbour's tables with the same AS num
into neighbour which has 'allowas-in' option set.
Signed-off-by: Milan Kocian <milon@wq.cz>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Traditionally, ttl-security feature has been associated with EBGP
sessions as those identify directly connected external peers. The
GTSM RFC (rfc 5082) does not make any restrictions on type of
peering. In fact, it is beneficial to support ttl-security for both
EBGP and IBGP sessions. Specifically, in data centers, there are
directly connected IBGP peerings that will benefit from the protection
ttl-security provides.
Signed-off-by: Dinesh G Dutt <ddutt@cumulusnetworks.com>
Reviewed-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
[DL: function refactoring split out into previous 2 patches. changes:
- bgp_set_socket_ttl(): ret type int -> void
- is_ebgp_multihop_configured(): stripped peer == NULL check
- comments/whitespace]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The check for an eBGP multihop configuration is unwieldy; factor it out
into a separate function.
[DL: originally by Dinesh G Dutt <ddutt@cumulusnetworks.com>,
split off from the next commit]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
TTL/min TTL are set from both bgp_accept() and bgp_connect(). Factor
them out so the following change to enable iBGP GTSM becomes more
readable.
[DL: originally by Dinesh G Dutt <ddutt@cumulusnetworks.com>,
split off from the next commit]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
ISSUES
1. When an interface goes down, the zclient callbacks are invoked
in the following order: (a) address_delete() that removes the
connected address list: ifp->connected, (b) interface_down()
that performs "fast external fallover" operation. The operation
relies on ifp->connected to look for peers that should be brought
down. That's a cyclic dependency.
2. 'ttl-security' configuration handler sets peer->ttl to
MAXTTL (so that BGP packets are sent with TTL=255, as per the
requirement of ttl-security). This, however, is incompatible
with 'fast external fallover' as the fallover operation checks
for (ttl == 1) to determine directly connected peers.
3. The current fallover operation does not work for IPv6 address family.
PATCH
1. The patch removes the dependency on 'ifp->connected' list for fast
fallover. The peer already contains a nexthop structure that reflects
the peering address. The nexthop structure has a pointer to the
interface (ifp) that peering address resolves to. Everytime the TCP
connection succeeds, the ifp is updated. The patch uses this ifp in
the interface_down() callback for a match for the peers that should be
brought down.
2. The evaluation for directly connected peering is enhanced as
'peer->ttl == 1' OR 'peer->gtsm_hops == 1'. Thus a ttl-security
configuration on the peer with one hop is directly connected and
should be brought down under 'fast external fallover'.
3. Because of fix (1), IPv6 address family works automatically.
Signed-off-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
Reviewed-by: Dinesh G Dutt <ddutt@cumulusnetworks.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
BGP does not respond fairly in high scale. As the number of BGP peers
and prefixes increase, triggers like interface flaps which lead to BGP
peer flaps, cause blockage in bgp_write.
BGP does handle the cases of TCP socket buffer full by queuing a write
event back, there is no functional issue there as such. Still,
increasing the peer socket buffer size should help reduce event queueing
in BGP.
Signed-off-by: Vipin Kumar <vipin@cumulusnetworks.com>
Reviewed-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
Reviewed-by: Dinesh Dutt <ddutt@cumulusnetworks.com>
[DL: patch split, this is item 3.]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
BGP was setting sockets to be non-blocking only for the accepted passive
peers. As a fix, setting the BGP sockets to be non-blocking even for
the active peers.
Signed-off-by: Vipin Kumar <vipin@cumulusnetworks.com>
Reviewed-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
Reviewed-by: Dinesh Dutt <ddutt@cumulusnetworks.com>
[DL: patch split, this is item 1.]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The issue it fixes is that the notification message is not sent to a
second peer when bgp is stopped manually.
According to BGP RFC4271, section 8.2.2, regarding the FSM transitions,
in OpenSent state:
If a ManualStop event (Event 2) is issued in the OpenSent state, the
local system:
* sends the NOTIFICATION with a Cease,
* sets the ConnectRetryTimer to zero,
* releases all BGP resources,
* drops the TCP connection,
* sets the ConnectRetryCounter to zero, and
* changes its state to Idle.
I've added a check for OpenSent state when the notification is sent from
the functions which are called from the CLI commands which
directly/indirectly stop/restart BGP.
Acked-by: Pradosh Mohapatra <pmohapat@cumulusnetworks.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
The output of "show ip bg" does not show whether and which routes are
installed as multipath routes along the best route:
BGP table version is 0, local router ID is 10.10.100.209
Status codes: s suppressed, d damped, h history, * valid, > best, i - internal,
r RIB-failure, S Stale, R Removed
Origin codes: i - IGP, e - EGP, ? - incomplete
Network Next Hop Metric LocPrf Weight Path
*>i1.0.0.0/24 10.10.100.1 1 111 0 15169 i
* i 10.10.100.2 1 111 0 15169 i
* i 10.10.100.3 1 111 0 65100 15169 i
This patch adds a new status code that is showing exactly which routes
are used as multipath:
BGP table version is 0, local router ID is 10.10.100.209
Status codes: s suppressed, d damped, h history, * valid, > best, = multipath,
i internal, r RIB-failure, S Stale, R Removed
Origin codes: i - IGP, e - EGP, ? - incomplete
Network Next Hop Metric LocPrf Weight Path
*>i1.0.0.0/24 10.10.100.1 1 111 0 15169 i
*=i 10.10.100.2 1 111 0 15169 i
* i 10.10.100.3 1 111 0 65100 15169 i
The inconsistency in the status code legend ("i - internal" vs. "i internal")
inherent from old IOS was fixed. It had to be touched anyways.
Signed-off-by: Boian Bonev <bbonev at ipacct.com>
[DL: rewrap long line, clean whitespace in same chunk]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
ISSUE:
Suppose route1 and route2 received from route-reflector-client1 and client2
respectively have identical attributes. The current logic of creating the
adj-rib-out for a peer threads the 'adv' structures for both routes against
the same attribute. This results in 'bgp_update_packet()' to pack those
routes in the same UPDATE message with one attr structure formatted. The
originator-id is thus set according to the first route's received router id.
This is incorrect.
PATCH:
Fix bgp_announce_check() function to set the originator-id in the
advertising attr structure. Also, fix the attribute hash function and
compare function to consider originator-id. Otherwise attributes where all
fields except the originator-id are identical get merged into one memory
location.
Signed-off-by: Pradosh Mohapatra <pmohapat at cumulusnetworks.com>
Reviewed-by: Scott Feldman <sfeldma at cumulusnetworks.com>
Reviewed-by: Ken Yin <kyin at cumulusnetworks.com>
[DL: whitespace changes dropped]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Compute multipath in BGP based on AS_PATH hop count match. If the knob
is turned on, it is not required to have an exact match of AS_PATHs
(provided other multipath conditions are met, of course).
Signed-off-by: Pradosh Mohapatra <pmohapat at cumulusnetworks.com>
Reviewed-by: Dinesh G Dutt <ddutt at cumulusnetworks.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
* bgp_attr.c: this UNSET_FLAG()s are bogus. I did a quick review and
I think that they could not cause any bug anyway.
Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Acked-by: Feng Lu <lu.feng@6wind.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Add support for keyword commands.
Includes new documentation for DEFUN() in lib/command.h, for preexisting
features as well as new keyword specification.
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Fixup some DEFUNS with incorrect command strings or mixed up helpstrings.
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
BGP: While advertising v4 prefixes over a v6 session, set the correct v4 nexthop.
ISSUE:
For an IPv6 peer, BGPd sets the local router-id as the next-hop's v4 address.
This is incorrect as the router-id may not be a valid next-hop to be included
in UPDATEs that contain v4 prefixes.
PATCH:
Set the v4 address in the next-hop field based on the interface that the
peering is on (directly connected interface or loopback).
Signed-off-by: Pradosh Mohapatra <pmohapat at cumulusnetworks.com>
Reviewed-by: Scott Feldman <sfeldma at cumulusnetworks.com>
Acked-by: Feng Lu <lu.feng@6wind.com>
Since commit ba281d3d04, ospfd uses NEXTHOP_IPV4_IFINDEX
routes. The API between zebra and bgpd which is used to query
nexthops for recursive routes did not support this nexthop
type and therefore, ospf changes (or any other IGP changes
which use NEXTHOP_IPV4_IFINDEX) would never trigger any
recursive route update.
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
When neighbor disable-connected-check was used, bgpd would accept routes
with unconnected nexthop as indended, however those routes would be
invalidated on the next bgp_scan run as that function did not know about
disable-connected-check.
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
bgp_interface_down() and bgp_exit() both proceed to delete the address
from bgpd's interface representation, so the second call gets a NULL
result from the hash lookup and subsequently crashes.
Signed-off-by: Rakesh Garimella <rakesh.garimella@sophos.com>
[reformatted]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Keepalives and updates are not expected in OpenSent, prior to receiving
the peer's open message. Terminate the session with the proper
notification.
From: Leonid Rosenboim <lrosenbo@wrs.com>
[split off FSM changes, some reordering & cleanup. read handling needs
to be separately addressed]
Signed-off-by: David Lamparter <equinox@diac24.net>
By forcing the hash table size to be a power of 2, a potentially
expensive divide can be replaced by a mask operation. Almost all
usage of the hash table was using default size of 1024. Only places
with different size was thread library (1011) and bgp aspath.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
This patch resolves the significance of order of group and password
statements.
It prevents passwords from being lost in cases where all
three conditions apply:
1. the peer is member of a group with or without group password
2. the peer has an individual password set
3. the peer is added to a group within an address-family ipv6
section
In addition this patch prevents the same issue in cases, where an IPv4
peer's password is set first and the peer is added to a group
afterwards.
Adding a peer to a group cancels his individual password. Without ipv6
this is not a problem, because choosing the right order of config
statements will do (set password only after adding peer to group).
When adding the peer to a group within the address-family
section, his password is definitely lost. The same workaround (ie.
setting the password after the address-family section) can not be used,
because "show run" will print the configuration statements in the wrong
order.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Commit 8692c50652 introduced a bug where bgpd would crash on
soft-reconfiguration.
This happens e.g. when there are filtered unicast routes because
rn->info is NULL in that case, which the code did not account for.
Reported-by: Paweł Staszewski <pstaszewski@itcare.pl>
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
commit fe9bb64... "bgpd: CVE-2012-1820, DoS in bgp_capability_orf()"
made the length test in bgp_capability_orf_entry() stricter and is now
causing us to refuse (with CEASE) ORF capabilites carrying any excess
data. This does not conform to the robustness principle as laid out by
RFC1122 ("be liberal in what you accept").
Even worse, RFC5291 is quite unclear on how to use the ORF capability
with multiple AFI/SAFIs. It can be interpreted as either "use one
instance, stuff everything in" but also as "use multiple instances".
So, if not for applying robustness, we end up clearing sessions from
implementations going by the former interpretation. (or if anyone dares
add a byte of padding...)
Cc: Denis Ovsienko <infrastation@yandex.ru>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
bgp_attr_munge_as4_attrs would previously try to reintegrate an AS4_PATH
with a NULL AS_PATH, leading to a rather nasty SEGV. Let's go by
RFC6793 and treat missing AS_PATH as 0-length AS_PATH, which in turn
means discarding the AS4_PATH.
[NB: we don't actually stick to the actual rule, which is discarding
AS4_PATH if it's longer than AS_PATH; indeed we should probably fix that
too]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Incorporate a patch by Svetozar Mihailov which implements
default-originate route-maps to behave as expected, i.e. allowing
the default route to be advertised conditionally, depending on a
criterion given by the route-map.
I am aware that the performance attributes of the following implementation
are far from optimal. However, this affects only code paths belonging to
a feature that is broken without this patch, therefore, it seems reasonable
to me to have this in the mainline for now.
Cc: Svetozar Mihailov <quagga@j.zarhi.com>
Reported-by: Sébastien Cramatte <scramatte@gmail.com>
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
This reverts commit b07458a055.
On second thought, the right way to do this is with rename(), not by
introducing a lock that can potentially even stall bgpd.
Reported-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
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>
The flag bit BGP_NODE_PROCESS_SCHEDULED is checked but never set.
This causes route node to be scheduled multiple times under load.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Invalid BGP Notification messages should be logged locally, cf.
RFC4271, Sect. 6.4, p 34,
NOTIFICATION Message Error Handling
Current notification for invalid Notification code:
2012/10/10 02:17:54 BGP: message index 10 not found in bgp_notify_msg (max is 8)
2012/10/10 02:17:54 BGP: 192.168.1.1 received NOTIFICATION 10/0 ((no item found)) 0 bytes
the logging should be a bit more clear. The above logging really doesn't
explain much and looks more like a programming error.
[rewrote most of it to get in something I can call a shape -David]
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
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>
