pimd/pim6d: fix the wrong check of multicast interface in nb

[anlancs]

For example, on this case of interfaces created by frr, no real one on system at that time:

int x1
   ip igmp
int x2
   ip igmp
int x3
……….

The yang_get_list_elements_count() in pim is wrong:
The configuration node is added at the end of the link list. Therefore,
this function will traverse from the configuration node, so it
usually/wrongly returns 1.

This commit introduced three changes:

1. replace "== MAXVIFS" with "> MAXVIFS" because the maximum is MAXVIFS.
2. the check of MAXVIFS should be limited to the same address family.
3. check pim and igmp for ipv4 family, and check pim6 and mld for ipv6 family.

[greptile-apps]

Greptile Summary

This PR fixes a longstanding correctness bug in pimd/pim_nb_config.c where the YANG northbound validation of the MAXVIFS limit was silently broken. The old code used yang_get_list_elements_count(if_dnode) which (per the PR description) traverses from the configuration node rather than the root of the list, almost always returning 1 and rendering the check inert.

Key changes:

• Introduces pim_interface_num_get(), a new helper that correctly walks all sibling interface list nodes under the lib container and counts only those with PIM (frr-pim:pim/address-family/pim-enable) or GMP (frr-gmp:gmp/address-family/enable) enabled for the compile-time address family (FRR_PIM_AF_XPATH_VAL). This correctly scopes the count to a single address family and avoids the traversal bug.
• Applies the helper in both lib_interface_pim_address_family_pim_enable_modify and lib_interface_gmp_address_family_enable_modify.
• Changes the comparison from == MAXVIFS to > MAXVIFS. However, pim_iface_next_vif_index() reserves VIF index 0 exclusively for the pimreg device and allocates user interfaces to indices 1..MAXVIFS-1 — only MAXVIFS-1 usable slots. Using > MAXVIFS allows the MAXVIFS-th user interface through validation, which then silently fails in pim_if_add_vif() at apply time. The threshold should be >= MAXVIFS.

Confidence Score: 3/5

• The fix is a clear improvement over the previously broken check, but introduces an off-by-one boundary error that allows one more PIM interface than the VIF table can accommodate, causing a silent apply-time failure at the MAXVIFS limit.
• The new pim_interface_num_get helper is logically sound (correct tree traversal, address-family scoping, and duplicate-feature deduplication via ||). The primary concern is the > MAXVIFS threshold: since VIF index 0 is permanently reserved for pimreg, only MAXVIFS-1 user VIF slots exist; the check should be >= MAXVIFS. This is a minor but real edge-case regression at the exact limit.
• pimd/pim_nb_config.c — both MAXVIFS boundary checks (lines 2264 and 4601)

Important Files Changed

Filename

Overview

pimd/pim_nb_config.c

Replaces broken yang_get_list_elements_count with a new pim_interface_num_get helper that correctly iterates all interface list nodes and counts only those with PIM or GMP enabled for the current address family. The boundary check is changed from == MAXVIFS to > MAXVIFS, but this is off by one: only MAXVIFS-1 user VIF slots exist (VIF 0 is reserved for pimreg), so the threshold should be >= MAXVIFS.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[NB_EV_VALIDATE\npim-enable or gmp-enable modify] --> B[yang_dnode_get_parent\nget interface node]
    B --> C[pim_interface_num_get\ncount PIM/GMP enabled interfaces]
    C --> D[lyd_parent: get lib container]
    D --> E[LY_LIST_FOR all interface list nodes]
    E --> F{nodetype == LYS_LIST?}
    F -- No --> E
    F -- Yes --> G[yang_dnode_getf gmp/enable\nfor current AF]
    G --> H[yang_dnode_getf pim/pim-enable\nfor current AF]
    H --> I{gmp enabled\nOR pim enabled?}
    I -- Yes --> J[cnt++]
    I -- No --> E
    J --> E
    E -- done --> K{cnt > MAXVIFS?\nnote: should be >= MAXVIFS}
    K -- Yes --> L[NB_ERR_VALIDATION\nmax multicast interfaces reached]
    K -- No --> M[NB_EV_APPLY\npim_cmd_interface_add / pim_cmd_gm_start]
    M --> N[pim_if_add_vif\nget next VIF index 1..MAXVIFS-1]
    N --> O{index >= MAXVIFS?}
    O -- Yes --> P[return -3\nsilent failure\nif cnt was exactly MAXVIFS]
    O -- No --> Q[VIF registered successfully]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: pimd/pim_nb_config.c
