Page MenuHomeVyOS Platform
Create Task
Maniphest T9502

accel-ppp: VPP crashes (SIGBUS in policer_input_node_fn) when sessions with a rate limit end
Open, HighPublicBUG
Actions

Description

With the VPP dataplane for PPPoE and a RADIUS rate limit, VPP can crash with SIGBUS in policer_input_node_fn_hsw when sessions end. VPP restarts and every session is dropped.

Seen in T9211 by a tester on a fresh rolling VM (VPP 25.10.0, FreeRADIUS, 2 real CPEs and about 1,700 emulated clients, about 1,000 sessions connecting within a minute):

  • SIGBUS in policer_input_node_fn_hsw at 13:44:52 UTC, VPP restarted, all sessions dropped
  • a second VPP restart at 14:41:22 UTC with no configuration change

I have not reproduced the crash. This is from reading the code, it is a likely cause and not confirmed.

vpppolicer_remove_limiter() in accel-ppp-ng (accel-pppd/session_hooks/vyos_vpp/vpppolicer.c) does not undo vpppolicer_install_limiter() in reverse order. Installing binds the input policer to the interface and then adds the policer-input node to the ip4-unicast, ip6-unicast, ip4-multicast and ip6-multicast feature arcs. Removing unbinds and deletes the input policer first and takes the node out of the arcs afterwards. In between, packets of the interface can still reach policer_input_node_fn while the policer it uses is gone.

Fix proposed by the tester and prepared as a branch: remove the policer-input features first, then unbind and delete the policer (the reverse of the install order). The output policer needs no change.

What is still open:

  • The change is untested. The window is small and a few sessions are not enough to hit it, so it needs a test with many sessions that end at once while traffic flows.
  • It is not known if the order alone is enough, or if deleting a policer while packets are in flight needs more.

Related: T9211 (found there), T9499 (the abort of accel-pppd when a VPP session ends, a different problem).

Details

Version
rolling
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Related Objects

Event Timeline

rherold created this task.Thu, Oct 22, 3:50 PM
rherold created this object in space S1 VyOS Public.
rherold added a comment.Thu, Oct 22, 4:04 PM
Branch with the change, based on the accel-ppp-ng version that rolling builds (dd07b23):
https://github.com/ruben-herold/accel-ppp-ng/commit/3c44e19 (branch `fix-vpp-policer-remove-order`)

It only moves the four `vpppoe_set_feature(..., 0, "policer-input", ...)` calls in `vpppolicer_remove_limiter()` in front of `vpppolicer_set_input(0, ...)` and the delete 
of the policer, the reverse of `vpppolicer_install_limiter()`.

Status: not confirmed. I have not reproduced the crash. A test build with this change on top of the build of T9211 is in the task T9211 for the tester, who has the load   
for it. A PR to accel-ppp-ng follows when it is confirmed.
Viacheslav raised the priority of this task from Low to High.Thu, Oct 22, 4:04 PM
Viacheslav changed the subtype of this task from "Task" to "Bug".
pasik subscribed.Thu, Oct 22, 4:05 PM
rherold mentioned this in T9211: VPP PPPoE with RADIUS Idle-Timeout: sessions drop every 60 s, accounting reports 0 bytes used.Thu, Oct 22, 4:21 PM

Test result so far: on a VM with three sessions with a RADIUS rate limit (VPP policers in use) and a ping flood on each, ending all sessions and reconnecting 15 times, VPP did not crash with the old order and not with the new order, and no policer was left behind after any round with the new order. With three sessions the window is too small to hit, so this does not show that the new order is the fix, only that it removes the policers cleanly. A build with the change is in the task T9211 for the tester, who has the load for it.

rherold added a subscriber: saadinnocent909.Thu, Oct 22, 4:31 PM

@saadinnocent909 (a mention subscribes you to this task) for your answers about crashes see https://vyos.dev/T9211, where I ask about the backtrace, the NAT44 output-feature workaround and the policer. The crash of T9501 (NAT44, nat_pre_in2out_output) is a different one from the crash of this task (policer_input_node_fn_hsw). T9211 reports a second VPP restart (14:41:22 UTC) without a cause, which could be either of them. The hook that creates and deletes the session interface is where the policer features are removed here and where marking each pppoe_sessionN as NAT inside (option 1 of T9501) would go, so if that is done the removal has to be the reverse of the setup from the start.

rherold added a comment.Fri, Oct 23, 4:05 PM

The change is open as a pull request: https://github.com/accel-ppp/accel-ppp-ng/pull/46

It removes the four policer-input features from the ip4 and ip6 feature arcs first, and then unbinds and deletes the input policer, the reverse of the install order. Nothing else is changed.

I could not reproduce the SIGBUS myself, with 60 sessions and a RADIUS rate limit, all ended at once, eight rounds, neither with the old order nor with the new one. What I can show is that the new order removes the policers cleanly under that load. The test of the tester of https://vyos.dev/T9211 is the one that supports it: about 2,000 sessions, user traffic of up to about 1 Gbit/s, 13 resets of one user, 6 CoA rate changes under traffic, Disconnect and reset pppoe-server all several times, a full day without a VPP crash, with the old order the tester had seen the SIGBUS. The PR says that one day without a crash is consistent with the fix but not a proof.

saadinnocent909 added a comment.Fri, Oct 23, 7:06 PM

I will check this tomorrow. By the way I am that 2000 session tester

rherold added a comment.Fri, Oct 23, 8:31 PM

@saadinnocent909

Quick heads-up: I’ll be attending an event next week (RIPE93 in Sofia) , but I’ll still be working on the tickets as usual, so no worries about any major delays.
On a side note, I’ve been really enjoying our exchanges and would love to learn a bit more about your specific use case outside of this ticket system whenever you have some free time.
If you’re open to a quick off-topic chat, feel free to reach out to me via email at (ruben at insecure . pw)

saadinnocent909 added a comment.Sat, Oct 24, 2:34 PM

Reviewed PR #46: it is the same reorder I tested in v6 (policer-input removed from the four ip4/ip6 arcs before unbind/delete). Over ~2000 sessions with ~1 Gbit/s policed traffic, 13 resets, 6 CoA rate changes under traffic, Disconnect and reset-all, VPP did not crash (NRestarts 0). Note: the second restart on 10-22 at 14:41:14 UTC was also SIGSEGV in policer_input_node_fn_hsw (not NAT), so both 10-22 crashes match T9502. Happy to run more churn rounds on the merged build.

rherold mentioned this in T9501: VPP: NAT44 does not translate traffic of PPPoE server sessions (pppoe_sessionN is not an inside interface).Sat, Oct 24, 4:39 PM