Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bgpd: fix as-path exclude modify crash #16779

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

louis-6wind
Copy link
Contributor

Fix a crash when modifying a route-map with set as-path exclude without as-path-access-list:

router(config)# route-map routemaptest deny 1
router(config-route-map)# set as-path exclude 33 34 35
router(config-route-map)# set as-path exclude as-path-access-list test

#0 raise (sig=) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x00007fb3959327de in core_handler (signo=11, siginfo=0x7ffd122da530, context=0x7ffd122da400) at lib/sigevent.c:258
#2
#3 0x000055ab2762a1bd in as_list_list_del (h=0x55ab27897680 <as_exclude_list_orphan>, item=0x55ab28204e20) at ./bgpd/bgp_aspath.h:77
#4 0x000055ab2762d1a8 in as_exclude_remove_orphan (ase=0x55ab28204e20) at bgpd/bgp_aspath.c:1574
#5 0x000055ab27550538 in route_aspath_exclude_free (rule=0x55ab28204e20) at bgpd/bgp_routemap.c:2366
#6 0x00007fb39591f00c in route_map_rule_delete (list=0x55ab28203498, rule=0x55ab28204170) at lib/routemap.c:1357
#7 0x00007fb39591f87c in route_map_add_set (index=0x55ab28203460, set_name=0x55ab276ad2aa "as-path exclude", set_arg=0x55ab281e4f70 "as-path-access-list test") at lib/routemap.c:1674
#8 0x00007fb39591d3f3 in generic_set_add (index=0x55ab28203460, command=0x55ab276ad2aa "as-path exclude", arg=0x55ab281e4f70 "as-path-access-list test", errmsg=0x7ffd122db870 "",
errmsg_len=8192) at lib/routemap.c:533
#9 0x000055ab2755e78e in lib_route_map_entry_set_action_rmap_set_action_exclude_as_path_modify (args=0x7ffd122db290) at bgpd/bgp_routemap_nb_config.c:2427
#10 0x00007fb3958fe417 in nb_callback_modify (context=0x55ab28205aa0, nb_node=0x55ab27cb31e0, event=NB_EV_APPLY, dnode=0x55ab28202690, resource=0x55ab27c32148, errmsg=0x7ffd122db870 "",
errmsg_len=8192) at lib/northbound.c:1538
#11 0x00007fb3958ff0ab in nb_callback_configuration (context=0x55ab28205aa0, event=NB_EV_APPLY, change=0x55ab27c32110, errmsg=0x7ffd122db870 "", errmsg_len=8192) at lib/northbound.c:1888
#12 0x00007fb3958ff5e4 in nb_transaction_process (event=NB_EV_APPLY, transaction=0x55ab28205aa0, errmsg=0x7ffd122db870 "", errmsg_len=8192) at lib/northbound.c:2016
#13 0x00007fb3958fddba in nb_candidate_commit_apply (transaction=0x55ab28205aa0, save_transaction=true, transaction_id=0x0, errmsg=0x7ffd122db870 "", errmsg_len=8192)
at lib/northbound.c:1356
#14 0x00007fb3958fdef0 in nb_candidate_commit (context=..., candidate=0x55ab27c2c9a0, save_transaction=true, comment=0x0, transaction_id=0x0, errmsg=0x7ffd122db870 "", errmsg_len=8192)
at lib/northbound.c:1389
#15 0x00007fb3959045ba in nb_cli_classic_commit (vty=0x55ab281f6680) at lib/northbound_cli.c:57
#16 0x00007fb395904b5a in nb_cli_apply_changes_internal (vty=0x55ab281f6680, xpath_base=0x7ffd122dfd10 "/frr-route-map:lib/route-map[name='routemaptest']/entry[sequence='1']",
clear_pending=false) at lib/northbound_cli.c:184
#17 0x00007fb395904ebf in nb_cli_apply_changes (vty=0x55ab281f6680, xpath_base_fmt=0x0) at lib/northbound_cli.c:240
--Type for more, q to quit, c to continue without paging--
#18 0x000055ab27557d2e in set_aspath_exclude_access_list_magic (self=0x55ab2775c300 <set_aspath_exclude_access_list_cmd>, vty=0x55ab281f6680, argc=5, argv=0x55ab28204c80,
as_path_filter_name=0x55ab28202040 "test") at bgpd/bgp_routemap.c:6397
#19 0x000055ab2754bdea in set_aspath_exclude_access_list (self=0x55ab2775c300 <set_aspath_exclude_access_list_cmd>, vty=0x55ab281f6680, argc=5, argv=0x55ab28204c80)
at ./bgpd/bgp_routemap_clippy.c:856
#20 0x00007fb39589435d in cmd_execute_command_real (vline=0x55ab281e61f0, vty=0x55ab281f6680, cmd=0x0, up_level=0) at lib/command.c:1003
#21 0x00007fb3958944be in cmd_execute_command (vline=0x55ab281e61f0, vty=0x55ab281f6680, cmd=0x0, vtysh=0) at lib/command.c:1062
#22 0x00007fb395894a0c in cmd_execute (vty=0x55ab281f6680, cmd=0x55ab28200f20 "set as-path exclude as-path-access-list test", matched=0x0, vtysh=0) at lib/command.c:1228
#23 0x00007fb39595242c in vty_command (vty=0x55ab281f6680, buf=0x55ab28200f20 "set as-path exclude as-path-access-list test") at lib/vty.c:625
#24 0x00007fb3959541c5 in vty_execute (vty=0x55ab281f6680) at lib/vty.c:1388
#25 0x00007fb3959563db in vtysh_read (thread=0x7ffd122e2bb0) at lib/vty.c:2400
#26 0x00007fb39594b785 in event_call (thread=0x7ffd122e2bb0) at lib/event.c:1996
#27 0x00007fb3958d1365 in frr_run (master=0x55ab27b56d70) at lib/libfrr.c:1231
#28 0x000055ab2747f1cc in main (argc=3, argv=0x7ffd122e2e08) at bgpd/bgp_main.c:555

