bgpd: dynamic neighbors not up with md5 in non default vrf

[hnattamaisub]

Issue:
When non default vrf, bgp instance and peer group with dynamic neighbors
and md5 config are configured together, bgp peer config happens first.
But bgp vrf enable notification is received later.
BGP listerner socket get created now, but the md5 password is not set on
listening FD. Due to this tcp session not getting established.

Fix: walk through the bgp peer group listening socket range for dynamic
neighbors and set md5 password when the listener socket is created for
non default vrf.

[greptile-apps]

Greptile Summary

This PR fixes a race condition where a non-default VRF BGP instance with peer-group MD5 and dynamic neighbors fails to establish TCP sessions because the listen socket is created after the BGP config is already loaded — leaving the listener without its MD5 password applied. The fix introduces bgp_listener_md5_listen_range() and walks all peer-group listen ranges in bgp_listener() for non-default VRF sockets, applying MD5 immediately after the listener is registered. A new topotest (bgp_vrf_dynamic_md5_listen_late) covers the specific ordering scenario. Previous review concerns (missing VRF_DEFAULT guard and per-address-family guard) have been addressed.

Confidence Score: 5/5

Safe to merge; the fix is targeted, previous P0/P1 concerns were resolved, and the one remaining comment is a defensive-coding suggestion.

Both prior P1 concerns (missing VRF_DEFAULT guard and wrong-address-family MD5 application) are resolved. The only open finding is a P2 suggestion to add a defensive close(listener->fd) in the teardown path to make cleanup self-contained; the current code is not broken in practice because bgp_socket always closes the fd on a non-zero return.

bgpd/bgp_network.c — specifically the !md5_ok teardown block around line 1113.

Important Files Changed

Filename	Overview
bgpd/bgp_network.c	Adds bgp_listener_md5_listen_range() helper and applies TCP MD5 to non-default VRF listen sockets after creation; includes VRF_DEFAULT guard and per-family guard to skip mismatched address families.
tests/topotests/bgp_vrf_dynamic_md5_listen_late/test_bgp_vrf_dynamic_md5_listen_late.py	New topotest covering non-default VRF dynamic neighbor + MD5 late-listen scenario; includes diagnostic bundle on failure and kernel version guard for tcp_l3mdev_accept.
tests/topotests/bgp_vrf_dynamic_md5_listen_late/switch1/frr.conf	DUT config: default BGP instance defined first (ordering intentional to reproduce the bug), then vrf1 with dynamic listen range and MD5 peergroup1.
tests/topotests/bgp_vrf_dynamic_md5_listen_late/switch2/frr.conf	Static peer toward DUT in vrf1 with matching MD5 password; uses disable-connected-check for cross-VRF eBGP.
tests/topotests/bgp_vrf_dynamic_md5_listen_late/switch3/frr.conf	Static peer toward DUT in default VRF with peergroup2 MD5; confirms default-VRF path is unaffected by the fix.

Sequence Diagram

sequenceDiagram
    participant Z as zebra
    participant B as bgpd
    participant K as kernel (TCP MD5)

    Note over B: Config loaded: bgp 65001 vrf vrf1<br/>peer-group peergroup1 + password + listen range

    Z->>B: VRF enable notification (vrf1)
    B->>B: bgp_socket(bgp_vrf1, port, addr)
    B->>K: vrf_socket() → sock fd
    B->>B: bgp_listener(sock, sa, salen, bgp_vrf1)
    B->>K: bind + listen on sock
    B->>B: listener->bgp = bgp_vrf1 (non-default VRF)
    B->>B: event_add_read + listnode_add

    Note over B: NEW: walk peer groups for non-default VRF
    loop each peer_group in bgp_vrf1->group
        loop each listen_range[AFI_IP / AFI_IP6]
            B->>B: bgp_listener_md5_listen_range(listener, group, prefix, peer)
            alt family matches and peer->password set
                B->>K: bgp_md5_set_socket(fd, su, prefixlen, password)
                K-->>B: 0 (success)
            else family mismatch or no password
                B->>B: skip (return 0)
            end
        end
    end

    alt all MD5 set OK
        B-->>Z: return 0 (listener active)
    else MD5 failure
        B->>B: event_cancel + listnode_delete + XFREE listener
        B->>K: close(sock) via bgp_socket on -1 return
        B-->>Z: return -1
    end

    Note over B: Incoming TCP SYN from switch2 (vrf1)<br/>MD5 now matched → session Established

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: bgpd/bgp_network.c
Line: 1113-1118

Comment:
**Listener teardown only succeeds when a listener was fully registered**

The teardown block (`event_cancel` + `listnode_delete` + `XFREE`) runs unconditionally on any MD5 failure, but `event_add_read` / `listnode_add` precede the MD5 loop. This ordering is fine for the current code. However, `listener->fd` is not closed here — the caller (`bgp_socket`) closes `sock` when `bgp_listener` returns non-zero (line 1189–1190), which is the correct pattern consistent with other failure paths in this function.

One subtle gap: if `bgp_socket` is called from a path other than the main one (e.g., future callers that don't mirror the `close(sock)` on failure), the fd would leak. A defensive `close(listener->fd)` before `XFREE(MTYPE_BGP_LISTENER, listener)` would make the teardown self-contained and guard against that:

```c
	if (!md5_ok) {
		event_cancel(&listener->event);
		listnode_delete(bm->listen_sockets, listener);
		close(listener->fd);
		XFREE(MTYPE_BGP_LISTENER, listener->name);
		XFREE(MTYPE_BGP_LISTENER, listener);
		return -1;
	}
```

This mirrors how `bgp_close_vrf_socket` (line ~1221) closes the fd before freeing the listener.

```suggestion
	if (!md5_ok) {
		event_cancel(&listener->event);
		listnode_delete(bm->listen_sockets, listener);
		close(listener->fd);
		XFREE(MTYPE_BGP_LISTENER, listener->name);
		XFREE(MTYPE_BGP_LISTENER, listener);
		return -1;
	}
```

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

_{Reviews (3): Last reviewed commit: "tests: Added topotests to simulate the p..." | Re-trigger Greptile}

[ton31337]

Please switch topotest using frr.con only (instead of per-daemon configuration files).

[hnattamaisub]

Please switch topotest using frr.con only (instead of per-daemon configuration files).

Taken care thanks

[ton31337]

@greptileai review

[riw777]

looks good, waiting on @ton31337 's ai comments

[riw777]

looks good