Skip to content

Commit

Permalink
isisd: fix incorrect snmp-id gen/free
Browse files Browse the repository at this point in the history 
Necessary structures for snmp-id generation are currently stored in
`struct isis`. When we generate the new circuit ID, we always use the
instance from the default VRF. When we free the circuit ID, we use the
instance from the circuit VRF. This causes the following problems:

1. If there is no instance in the default VRF, this code doesn't work.
2. When circuit in non-default VRF is deleted, the ID is not actually
   freed.

This is fixed by using global structures instead. The code itself is
moved to isis_snmp.c and linked to the main code using hooks. We should
not call SNMP-related code when the SNMP module is not loaded at all.

More than that, we don't allow to activate the circuit if we failed to
generate the SNMP ID. Even if SNMP support is completely disabled! This
check is removed.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
  • Loading branch information
@idryzhov
idryzhov committed Apr 29, 2021
1 parent d8c3dac commit e2b5b7d
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 83 deletions.
64 changes: 5 additions & 59 deletions isisd/isis_circuit.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,47 +73,8 @@ DEFINE_HOOK(isis_if_new_hook, (struct interface *ifp), (ifp));
int isis_if_new_hook(struct interface *);
int isis_if_delete_hook(struct interface *);

static int isis_circuit_smmp_id_gen(struct isis_circuit *circuit)
{
struct vrf *vrf = vrf_lookup_by_id(VRF_DEFAULT);
struct isis *isis = NULL;
uint32_t id;
uint32_t i;

isis = isis_lookup_by_vrfid(vrf->vrf_id);
if (isis == NULL)
return 0;

id = isis->snmp_circuit_id_last;
id++;

/* find next unused entry */
for (i = 0; i < SNMP_CIRCUITS_MAX; i++) {
if (id >= SNMP_CIRCUITS_MAX) {
id = 0;
continue;
}

if (id == 0)
continue;

if (isis->snmp_circuits[id] == NULL)
break;

id++;
}

if (i == SNMP_CIRCUITS_MAX) {
zlog_warn("Could not allocate a smmp-circuit-id");
return 0;
}

isis->snmp_circuits[id] = circuit;
isis->snmp_circuit_id_last = id;
circuit->snmp_id = id;

return 1;
}
DEFINE_HOOK(isis_circuit_new_hook, (struct isis_circuit *circuit), (circuit));
DEFINE_HOOK(isis_circuit_del_hook, (struct isis_circuit *circuit), (circuit));

struct isis_circuit *isis_circuit_new(struct isis *isis)
{
Expand All @@ -123,11 +84,6 @@ struct isis_circuit *isis_circuit_new(struct isis *isis)
circuit = XCALLOC(MTYPE_ISIS_CIRCUIT, sizeof(struct isis_circuit));

circuit->isis = isis;
/*
* Note: if snmp-id generation failed circuit will fail
* up operation
*/
isis_circuit_smmp_id_gen(circuit);

/*
* Default values
Expand Down Expand Up @@ -193,24 +149,21 @@ struct isis_circuit *isis_circuit_new(struct isis *isis)
isis_lfa_excluded_ifaces_init(circuit, ISIS_LEVEL1);
isis_lfa_excluded_ifaces_init(circuit, ISIS_LEVEL2);

hook_call(isis_circuit_new_hook, circuit);

QOBJ_REG(circuit, isis_circuit);

return circuit;
}

void isis_circuit_del(struct isis_circuit *circuit)
{
struct isis *isis = NULL;

if (!circuit)
return;

QOBJ_UNREG(circuit);

if (circuit->interface) {
isis = isis_lookup_by_vrfid(circuit->interface->vrf_id);
isis->snmp_circuits[circuit->snmp_id] = NULL;
}
hook_call(isis_circuit_del_hook, circuit);

isis_circuit_if_unbind(circuit, circuit->interface);

Expand Down Expand Up @@ -681,13 +634,6 @@ int isis_circuit_up(struct isis_circuit *circuit)
return ISIS_OK;
}

if (circuit->snmp_id == 0) {
/* We cannot bring circuit up if does not have snmp-id */
flog_err(EC_ISIS_CONFIG,
"No snnmp-id: there are too many circuits:");
return ISIS_ERROR;
}

