Fix use-after-free when destroying interfaces on shutdown #439
Conversation
rjarry
force-pushed
the
control-output-fini
branch
from
7236ec3 to
0f9b358
Compare
📝 WalkthroughWalkthroughAdds a 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{c,h}⚙️ CodeRabbit configuration file
Files:
**/*.sh⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (6)📚 Learning: 2025-09-22T09:21:51.749ZApplied to files:
📚 Learning: 2025-09-25T07:52:17.403ZApplied to files:
📚 Learning: 2025-09-22T09:21:51.749ZApplied to files:
📚 Learning: 2025-11-05T13:55:26.189ZApplied to files:
📚 Learning: 2025-09-05T07:06:51.554ZApplied to files:
📚 Learning: 2025-10-13T08:37:56.819ZApplied to files:
🧬 Code graph analysis (1)smoke/shutdown_test.sh (1)
🪛 Shellcheck (0.11.0)smoke/shutdown_test.sh[warning] 5-5: Quote this to prevent word splitting. (SC2046) [warning] 34-34: grout_pid is referenced but not assigned. (SC2154) [warning] 38-38: tmp is referenced but not assigned. (SC2154) 🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Explain ? |
See top level description |
|
Retag 14.0 ? |
|
No, I will tag a 0.14.1 once this is merged. |
rjarry
force-pushed
the
control-output-fini
branch
from
0f9b358 to
c8d5852
Compare
|
Tested-By: Christophe Fontaine [email protected] |
grout-bot
force-pushed
the
control-output-fini
branch
from
c8d5852 to
3d37a22
Compare
grout-bot
force-pushed
the
control-output-fini
branch
from
3d37a22 to
9b85f3c
Compare
rjarry
force-pushed
the
control-output-fini
branch
from
9b85f3c to
3e1deea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
smoke/shutdown_test.sh (1)
5-5: Quote the command substitution.To prevent potential word splitting, apply this fix:
-. $(dirname $0)/_init.sh +. "$(dirname $0)"/_init.sh
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
main/module.c(2 hunks)modules/infra/control/control_output.c(1 hunks)modules/infra/control/graph.c(0 hunks)modules/infra/control/iface.c(1 hunks)modules/infra/control/nexthop.c(1 hunks)smoke/_init.sh(1 hunks)smoke/shutdown_test.sh(1 hunks)
💤 Files with no reviewable changes (1)
- modules/infra/control/graph.c
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/infra/control/nexthop.c
- modules/infra/control/iface.c
🧰 Additional context used
📓 Path-based instructions (2)
**/*.sh
⚙️ CodeRabbit configuration file
**/*.sh: - Don't bother about unquoted shell variables.
Files:
smoke/_init.shsmoke/shutdown_test.sh
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Never suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/module.cmodules/infra/control/control_output.c
🧠 Learnings (2)
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Applied to files:
smoke/_init.shsmoke/shutdown_test.sh
📚 Learning: 2025-10-13T08:37:56.819Z
Learnt from: abhiramnarayana
Repo: DPDK/grout PR: 337
File: docs/grcli-dnat44.1.md:42-56
Timestamp: 2025-10-13T08:37:56.819Z
Learning: In the grout project's man pages, the EXAMPLES sections omit the `grcli` prefix from commands for brevity and readability, showing commands as if executed within a grcli context (e.g., `address show` instead of `grcli address show`). The full command structure with `grcli` is documented in the SYNOPSIS section.
Applied to files:
smoke/shutdown_test.sh
🧬 Code graph analysis (1)
smoke/shutdown_test.sh (1)
smoke/_init.sh (1)
fail(98-101)
🪛 Shellcheck (0.11.0)
smoke/shutdown_test.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
[warning] 34-34: grout_pid is referenced but not assigned.
(SC2154)
[warning] 38-38: tmp is referenced but not assigned.
(SC2154)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (4)
modules/infra/control/control_output.c (1)
130-135: Dependency change correctly fixes the finalization order.The switch from
"*"to"graph"combined with iface/nexthop now depending oncontrol_outputensures that interfaces and nexthops are finalized beforectrlout_ringis freed, preventing the use-after-free.main/module.c (1)
62-80: Implementation correctly enables comma-separated dependencies.The tokenization logic properly splits
depends_onon commas and checks each token viafnmatch. The local buffer copy avoids modifying the original constant string. This enables the multi-dependency declarations needed for the fix (e.g.,"*route,control_output").smoke/_init.sh (1)
45-58: Good defensive handling of socket availability during cleanup.Wrapping interface deletion in a socket connectivity check ensures cleanup proceeds gracefully whether grout is still running or has crashed, aligning with the PR's shutdown robustness goal.
smoke/shutdown_test.sh (1)
1-38: Test logic is sound for the graceful shutdown scenario.The script correctly sets up state, sends a shutdown signal to grout, and strips grcli commands from cleanup to avoid post-shutdown invocations. However, note that this test assumes grout was started by
_init.sh(i.e.,run_grout=true). If run against an external grout instance whererun_grout=false, the test will fail becausegrout_pidwon't be defined at line 34. This may be acceptable by design, but consider whether an explicit guard or skip condition would improve robustness.
This reverts commit 2f97bb1. Signed-off-by: Robin Jarry <[email protected]> Tested-by: Christophe Fontaine <[email protected]> Reviewed-by: Maxime Leroy <[email protected]>
Allow specifying multiple patterns separated by commas in the gr_module.depends_on string. Signed-off-by: Robin Jarry <[email protected]> Tested-by: Christophe Fontaine <[email protected]> Reviewed-by: Maxime Leroy <[email protected]>
Since commit e954453 ("control-output: prevent use-after-free on object deletion"), deleting interfaces or nexthops causes a call to control_output_poll() in order to drain the packets that may reference the deleted object. On shutdown, the modules finalization order causes control_output to be finalized before the iface module. When the iface module is finalized, all remaining interfaces are deleted and control_output_poll() accesses the ring that was already "freed". ERROR: AddressSanitizer: heap-use-after-free on address 0x00016d19d608 at pc 0x00000045901c bp 0xffffca318bd0 sp 0xffffca318be8 READ of size 4 at 0x00016d19d608 thread T0 #0 0x000000459018 in rte_ring_dequeue_bulk_elem ../subprojects/dpdk/lib/ring/rte_ring_elem.h:375 DPDK#1 0x000000459018 in rte_ring_dequeue_elem ../subprojects/dpdk/lib/ring/rte_ring_elem.h:471 DPDK#2 0x000000459018 in rte_ring_dequeue ../subprojects/dpdk/lib/ring/rte_ring.h:496 DPDK#3 0x000000459018 in control_output_poll ../modules/infra/control/control_output.c:33 DPDK#4 0x0000004648a0 in event_handler ../modules/infra/control/control_output.c:76 DPDK#5 0x000000416974 in gr_event_push ../main/event.c:26 DPDK#6 0x00000046a930 in iface_destroy ../modules/infra/control/iface.c:424 DPDK#7 0x00000046b138 in iface_fini ../modules/infra/control/iface.c:462 DPDK#8 0x00000041d63c in modules_fini ../main/module.c:112 DPDK#9 0x00000041b5a4 in main ../main/main.c:326 Ensure both iface and nexthop are finalized before control_output. Fixes: e954453 ("control-output: prevent use-after-free on object deletion") Signed-off-by: Robin Jarry <[email protected]> Tested-by: Christophe Fontaine <[email protected]> Reviewed-by: Maxime Leroy <[email protected]>
Configure a bunch of stuff and shutdown grout without clearing the stuff first. Ensure grout shuts down without issues. In the cleanup() function, only delete interfaces if grout is still listening to the api socket. This avoids spurious error messages during cleanup. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Maxime Leroy <[email protected]>
grout-bot
force-pushed
the
control-output-fini
branch
from
3e1deea to
25cea4c
Compare
3 participants
Since commit e954453, deleting interfaces or nexthops causes a call to control_output_poll() in order to drain the packets that may reference the deleted object.
On shutdown, the modules finalization order causes control_output to be finalized before the iface module. When the iface module is finalized, all remaining interfaces are deleted and control_output_poll() accesses the ring that was already "freed".
Ensure both iface and nexthop are finalized before control_output.
Fixes: e954453