Fixes: 094dcc3 ("bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issues")

Fix a crash when modifying a route-map with set as-path exclude without
as-path-access-list:

> router(config)# route-map routemaptest deny 1
> router(config-route-map)# set as-path exclude 33 34 35
> router(config-route-map)# set as-path exclude as-path-access-list test

> #0  raise (sig=<optimized out>) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1  0x00007fb3959327de in core_handler (signo=11, siginfo=0x7ffd122da530, context=0x7ffd122da400) at lib/sigevent.c:258
> #2  <signal handler called>
> #3  0x000055ab2762a1bd in as_list_list_del (h=0x55ab27897680 <as_exclude_list_orphan>, item=0x55ab28204e20) at ./bgpd/bgp_aspath.h:77
> FRRouting#4  0x000055ab2762d1a8 in as_exclude_remove_orphan (ase=0x55ab28204e20) at bgpd/bgp_aspath.c:1574
> FRRouting#5  0x000055ab27550538 in route_aspath_exclude_free (rule=0x55ab28204e20) at bgpd/bgp_routemap.c:2366
> FRRouting#6  0x00007fb39591f00c in route_map_rule_delete (list=0x55ab28203498, rule=0x55ab28204170) at lib/routemap.c:1357
> FRRouting#7  0x00007fb39591f87c in route_map_add_set (index=0x55ab28203460, set_name=0x55ab276ad2aa "as-path exclude", set_arg=0x55ab281e4f70 "as-path-access-list test") at lib/routemap.c:1674
> FRRouting#8  0x00007fb39591d3f3 in generic_set_add (index=0x55ab28203460, command=0x55ab276ad2aa "as-path exclude", arg=0x55ab281e4f70 "as-path-access-list test", errmsg=0x7ffd122db870 "",
>     errmsg_len=8192) at lib/routemap.c:533
> FRRouting#9  0x000055ab2755e78e in lib_route_map_entry_set_action_rmap_set_action_exclude_as_path_modify (args=0x7ffd122db290) at bgpd/bgp_routemap_nb_config.c:2427
> FRRouting#10 0x00007fb3958fe417 in nb_callback_modify (context=0x55ab28205aa0, nb_node=0x55ab27cb31e0, event=NB_EV_APPLY, dnode=0x55ab28202690, resource=0x55ab27c32148, errmsg=0x7ffd122db870 "",
>     errmsg_len=8192) at lib/northbound.c:1538
> FRRouting#11 0x00007fb3958ff0ab in nb_callback_configuration (context=0x55ab28205aa0, event=NB_EV_APPLY, change=0x55ab27c32110, errmsg=0x7ffd122db870 "", errmsg_len=8192) at lib/northbound.c:1888
> FRRouting#12 0x00007fb3958ff5e4 in nb_transaction_process (event=NB_EV_APPLY, transaction=0x55ab28205aa0, errmsg=0x7ffd122db870 "", errmsg_len=8192) at lib/northbound.c:2016
> FRRouting#13 0x00007fb3958fddba in nb_candidate_commit_apply (transaction=0x55ab28205aa0, save_transaction=true, transaction_id=0x0, errmsg=0x7ffd122db870 "", errmsg_len=8192)
>     at lib/northbound.c:1356
> FRRouting#14 0x00007fb3958fdef0 in nb_candidate_commit (context=..., candidate=0x55ab27c2c9a0, save_transaction=true, comment=0x0, transaction_id=0x0, errmsg=0x7ffd122db870 "", errmsg_len=8192)
>     at lib/northbound.c:1389
> FRRouting#15 0x00007fb3959045ba in nb_cli_classic_commit (vty=0x55ab281f6680) at lib/northbound_cli.c:57
> FRRouting#16 0x00007fb395904b5a in nb_cli_apply_changes_internal (vty=0x55ab281f6680, xpath_base=0x7ffd122dfd10 "/frr-route-map:lib/route-map[name='routemaptest']/entry[sequence='1']",
>     clear_pending=false) at lib/northbound_cli.c:184
> FRRouting#17 0x00007fb395904ebf in nb_cli_apply_changes (vty=0x55ab281f6680, xpath_base_fmt=0x0) at lib/northbound_cli.c:240
> --Type <RET> for more, q to quit, c to continue without paging--
> FRRouting#18 0x000055ab27557d2e in set_aspath_exclude_access_list_magic (self=0x55ab2775c300 <set_aspath_exclude_access_list_cmd>, vty=0x55ab281f6680, argc=5, argv=0x55ab28204c80,
>     as_path_filter_name=0x55ab28202040 "test") at bgpd/bgp_routemap.c:6397
> FRRouting#19 0x000055ab2754bdea in set_aspath_exclude_access_list (self=0x55ab2775c300 <set_aspath_exclude_access_list_cmd>, vty=0x55ab281f6680, argc=5, argv=0x55ab28204c80)
>     at ./bgpd/bgp_routemap_clippy.c:856
> FRRouting#20 0x00007fb39589435d in cmd_execute_command_real (vline=0x55ab281e61f0, vty=0x55ab281f6680, cmd=0x0, up_level=0) at lib/command.c:1003
> FRRouting#21 0x00007fb3958944be in cmd_execute_command (vline=0x55ab281e61f0, vty=0x55ab281f6680, cmd=0x0, vtysh=0) at lib/command.c:1062
> FRRouting#22 0x00007fb395894a0c in cmd_execute (vty=0x55ab281f6680, cmd=0x55ab28200f20 "set as-path exclude as-path-access-list test", matched=0x0, vtysh=0) at lib/command.c:1228
> FRRouting#23 0x00007fb39595242c in vty_command (vty=0x55ab281f6680, buf=0x55ab28200f20 "set as-path exclude as-path-access-list test") at lib/vty.c:625
> FRRouting#24 0x00007fb3959541c5 in vty_execute (vty=0x55ab281f6680) at lib/vty.c:1388
> FRRouting#25 0x00007fb3959563db in vtysh_read (thread=0x7ffd122e2bb0) at lib/vty.c:2400
> FRRouting#26 0x00007fb39594b785 in event_call (thread=0x7ffd122e2bb0) at lib/event.c:1996
> FRRouting#27 0x00007fb3958d1365 in frr_run (master=0x55ab27b56d70) at lib/libfrr.c:1231
> FRRouting#28 0x000055ab2747f1cc in main (argc=3, argv=0x7ffd122e2e08) at bgpd/bgp_main.c:555

