The bgp labelpool code is grabbing the vpn policy data structure.
This vpn_policy has a pointer to the bgp data structure. If
a item placed on the bgp label pool workqueue happens to sit
there for the microsecond or so and the operator issues a
`no router bgp...` command that corresponds to the vpn_policy
bgp pointer, when the workqueue is run it will crash because
the bgp pointer is now freed and something else owns it.
Modify the labelpool code to store the vrf id associated
with the request on the workqueue. When you wake up
if the vrf id still has a bgp pointer allow the request
to continue, else drop it.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Memory was not zeroed at allocation.
> ==2604268== Conditional jump or move depends on uninitialised value(s)
> ==2604268== at 0x26F026: bgp_labels_unintern (bgp_label.c:116)
> ==2604268== by 0x30000C: adj_free (bgp_updgrp_adv.c:81)
> ==2604268== by 0x3019EE: bgp_adj_out_remove_subgroup (bgp_updgrp_adv.c:728)
> ==2604268== by 0x30537C: subgroup_withdraw_packet (bgp_updgrp_packet.c:1042)
> ==2604268== by 0x2970AF: bgp_generate_updgrp_packets (bgp_packet.c:508)
> ==2604268== by 0x49E06F1: event_call (event.c:2001)
> ==2604268== by 0x495AB8B: frr_run (libfrr.c:1238)
> ==2604268== by 0x200C4B: main (bgp_main.c:555)
Fixes: 3c86f776f0 ("bgpd: add bgp_labels hash")
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Fix static-analyser warnings with BGP labels:
> $ scan-build make -j12
> bgpd/bgp_updgrp_packet.c:819:10: warning: Access to field 'extra' results in a dereference of a null pointer (loaded from variable 'path') [core.NullDereference]
> ? &path->extra->labels->label[0]
> ^~~~~~~~~
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
The handling of MPLS labels in BGP faces an issue due to the way labels
are stored in memory. They are stored in bgp_path_info but not in
bgp_adj_in and bgp_adj_out structures. As a consequence, some
configuration changes result in losing labels or even a bgpd crash. For
example, when retrieving routes from the Adj-RIB-in table
("soft-reconfiguration inbound" enabled), labels are missing.
bgp_path_info stores the MPLS labels, as shown below:
> struct bgp_path_info {
> struct bgp_path_info_extra *extra;
> [...]
> struct bgp_path_info_extra {
> mpls_label_t label[BGP_MAX_LABELS];
> uint32_t num_labels;
> [...]
To solve those issues, a solution would be to set label data to the
bgp_adj_in and bgp_adj_out structures in addition to the
bgp_path_info_extra structure. The idea is to reference a common label
pointer in all these three structures. And to store the data in a hash
list in order to save memory.
However, an issue in the code prevents us from setting clean data
without a rework. The extra->num_labels field, which is intended to
indicate the number of labels in extra->label[], is not reliably checked
or set. The code often incorrectly assumes that if the extra pointer is
present, then a label must also be present, leading to direct access to
extra->label[] without verifying extra->num_labels. This assumption
usually works because extra->label[0] is set to MPLS_INVALID_LABEL when
a new bgp_path_info_extra is created, but it is technically incorrect.
Cleanup the label code by setting num_labels each time values are set in
extra->label[] and checking extra->num_labels before accessing the
labels.
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
This will allow a consistency of approach to adding/removing
pi's to from the workqueue for processing as well as properly
handling the dest->info pi list more appropriately.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Let's use the natural data structure in bgp for the prefix display
instead of a bunch of places where we call a translator function.
The %pBD does this and actually ensures data is correct.
Also fix a few spots in bgp_zebra.c where the cast to a NULL
pointer causes the catcher functionality to not work and fix
the resulting crash that resulted.
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Create a bgp_labels_same() function that does the
same operations as the static function labels_same from
bgp_mplsvpn.c.
Signed-off-by: Philippe Guibert <philippe.guibert@6wind.com>
This is a first in a series of commits, whose goal is to rename
the thread system in FRR to an event system. There is a continual
problem where people are confusing `struct thread` with a true
pthread. In reality, our entire thread.c is an event system.
In this commit rename the thread.[ch] files to event.[ch].
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
This is already handled in bgp_nlri_parse() by checking error code.
Even more, we should send error sub-code to be according the NLRI type.
If it's MP_UPDATE/MP_WITHDRAW, sub-code should be an Optional Attribute error.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Fixes a couple crashes associated with attempting to read
beyond the end of the stream.
Reported-by: Iggy Frankovic <iggyfran@amazon.com>
Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Start using mpls_lse_encode/mpls_lse_decode, that is endian-aware, because
we always use host-byte order, should use network-byte.
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Don't hide the LABELED_UNICAST safi when processing route
updates; map it where necessary (to use the UNICAST table
for instance).
Signed-off-by: Mark Stapp <mstapp@nvidia.com>
In some cases where bgp is at the mpls edge, where it has a BGP-LU
peer downstream but an IP peer upstream, it can advertise the
IMPLICIT_NULL label instead of a per-prefix label.
Signed-off-by: Mark Stapp <mstapp@nvidia.com>
when we de-register for a label associated with a prefix then release
it back to the labelpool to clean up the stale reference held in the
ledger and inuse lists.
Signed-off-by: Pat Ruddy <pat@voltanet.io>
To prepare for fixing an issue where labels do not get released back
to the labelpool when the route is deleted some refactoring is
necessary. There are 2 parts to this.
1. restructure the code to remove the circular nature of label
allocations via the labelpool and decouple the label type decision
from the notification fo the FEC.
The code to notify the FEC association to zebra has been split out
into a separate function so that it can be called from the synchronous
path (for registration of index-based labels and de-registration of all
labels), and from the asynchronous path where we need to wait for a
callback from the labelpool code with a label allocation.
The decision about whether we are using an index-based label or an
allocated label is reflected in the state of the BGP_NODE_LABEL_REQUESTED
flag so the checks on the path_info in the labelpool callback code are
no longer required.
2. change the owned of a labelpool allocated label from the path info
structure to the bgp_dest structure. This allows labels to be released
(in a subsequent commit) when the owner (bgp_dest) goes away.
Signed-off-by: Pat Ruddy <pat@voltanet.io>
Remove mid-string line breaks, cf. workflow doc:
.. [#tool_style_conflicts] For example, lines over 80 characters are allowed
for text strings to make it possible to search the code for them: please
see `Linux kernel style (breaking long lines and strings)
<https://www.kernel.org/doc/html/v4.10/process/coding-style.html#breaking-long-lines-and-strings>`_
and `Issue #1794 <https://github.com/FRRouting/frr/issues/1794>`_.
Scripted commit, idempotent to running:
```
python3 tools/stringmangle.py --unwrap `git ls-files | egrep '\.[ch]$'`
```
Signed-off-by: David Lamparter <equinox@diac24.net>
This is the bulk part extracted from "bgpd: Convert from `struct
bgp_node` to `struct bgp_dest`". It should not result in any functional
change.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
Add new function `bgp_node_get_prefix()` and modify
the bgp code base to use it.
This is prep work for the struct bgp_dest rework.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
uint8_t * cannot be cast to uint32_t * unless the
pointed-to address is aligned according to uint32_t's
alignment rules. And it usually is not.
Signed-off-by: Santosh P K <sapk@vmware.com>
under some conditions, the callback to get a label for
a LU bgp path could be called after the path had already
been freed. In this case we would be reading garbage
and potentially crash. Lock the path info before
queueing the callback, and unlock as the first step
of the callback, exiting gracefully if the path info
is now NULL.
Signed-off-by: Emanuele Di Pascale <emanuele@voltanet.io>
This commit is the last missing piece to complete BGP LU support in bgpd. To this moment, bgpd (and zebra) supported auto label assignment only for prefixes leaked from VRFs to vpn and for MPLS SR prefixes. This adds auto label assignment to other routes types in bgpd. The following enhancements have been made:
* bgp_route.c:bgp_process_main_one() now sets implicit-null local_label to all local, aggregate and redistributed routes.
* bgp_route.c:bgp_process_main_one() now will request a label from the label pool for any prefix that loses the label for some reason (for example, when the static label assignment config is removed)
* bgp_label.c:bgp_reg_dereg_for_label() now requests labels from label pool for routes which have no associated label index
* zebra_mpls.c:zebra_mpls_fec_register() now expects both label and label_index from the calling function, one of which must be set to MPLS_INVALID_LABEL or MPLS_INVALID_LABEL_INDEX, based on this it will decide how to register the provided FEC.
Signed-off-by: Anton Degtyarev <anton@cumulusnetworks.com>
Do a straight conversion of `struct bgp_info` to `struct bgp_path_info`.
This commit will setup the rename of variables as well.
This is being done because `struct bgp_info` is not descriptive
of what this data actually is. It is path information for routes
that we keep to build the actual routes nexthops plus some extra
information.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
The warning given by PVS-Studio is related to per-element overflow (there is
no real overflow, because of how elements are mapped in the union). This
same warning is typically reported by Coverity, too.
Signed-off-by: F. Aragon <paco@voltanet.io>
