bgpd: fix, do not access peer->notify.data when it is null #16546

dmytroshytyi-6WIND · 2024-08-09T14:37:11Z

bgp crash on rt2 spotted when 2 commands
launched consequently:

T0: rr, config -> router bgp 65004 -> neighbor 192.168.12.2 password
8888
T1: rt2, snmpwalk -v 2c -c public 127.0.0.1 .1.3.6.1.4.1.7336.4.2.1
T2: rt2 bgp crashed.

[EDIT1] -> reduce the configs as @ton31337 requested
config rt2-bgp:

!
debug bgp updates
!
router bgp 65002
 no bgp ebgp-requires-policy
 no bgp network import-check
 no bgp default ipv4-unicast
 neighbor 192.168.12.4 remote-as external
 neighbor 192.168.12.4 timers 1 3
 neighbor 192.168.12.4 timers connect 1
 !
 address-family ipv4 unicast
  neighbor 192.168.12.4 activate
  neighbor 192.168.12.4 addpath-tx-all-paths
 exit-address-family
 !
agentx
!

config rt2-snmpd:

agentAddress 127.0.0.1,[::1]

group public_group v1 public
group public_group v2c public
access public_group "" any noauth prefix all all none

rocommunity public default

view all included .1

iquerySecName frr
rouser frr

master agentx

agentXSocket /etc/frr/agentx
agentXPerms 777 755 root frr

config rt2-zebra:

!
interface r2-eth0
 ip address 192.168.12.2/24
 !

config rr-bgpd:

!
debug bgp updates
!
router bgp 65004
 no bgp ebgp-requires-policy
 no bgp network import-check
 no bgp default ipv4-unicast
 neighbor 192.168.12.2 remote-as external
 neighbor 192.168.12.2 timers 1 3
 neighbor 192.168.12.2 timers connect 1
 !
 address-family ipv4 unicast
  neighbor 192.168.12.2 activate
  neighbor 192.168.12.2 addpath-tx-all-paths
  neighbor 192.168.12.2 route-server-client
 exit-address-family
!
agentx
!

config rr-zebra:

!
interface rr-eth0
 ip address 192.168.12.4/24
!

Signed-off-by: Dmytro Shytyi [email protected]

dmytroshytyi-6WIND · 2024-08-09T18:01:34Z

ci:rerun

ton31337

Please reduce every *.conf to a minimal that is needed for a test, because now it's lots of leftovers, etc. To ease the life when need to debug it.

ton31337 · 2024-08-09T18:40:34Z

tests/topotests/bgp_snmp_bgp4v2_notification/test_bgp_snmp_bgp4v2_notification.py

@@ -0,0 +1,123 @@
+#!/usr/bin/env python
+


Please add SPDX-License tag.

Hello @ton31337
Thank you for your comments.

SPDX-License tag fixed and configs are reduced.

ton31337

Some cleanups for topotest file and LGTM.

ton31337 · 2024-08-12T18:43:05Z

tests/topotests/bgp_snmp_bgp4v2_notification/test_bgp_snmp_bgp4v2_notification.py

+import os
+import sys
+import json
+from time import sleep


Could you drop this import, it's not used.

ton31337 · 2024-08-12T18:43:13Z

tests/topotests/bgp_snmp_bgp4v2_notification/test_bgp_snmp_bgp4v2_notification.py

+from time import sleep
+import pytest
+import functools
+import subprocess


ton31337 · 2024-08-12T18:59:12Z

@Mergifyio backport stable/10.1 stable/10.0 stable/9.1 stable/9.0

mergify · 2024-08-12T18:59:31Z

backport stable/10.1 stable/10.0 stable/9.1 stable/9.0

✅ Backports have been created

#16622 bgpd: fix, do not access peer->notify.data when it is null (backport #16546) has been created for branch stable/10.1
#16623 bgpd: fix, do not access peer->notify.data when it is null (backport #16546) has been created for branch stable/10.0
#16624 bgpd: fix, do not access peer->notify.data when it is null (backport #16546) has been created for branch stable/9.1
#16625 bgpd: fix, do not access peer->notify.data when it is null (backport #16546) has been created for branch stable/9.0

ton31337 · 2024-08-15T07:03:27Z

tests/topotests/bgp_snmp_bgp4v2_notification/test_bgp_snmp_bgp4v2_notification.py

+    test_func1 = functools.partial(_check_bgp_session)
+    _, result1 = topotest.run_and_expect(test_func1, None, count=60, wait=0.5)
+
+    assert result1 is None, "Failed to verify the nexthop_resolution MPLS: bgp session"


ton31337 · 2024-08-15T07:04:52Z

tests/topotests/bgp_snmp_bgp4v2_notification/test_bgp_snmp_bgp4v2_notification.py

+
+        output = json.loads(r2.vtysh_cmd("show bgp summary json"))
+
+        if output["ipv4Unicast"]["peers"]["192.168.12.4"]["state"] == "Established":


please use the same format we have/use for such things, see "expected = {...}".

bgp crash on rt2 spotted when 2 commands launched consequently: T0: rr, config -> router bgp 65004 -> neighbor 192.168.12.2 password 8888 T1: rt2, snmpwalk -v 2c -c public 127.0.0.1 .1.3.6.1.4.1.7336.4.2.1 T2: rt2 bgp is crashed. config rt2-bgp: --------------- ! debug bgp updates ! router bgp 65002 no bgp ebgp-requires-policy no bgp network import-check no bgp default ipv4-unicast neighbor 192.168.12.4 remote-as external neighbor 192.168.12.4 timers 1 3 neighbor 192.168.12.4 timers connect 1 neighbor 2001:db8::12:4 remote-as external neighbor 2001:db8::12:4 timers 1 3 neighbor 2001:db8::12:4 timers connect 1 ! address-family ipv4 unicast neighbor 192.168.12.4 activate neighbor 192.168.12.4 addpath-tx-all-paths exit-address-family address-family ipv6 unicast neighbor 2001:db8::12:4 activate exit-address-family ! agentx ! config rt2-snmpd: agentAddress 127.0.0.1,[::1] group public_group v1 public group public_group v2c public access public_group "" any noauth prefix all all none rocommunity public default view all included .1 iquerySecName frr rouser frr master agentx agentXSocket /etc/frr/agentx agentXPerms 777 755 root frr config rt2-zebra: ----------------- ! interface r2-eth0 ip address 192.168.12.2/24 ipv6 address 2001:db8::12:2/64 ! config rr-bgpd: --------------- ! debug bgp updates ! router bgp 65004 no bgp ebgp-requires-policy no bgp network import-check no bgp default ipv4-unicast neighbor 192.168.12.1 remote-as external neighbor 192.168.12.1 timers 1 3 neighbor 192.168.12.1 timers connect 1 neighbor 192.168.12.2 remote-as external neighbor 192.168.12.2 timers 1 3 neighbor 192.168.12.2 timers connect 1 neighbor 192.168.12.3 remote-as external neighbor 192.168.12.3 timers 1 3 neighbor 192.168.12.3 timers connect 1 neighbor 2001:db8::12:1 remote-as external neighbor 2001:db8::12:1 timers 1 3 neighbor 2001:db8::12:1 timers connect 1 neighbor 2001:db8::12:2 remote-as external neighbor 2001:db8::12:2 timers 1 3 neighbor 2001:db8::12:2 timers connect 1 neighbor 2001:db8::12:3 remote-as external neighbor 2001:db8::12:3 timers 1 3 neighbor 2001:db8::12:3 timers connect 1 ! address-family ipv4 unicast neighbor 192.168.12.1 activate neighbor 192.168.12.1 addpath-tx-all-paths neighbor 192.168.12.1 route-server-client neighbor 192.168.12.2 activate neighbor 192.168.12.2 addpath-tx-all-paths neighbor 192.168.12.2 route-server-client neighbor 192.168.12.3 activate neighbor 192.168.12.3 addpath-tx-all-paths neighbor 192.168.12.3 route-server-client exit-address-family address-family ipv6 unicast neighbor 2001:db8::12:1 activate neighbor 2001:db8::12:1 route-server-client neighbor 2001:db8::12:2 activate neighbor 2001:db8::12:2 route-server-client neighbor 2001:db8::12:3 activate neighbor 2001:db8::12:3 route-server-client exit-address-family ! agentx ! config rr-zebra: ---------------- ! interface rr-eth0 ip address 192.168.12.4/24 ipv6 address 2001:db8::12:4/64 ! Fixes: 2d8fff6b81bb ("bgpd: Implement BGP4V2-MIB(bgp4V2PeerErrorsTable)") Signed-off-by: Dmytro Shytyi <[email protected]>

This test checks the bgp crash on rt2 when 2 commands launched consequently: T0: rr, config -> router bgp 65004 -> neighbor 192.168.12.2 password 8888 T1: rt2, snmpwalk -v 2c -c public 127.0.0.1 .1.3.6.1.4.1.7336.4.2.1 T2: test if rt2 bgp is crashed. Signed-off-by: Dmytro Shytyi <[email protected]>

ton31337

LGTM

bgpd: fix, do not access peer->notify.data when it is null (backport #16546)

frrbot bot added bgp bugfix labels Aug 9, 2024

github-actions bot added size/L master labels Aug 9, 2024

ton31337 requested changes Aug 9, 2024

View reviewed changes

dmytroshytyi-6WIND force-pushed the fix_bgp_open_notification_snmp branch from 7712317 to 55f3829 Compare August 12, 2024 16:43

ton31337 approved these changes Aug 12, 2024

View reviewed changes

github-actions bot added the backport label Aug 12, 2024

dmytroshytyi-6WIND force-pushed the fix_bgp_open_notification_snmp branch from 55f3829 to 9360fbc Compare August 14, 2024 11:43

ton31337 reviewed Aug 15, 2024

View reviewed changes

dmytroshytyi-6WIND added 2 commits August 21, 2024 15:27

dmytroshytyi-6WIND force-pushed the fix_bgp_open_notification_snmp branch from 9360fbc to e23005f Compare August 21, 2024 13:28

ton31337 approved these changes Aug 21, 2024

View reviewed changes

ton31337 merged commit 0db6045 into FRRouting:master Aug 22, 2024
10 of 11 checks passed

ton31337 added a commit that referenced this pull request Aug 22, 2024

Merge pull request #16622 from FRRouting/mergify/bp/stable/10.1/pr-16546

d9bf612

bgpd: fix, do not access peer->notify.data when it is null (backport #16546)

ton31337 added a commit that referenced this pull request Aug 22, 2024

Merge pull request #16624 from FRRouting/mergify/bp/stable/9.1/pr-16546

ee98186

bgpd: fix, do not access peer->notify.data when it is null (backport #16546)

donaldsharp added a commit that referenced this pull request Aug 22, 2024

Merge pull request #16625 from FRRouting/mergify/bp/stable/9.0/pr-16546

b2be78e

bgpd: fix, do not access peer->notify.data when it is null (backport #16546)

donaldsharp added a commit that referenced this pull request Aug 22, 2024

Merge pull request #16623 from FRRouting/mergify/bp/stable/10.0/pr-16546

a23ef65

bgpd: fix, do not access peer->notify.data when it is null (backport #16546)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bgpd: fix, do not access peer->notify.data when it is null #16546

bgpd: fix, do not access peer->notify.data when it is null #16546

dmytroshytyi-6WIND commented Aug 9, 2024 •

edited

Loading

dmytroshytyi-6WIND commented Aug 9, 2024

ton31337 left a comment

ton31337 Aug 9, 2024

dmytroshytyi-6WIND Aug 12, 2024

ton31337 left a comment

ton31337 Aug 12, 2024

dmytroshytyi-6WIND Aug 14, 2024

ton31337 Aug 12, 2024

dmytroshytyi-6WIND Aug 14, 2024

ton31337 commented Aug 12, 2024

mergify bot commented Aug 12, 2024 •

edited

Loading

ton31337 Aug 15, 2024

dmytroshytyi-6WIND Aug 21, 2024

ton31337 Aug 15, 2024

dmytroshytyi-6WIND Aug 21, 2024

ton31337 left a comment


		output = json.loads(r2.vtysh_cmd("show bgp summary json"))

		if output["ipv4Unicast"]["peers"]["192.168.12.4"]["state"] == "Established":

bgpd: fix, do not access peer->notify.data when it is null #16546

bgpd: fix, do not access peer->notify.data when it is null #16546

Conversation

dmytroshytyi-6WIND commented Aug 9, 2024 • edited Loading

[EDIT1] -> reduce the configs as @ton31337 requested config rt2-bgp:

config rt2-zebra:

config rr-bgpd:

config rr-zebra:

dmytroshytyi-6WIND commented Aug 9, 2024

ton31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ton31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ton31337 commented Aug 12, 2024

mergify bot commented Aug 12, 2024 • edited Loading

✅ Backports have been created

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ton31337 left a comment

Choose a reason for hiding this comment

dmytroshytyi-6WIND commented Aug 9, 2024 •

edited

Loading

[EDIT1] -> reduce the configs as @ton31337 requested
config rt2-bgp:

mergify bot commented Aug 12, 2024 •

edited

Loading