if (circuit->area->lsp_mtu > isis_circuit_pdu_size(circuit)) {
flog_err(
EC_ISIS_CONFIG,
Expand Down
3 changes: 3 additions & 0 deletions isisd/isis_circuit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,4 +238,7 @@ DECLARE_HOOK(isis_circuit_config_write,
DECLARE_HOOK(isis_circuit_add_addr_hook, (struct isis_circuit *circuit),
(circuit));

DECLARE_HOOK(isis_circuit_new_hook, (struct isis_circuit *circuit), (circuit));
DECLARE_HOOK(isis_circuit_del_hook, (struct isis_circuit *circuit), (circuit));

#endif /* _ZEBRA_ISIS_CIRCUIT_H */
74 changes: 54 additions & 20 deletions isisd/isis_snmp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -629,25 +629,69 @@ static uint8_t isis_null_sysid[ISIS_SYS_ID_LEN];
/* Protocols supported value */
static uint8_t isis_snmp_protocols_supported = 0x7; /* All: iso, ipv4, ipv6 */

#define SNMP_CIRCUITS_MAX (512)

static struct isis_circuit *snmp_circuits[SNMP_CIRCUITS_MAX];
static uint32_t snmp_circuit_id_last;

static int isis_circuit_snmp_id_gen(struct isis_circuit *circuit)
{
uint32_t id;
uint32_t i;

id = snmp_circuit_id_last;
id++;

/* find next unused entry */
for (i = 0; i < SNMP_CIRCUITS_MAX; i++) {
if (id >= SNMP_CIRCUITS_MAX) {
id = 0;
continue;
}

if (id == 0)
continue;

if (snmp_circuits[id] == NULL)
break;

id++;
}

if (i == SNMP_CIRCUITS_MAX) {
zlog_warn("Could not allocate a smmp-circuit-id");
return 0;
}

snmp_circuits[id] = circuit;
snmp_circuit_id_last = id;
circuit->snmp_id = id;

return 0;
}

static int isis_circuit_snmp_id_free(struct isis_circuit *circuit)
{
snmp_circuits[circuit->snmp_id] = NULL;
circuit->snmp_id = 0;
return 0;
}

/*
* Convenience function to move to the next circuit,
*/
static struct isis_circuit *isis_snmp_circuit_next(struct isis_circuit *circuit)
{
uint32_t start;
uint32_t off;
struct isis *isis = isis_lookup_by_vrfid(VRF_DEFAULT);

if (isis == NULL)
return NULL;

start = 1;

if (circuit != NULL)
start = circuit->snmp_id + 1;

for (off = start; off < SNMP_CIRCUITS_MAX; off++) {
circuit = isis->snmp_circuits[off];
circuit = snmp_circuits[off];

if (circuit != NULL)
return circuit;
Expand Down Expand Up @@ -912,16 +956,12 @@ static int isis_snmp_circuit_lookup_exact(oid *oid_idx, size_t oid_idx_len,
struct isis_circuit **ret_circuit)
{
struct isis_circuit *circuit;
struct isis *isis = isis_lookup_by_vrfid(VRF_DEFAULT);

if (isis == NULL)
return 0;

if (oid_idx == NULL || oid_idx_len < 1
|| oid_idx[0] > SNMP_CIRCUITS_MAX)
return 0;

circuit = isis->snmp_circuits[oid_idx[0]];
circuit = snmp_circuits[oid_idx[0]];
if (circuit == NULL)
return 0;

Expand All @@ -937,10 +977,6 @@ static int isis_snmp_circuit_lookup_next(oid *oid_idx, size_t oid_idx_len,
oid off;
oid start;
struct isis_circuit *circuit;
struct isis *isis = isis_lookup_by_vrfid(VRF_DEFAULT);

if (isis == NULL)
return 0;

start = 0;

Expand All @@ -952,7 +988,7 @@ static int isis_snmp_circuit_lookup_next(oid *oid_idx, size_t oid_idx_len,
}

for (off = start; off < SNMP_CIRCUITS_MAX; ++off) {
circuit = isis->snmp_circuits[off];
circuit = snmp_circuits[off];

if (circuit != NULL && off > start) {
if (ret_circuit != NULL)
Expand Down Expand Up @@ -1011,10 +1047,6 @@ static int isis_snmp_circuit_level_lookup_next(
oid start;
struct isis_circuit *circuit;
int level;
struct isis *isis = isis_lookup_by_vrfid(VRF_DEFAULT);

if (isis == NULL)
return 0;

start = 0;

Expand All @@ -1026,7 +1058,7 @@ static int isis_snmp_circuit_level_lookup_next(
}

for (off = start; off < SNMP_CIRCUITS_MAX; off++) {
circuit = isis->snmp_circuits[off];
circuit = snmp_circuits[off];

if (circuit == NULL)
continue;
Expand Down Expand Up @@ -3449,6 +3481,8 @@ static int isis_snmp_module_init(void)
hook_register(isis_hook_adj_state_change,
isis_snmp_adj_state_change_update);
hook_register(isis_hook_lsp_error, isis_snmp_lsp_error_update);
hook_register(isis_circuit_new_hook, isis_circuit_snmp_id_gen);
hook_register(isis_circuit_del_hook, isis_circuit_snmp_id_free);

hook_register(frr_late_init, isis_snmp_init);
return 0;
Expand Down
4 changes: 0 additions & 4 deletions isisd/isisd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ extern void isis_cli_init(void);
all_vrf = strmatch(vrf_name, "all"); \
}

#define SNMP_CIRCUITS_MAX (512)

extern struct zebra_privs_t isisd_privs;

/* uncomment if you are a developer in bug hunt */
Expand Down Expand Up @@ -97,8 +95,6 @@ struct isis {
time_t uptime; /* when did we start */
struct thread *t_dync_clean; /* dynamic hostname cache cleanup thread */
uint32_t circuit_ids_used[8]; /* 256 bits to track circuit ids 1 through 255 */
struct isis_circuit *snmp_circuits[SNMP_CIRCUITS_MAX];
uint32_t snmp_circuit_id_last;
int snmp_notifications;

struct route_table *ext_info[REDIST_PROTOCOL_COUNT];
Expand Down

0 comments on commit e2b5b7d

Please sign in to comment.