Fixes: 094dcc3 ("bgpd: fix "bgp as-pah access-list" with "set aspath exclude" set/unset issues")
Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
@ton31337
Copy link
Member

@Mergifyio backport stable/10.1 stable/10.0

Copy link

mergify bot commented Sep 10, 2024

backport stable/10.1 stable/10.0

✅ Backports have been created

Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a topotest exactly to test this case?

Check that the following does not cause a crash:

> route-map r2 permit 6
>  set as-path exclude 65555
>  set as-path exclude as-path-access-list NON-EXISTING

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
@frrbot frrbot bot added the tests Topotests, make check, etc label Sep 10, 2024
@github-actions github-actions bot added size/S and removed size/XS labels Sep 10, 2024
@louis-6wind
Copy link
Contributor Author

Can we have a topotest exactly to test this case?

done

Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good ... waiting on the one comment about the topo test timing

No need to do 'no set as-path exclude' to replace the current rule by
another. The code is supposed to support the replacement.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
Extend timers in bgp_set_aspath_exclude in case of high load on the CI.

Signed-off-by: Louis Scalbert <louis.scalbert@6wind.com>
@github-actions github-actions bot added size/M and removed size/S labels Sep 11, 2024
@ton31337 ton31337 merged commit b916ed3 into FRRouting:master Sep 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants