| Age | Commit message (Collapse) | Author |
|---|
|
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>
|
|
Commit 558d1fec11749d3257e improved bgp_attr_dup so it would be possible
for the caller to provide attr_extra, allowing to use the stack instead
of the heap for operations requiring only a short lived attr.
However, this commit introduced a bug where bgp_attr_dup wouldn't copy
attr_extra at all (but provide a reference to the original) if the
caller provided attr_extra.
Cc: Jorge Boncompte [DTI2] <jorge@dti2.net>
Signed-off-by: Christian Franke <chris@opensourcerouting.org>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
When going through the code to write the documentation for local-as,
I discovered that one of the comments was out-of-date.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
Added replace-as modifier for BGP neighbors when using
local-as. If the replace-as modifier is specified, only the
replacement AS as specified by the local-as modifier is
prepended to the AS_PATH, not the process's AS.
In bgp_attr.c, I decided that
if (peer->change_local_as) {
/* If replace-as is specified, we only use the change_local_as when
advertising routes. */
if( ! CHECK_FLAG (peer->flags, PEER_FLAG_LOCAL_AS_REPLACE_AS) ) {
aspath = aspath_add_seq (aspath, peer->local_as);
}
aspath = aspath_add_seq (aspath, peer->change_local_as);
} else {
aspath = aspath_add_seq (aspath, peer->local_as);
}
was clearer than the alternative that didn't duplicate the prepending of the
process's AS:
/* First, append the process local AS unless we have an alternate local_as
* and we're replacing it (as opposed to just prepending it). */
if (! (peer->change_local_as
&& CHECK_FLAG (peer->flags, PEER_FLAG_LOCAL_AS_REPLACE_AS) ) ) {
aspath = aspath_add_seq (aspath, peer->local_as);
}
if (peer->change_local_as)
aspath = aspath_add_seq (aspath, peer->change_local_as);
}
But I could be convinced otherwise.
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
Use the array_size() helper macro. Replaces several instances of local
macros with the same definition.
Reviewed-by: Scott Feldman <sfeldma@cumulusnetworks.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
Try to use on stack structs for temporary uses.
Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
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>
|
|
Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
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>
|
|
Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
* bgp_attr.c: (bgp_attr_default_intern) bgp_attr_default_set() already
initializes the memory. Fixes a struct attr_extra leak.
* bgp_route.c: Remove useless on stack struct initializations.
Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
peer_sort() it's called so much as to be annoying. In the assumption
that the 'sort' of the peer doesn't change during an established session,
I have changed all calls to peer_sort() in the 'fast-path' to only check
the 'sort'. All the calls from the vty and such still recalculate the sort
and store it in the peer.
There's a lot of other calls to peer_sort() that could be changed but some
maube tricky, someone more knowledgeable may try to reduce them.
This hits peer_sort() from 5th out of the stadium^H^H list on a full
internet table loading profiling session.
Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
|
|
* bgp_attr.c: (bgp_attr_unknown) total should be initialised from the args.
|
|
* bgpd/bgp_attr.c: (bgp_attr_flags_diagnose) debug code for error-handling
paths probably shouldn't assert, instead it should just log that there
was no problem.
|
|
* bgpd/bgp_attr.c: (bgp_attr_parse) the invalid flag check call to
bgp_attr_malformed is pretty useless if it doesn't actually allow
for the PROCEED non-error case.
|
|
* bgp_attr.c: (bgp_attr_flag_invalid) flags is meant to be masked
off with the mask variable...
|
|
* bgp_attr.c: (bgp_attr_malformed) When a malformed attribute error can be
ignored, and BGP message processing may still proceed, the stream getp
should be adjusted to the end of the attribute - the caller may not have
consumed all the attribute. Problem noted by Martin Winter in bug 678.
Also, rename the 'startp' local to 'notify_datap', for clarity.
|
|
* bgp_attr.h: (struct bgp_attr_parser_args) Attribute parsing context,
containing common arguments.
* bgp_attr.c: (general) Move the bgp_attr_flag_invalid flag-check calls up,
out of each individual attr parser function, to be done once in attr_parse.
Similarly move the calculation of the 'total' attribute length field up
to attr_parse.
Bundle together common arguments to attr-parsing functions and helpers
into (struct bgp_attr_parser_args), so it can be passed by reference down
the stack & also de-clutter the argument lists & make it easier to
add/modify the context for attr-parsing - add local const aliases to avoid
modifying body of code too much. This also should help avoid cut & paste
errors, where calls to helpers with hard-coded attribute types are pasted
to other functions but the code isn't changed.
(bgp_attr_flags_diagnose) as above.
(bgp_attr_flag_invalid) as above.
(bgp_attr_{origin,aspath,as4_path,nexthop,med,local_pref,atomic}) as above.
(bgp_attr_{aggregator,as4_aggregator,community,originator_id}) as above
(bgp_attr_{cluster_list,ext_communities},bgp_mp_{un,}reach_parse) as above
(bgp_attr_unknown) as above.
(bgp_attr_malformed) as above. Also, startp and length have to be
special-cased, because whether or not to send attribute data depends
on the particular error - a separate length argument, distinct from
args->length, indicates whether or not the attribute data should be sent
in the NOTIFY.
(bgp_attr_aspath_check) Call to bgp_attr_malformed is wrong here, there is
no attribute parsing context - e.g. the 'flag' argument is unlikely to be
right, remove it. Explicitly handle the error instead.
(bgp_attr_munge_as4_attrs) Flag argument is pointless.
As the comment notes, the check here is pointless as AS_PATH presence
already checked elsewhere.
(bgp_attr_parse) Do bgp_attr_flag_invalid call here.
Use (struct bgp_attr_parser_args) for args to attr parser functions.
Remove out-of-context 'flag' argument to as4 checking functions.
|
|
* bgpd/bgp_attr.c: (attr_flags_values []) array of required flags for
attributes, EXTLEN & PARTIAL masked off as "dont care" as appropriate.
(bgp_attr_flag_invalid) check if flags may be invalid, according to
the above table & RFC rules.
(bgp_attr_*) Use bgp_attr_flag_invalid.
(bgp_attr_as4_aggregator) ditto, also take startp argument for the
NOTIFY data.
(bgp_attr_parse) pass startp to bgp_attr_as4_aggregator
|
|
|
|
* bgpd/bgp_attr.c: (bgp_attr_aspath) error message could be misleading,
clearly log what flag was incorrect.
(Problem noted in "bgpd: fix error message in bgp_attr_aspath()" in
Quagga-RE)
|
|
* bgp_attr.c
* attr_flag_str: new message list
* bgp_attr_flags_diagnose(): new function, implements previously added
error logging in a generic way
* bgp_attr_origin(): use bgp_attr_flags_diagnose()
* bgp_attr_nexthop(): ditto
* bgp_attr_med(): ditto
* bgp_attr_local_pref(): ditto
* bgp_attr_atomic(): ditto
* bgp_attr_originator_id(): ditto
* bgp_attr_cluster_list(): ditto
* bgp_mp_reach_parse(): ditto
* bgp_mp_unreach_parse(): ditto
|
|
(This patch was modified to leave calls to stream_getl() in place, they
are necessary for the stream's internal pointer to advance to the
correct position. -- Denis)
Signed-off-by: Denis Ovsienko <infrastation@yandex.ru>
Fix gcc warnings about varables that are set but never used.
* bgpd/bgp_attr.c
* cluster_unintern(): ret
* transit_unintern(): ret
* bgp_attr_default_intern(): attre
* bgp_mp_reach_parse(): rd_high, rd_low
* bgpd/bgp_route.c
* bgp_announce_check_rsclient(): bgp
* bgpd/bgp_zebra.c
* zebra_read_ipv4(): ifindex
* zebra_read_ipv6(): ifindex
* bgpd/bgpd.c
* bgp_config_write_peer(): filter
* lib/distribute.c
* distribute_list_all(): dist
* distribute_list(): dist
* distribute_list_prefix_all(): dist
* distribute_list_prefix(): dist
* lib/if_rmap.c
* if_rmap(): if_rmap
* lib/vty.c
* vty_accept(): vty
* lib/zclient.c
* zclient_read(): ret
* zebra/irdp_interface.c
* if_group(): zi
* zebra/rt_netlink.c
* kernel_read(): ret, sock
|
|
this fixes commit b881c7074bb698aeb1b099175b325734fc6e44d2
|
|
* bgp_attr.c (bgp_attr_originator_id, bgp_attr_cluster_list): provide
required arguments to bgp_attr_malformed()
|
|
* bgp_attr.c
* bgp_attr_aggregator(): check Optional/Transitive flag bits
|
|
Commit 05a4936b713b9882171d0f7fb20b8439df23939e fixed some of the
attributes involved, but not all. This commit should do it.
* bgp_attr.c
* bgp_attr_originator_id()
* bgp_attr_cluster_list()
* bgp_mp_reach_parse()
* bgp_mp_unreach_parse()
|
|
Some of the recent attribute flags/length checks copied from QRE use
bgp_notify_send_with_data() directly, but master branch assumes
using bgp_attr_malformed().
* bgp_attr.c
* bgp_attr_med()
* bgp_attr_local_pref()
* bgp_attr_atomic()
* bgp_attr_originator_id()
* bgp_attr_cluster_list()
* bgp_mp_reach_parse()
* bgp_mp_unreach_parse()
|
|
* bgp_attr.[ch]
* bgp_mp_reach_parse(): add extra arguments and a uniform flag
check block
* bgp_mp_unreach_parse(): idem
* bgp_attr_parse(): provide extra arguments
* bgp_mp_attr_test.c
* parse_test(): justify respective calls
|
|
|
|
* bgp_attr.c
* bgp_attr_cluster_list(): accept extra argument, add checks for
"optional", "transitive" and "partial" bits, log each error
condition independently
* bgp_attr_parse(): provide extra arguments
|
|
* bgp_attr.c
* bgp_attr_originator_id(): accept extra argument, add checks for
"optional", "transitive" and "partial" bits, log each error
condition independently
* bgp_attr_parse(): provide extra arguments
|
|
Commit 2febf323411c1aed9d7694898f852ce2ef36a7e5 assumed every flag
bit except optional/transitive/partial unset, which at times could
not be true for "extended length" bit.
* bgp_attr.c
* bgp_attr_origin(): exclude BGP_ATTR_FLAG_EXTLEN from comparison
* bgp_attr_nexthop(): idem
* bgp_attr_med(): idem
* bgp_attr_local_pref(): idem
* bgp_attr_atomic(): idem
|
|
* bgp_attr.c
* bgp_attr_parse(): provide extra argument to bgp_attr_aggregator()
* bgp_attr_local_pref(): use bgp_notify_send_with_data()
* bgp_attr_atomic(): idem
* bgp_attr_aggregator(): idem
Conflicts:
bgpd/bgp_attr.c
|
|
Do not check each of the Optional/Transitive/Partial attribute
flag bits, when their only valid combination is known in advance,
but still perform bit-deep error message logging. This change
assumes unused (low-order) 4 bits of the flag octet cleared.
* bgp_attr.c
* bgp_attr_origin(): rewrite check
* bgp_attr_nexthop(): idem
* bgp_attr_med(): idem
* bgp_attr_local_pref(): idem
* bgp_attr_atomic(): idem
Conflicts:
bgpd/bgp_attr.c
|
|
|
|
ORIGIN handling function used to have "partial" bit check and recent
commits added it for NEXT_HOP, MULTI_EXIT_DISC and ATOMIC_AGGREGATE
cases. This commit adds "partial" check for AS_PATH and LOCAL_PREF
cases, which should leave attributes 1 through 6 inclusive completely
covered with attribute flags checks.
* bgp_attr.c
* bgp_attr_origin(): use bit-by-bit checks for better diagnostics
* bgp_attr_aspath(): add flag check
* bgp_attr_local_pref(): idem
Conflicts:
bgpd/bgp_attr.c
|
|
* lib/prefix.h
* IPV4_CLASS_DE(): new helper macro
* bgp_attr.c
* bgp_attr_nexthop(): add check for "partial" bit, refresh flag error
reporting, explain meaning of RFC4271 section 6.3 and implement it
Conflicts:
bgpd/bgp_attr.c
|
|
(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
|
|
* bgp_attr.c
* bgp_attr_atomic(): accept extra argument, add checks for
"optional", "transitive" and "partial" bits, log each error
condition independently
* bgp_attr_parse(): provide extra argument
|
|
* bgp_attr.c
* bgp_attr_med(): add checks for "optional", "transitive" and
"partial" bits, log each error condition independently
|
|
* bgp_attr.c
* bgp_attr_local_pref(): accept extra argument, add checks for
"optional" and "transitive" bits, log each error condition
independently
* bgp_attr_parse(): provide extra argument
|
|
|
|
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.
|
|
* bgp_attr.c: (attrhash_key_make) 98e30f should have changed jhash2 to jhash.
These kinds of merge errors would be reduced and life would be easier if
people would submit fully-formed fixes that could be chucked directly into
git-am.
|
|
* bgp_attr.c; (attrhash_key_make) s6_addr is only member of in6_addr
guaranteed to be available - s6_addr32 isn't. Fix to be more portable, and
thus allow compilation on BSD again.
|
|
* 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
|
|
* draft-ietf-idr-as-pathlimit doesn't seem to have gone anywhere, and its
author does not think it will make progress in IDR. Remove all support
introduced for it, but leave stubs for the commands to avoid breaking
any configurations.
Basically reverts cecab5e9725792e60a5e4b473e238a14cd85815d.
|
|
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?)
|
|
Some of the changes made in commit cddb8112b80fa9867156c637d63e6e79eeac67bb
don't work particularly well for other changes that need to be made to
address BGP attribute error handling problems. In particular, returning
a pointer from complex attribute data parsing functions will not suffice
to express the require range of return status conditions.
* bgp_aspath.c: (assegments_parse) Rollback to a more minimal set of
changes to fix the original problem.
(aspath_parse) Slightly needless pushing around of code, and taking
2 parameters to say whether ot use 2 or 4 byte encoding seems unnecessary.
* bgp_attr.c: (bgp_attr_as{,4}path) Rollback, in preparation for BGP
attribute error handling update.
|