Line: 2264

Comment:
**Off-by-one: allows one more interface than VIF slots support**

`pim_iface_next_vif_index()` always reserves VIF index 0 for `pimreg` and assigns user interfaces indices 1..`MAXVIFS-1` (see `pim_iface.c:952-956`):

```c
for (i = 1; i < MAXVIFS; i++) {   // only MAXVIFS-1 slots
    if (pim->iface_vif_index[i] == 0)
        return i;
}
return MAXVIFS;                    // signals "full"
```

`pim_if_add_vif()` rejects any index `>= MAXVIFS`. This means at most **`MAXVIFS-1` user-configured PIM interfaces** can be given VIF indices at apply time.

With `pim_interface_num_get(if_dnode) > MAXVIFS`, the validation passes when count equals exactly `MAXVIFS` (the `MAXVIFS`-th user interface). That interface gets through `NB_EV_VALIDATE` but then fails silently at `NB_EV_APPLY` because `pim_if_add_vif()` returns `-3` (returning `NB_ERR_INCONSISTENCY` only if `pim_cmd_interface_add` propagates the error).

The threshold should be `>= MAXVIFS` so validation catches this one case earlier:

```suggestion
		if (pim_interface_num_get(if_dnode) >= MAXVIFS) {
```

The same applies at line 4601 in `lib_interface_gmp_address_family_enable_modify`.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: pimd/pim_nb_config.c
Line: 4601

Comment:
**Same off-by-one as line 2264**

As noted for the PIM-enable handler at line 2264, `> MAXVIFS` should be `>= MAXVIFS` here as well. With `> MAXVIFS`, enabling GMP on the `MAXVIFS`-th interface passes validation but subsequently fails at VIF allocation time.

```suggestion
		if (pim_interface_num_get(if_dnode) >= MAXVIFS) {
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "pimd/pim6d: fix the ..."}

[anlancs]

@greptile review

[nabahr]

I wonder if it'd be better to determine if there are any available VIF indexes available by looking at pim->iface_vif_index instead.
int iface_vif_index[MAXVIFS];

This would more accurately determine if there is a VIF available for the new interface or not.

[anlancs]

It's nb code, seems better to check it in nb way. I'm not sure...

[nabahr]

The part that the NB doesn't account for are the vif indexes used by the pimreg interface(s).

[Jafaral]

@greptile review

[greptile-apps]

Off-by-one: allows one more interface than VIF slots support

pim_iface_next_vif_index() always reserves VIF index 0 for pimreg and assigns user interfaces indices 1..MAXVIFS-1 (see pim_iface.c:952-956):

for (i = 1; i < MAXVIFS; i++) {   // only MAXVIFS-1 slots
    if (pim->iface_vif_index[i] == 0)
        return i;
}
return MAXVIFS;                    // signals "full"

pim_if_add_vif() rejects any index >= MAXVIFS. This means at most MAXVIFS-1 user-configured PIM interfaces can be given VIF indices at apply time.

With pim_interface_num_get(if_dnode) > MAXVIFS, the validation passes when count equals exactly MAXVIFS (the MAXVIFS-th user interface). That interface gets through NB_EV_VALIDATE but then fails silently at NB_EV_APPLY because pim_if_add_vif() returns -3 (returning NB_ERR_INCONSISTENCY only if pim_cmd_interface_add propagates the error).

The threshold should be >= MAXVIFS so validation catches this one case earlier:

Suggested change

	if (pim_interface_num_get(if_dnode) > MAXVIFS) {
	if (pim_interface_num_get(if_dnode) >= MAXVIFS) {

The same applies at line 4601 in lib_interface_gmp_address_family_enable_modify.

Prompt To Fix With AI

This is a comment left during a code review.
Path: pimd/pim_nb_config.c
Line: 2264

Comment:
**Off-by-one: allows one more interface than VIF slots support**

`pim_iface_next_vif_index()` always reserves VIF index 0 for `pimreg` and assigns user interfaces indices 1..`MAXVIFS-1` (see `pim_iface.c:952-956`):

```c
for (i = 1; i < MAXVIFS; i++) {   // only MAXVIFS-1 slots
    if (pim->iface_vif_index[i] == 0)
        return i;
}
return MAXVIFS;                    // signals "full"
```

`pim_if_add_vif()` rejects any index `>= MAXVIFS`. This means at most **`MAXVIFS-1` user-configured PIM interfaces** can be given VIF indices at apply time.

With `pim_interface_num_get(if_dnode) > MAXVIFS`, the validation passes when count equals exactly `MAXVIFS` (the `MAXVIFS`-th user interface). That interface gets through `NB_EV_VALIDATE` but then fails silently at `NB_EV_APPLY` because `pim_if_add_vif()` returns `-3` (returning `NB_ERR_INCONSISTENCY` only if `pim_cmd_interface_add` propagates the error).

The threshold should be `>= MAXVIFS` so validation catches this one case earlier:

```suggestion
		if (pim_interface_num_get(if_dnode) >= MAXVIFS) {
```

The same applies at line 4601 in `lib_interface_gmp_address_family_enable_modify`.

How can I resolve this? If you propose a fix, please make it concise.

[nabahr]

It is off by at least one. I believe there is a pimreg interface created for each vrf instance as well.
I take that back, looks like the available VIF is per-vrf/mroute table so it's just off by one, but still needs to count interfaces per-vrf and not globally.

[greptile-apps]

Looking at the code, I can see each VRF creates its own pimreg interface (pim_instance.c:202), and each pim_instance has its own iface_vif_index[MAXVIFS] array (pim_instance.c:55-56).

However, the key question is whether the validation scope matches the VIF allocation scope:

1. VIF allocation is per-VRF: each pim_instance (VRF) has its own iface_vif_index[] array
2. But the new pim_interface_num_get() counts globally (or at least not VRF-scoped)

So you're correct that the check should account for pimreg. But I need to verify: is the validation counting per-VRF or globally?

Looking at the PR code, pim_interface_num_get() iterates lyd_child(lib_dnode) — this traverses all interfaces under the lib node. If that's global (not VRF-filtered), then:

• With 2 VRFs: validation counts 2 pimreg + all user interfaces globally
• But VIF allocation is per-VRF: each VRF can have up to MAXVIFS interfaces (including its own pimreg)

Could you clarify: does the current pim_interface_num_get() count all interfaces globally, or is it scoped to the VRF of the interface being configured?

If it's global, the check is too strict (multi-VRF setups would hit the limit prematurely). If it's per-VRF but doesn't exclude pimreg, then the threshold should indeed be >= MAXVIFS (not >), since:

• User wants to add their Nth user interface
• Count = N user interfaces + 1 pimreg = N+1 total
• At apply time, pimreg takes slot 0, user interfaces need slots 1..N
• Max user interfaces = MAXVIFS - 1, so count should reject at N+1 >= MAXVIFS, i.e., N >= MAXVIFS-1

[anlancs]

@nabahr Agreed, this commit is not good.
Because the interface can switch between different VRFs later on (maybe not config it on vtysh), it seems difficult to control...