dhcp: fixes, continued - #484
Conversation
The arp_input_request and arp_input_reply nodes were calling gr_mbuf_trace_add() without capturing any packet data. Copy the ARP header into the trace buffer and register trace_arp_format as the format callback for both nodes so that ARP traces display useful information. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Move the DHCP UDP port registration from dhcp_init() to the node's register_callback. This is consistent with how other datapath nodes register their protocol handlers and keeps the registration logic co-located with the node definition. The function is now static since it no longer needs to be exported to the control plane. Signed-off-by: Robin Jarry <rjarry@redhat.com>
📝 WalkthroughWalkthroughAdds a public bool dhcp_enabled(uint16_t iface_id) and changes dhcp_input_register_port to static (internal). DHCP input node gains headroom-aware handling and per-mbuf interface-aware routing: if DHCP is enabled for the iface it follows control-plane path, otherwise Ethernet/IP headers are reconstructed and sent to iface TX; a NO_HEADROOM edge was added. iface_cp_tx declaration moved from ctlplane.h (removed) to gr_rxtx.h and includes updated. ARP request/reply nodes capture ARP headers early for tracing and expose a trace_format callback. 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 |
When DHCP is not enabled on an interface, forward incoming DHCP packets to the control plane interface instead of processing and dropping them with an error log message. This allows userspace applications listening on UDP port 68 to receive the packets. Since the dhcp_input node receives packets after IP header processing, reconstruct the IP and Ethernet headers before sending to the control plane interface via iface_cp_tx. Move the iface_cp_tx declaration from the private ctlplane.h header to the public gr_rxtx.h so it can be used by the DHCP datapath node. Delete ctlplane.h since it is no longer needed. Signed-off-by: Robin Jarry <rjarry@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@modules/dhcp/control/client.c`:
- Around line 32-35: dhcp_enabled currently reads dhcp_clients[] without
synchronization causing a data race; change dhcp_enabled() to load the pointer
with atomic_load_explicit(&dhcp_clients[iface_id], memory_order_acquire) and
compare against NULL, and modify dhcp_start() to publish the new client with
atomic_store_explicit(&dhcp_clients[iface_id], client, memory_order_release); in
dhcp_stop() set the array slot to NULL with atomic_store_explicit before freeing
the client, then call rte_rcu_qsbr_synchronize(gr_datapath_rcu(),
RTE_QSBR_THRID_INVALID) to wait for readers (RCU) and only then free the client
to match the conntrack/nexthop patterns.
| bool dhcp_enabled(uint16_t iface_id) { | ||
| if (iface_id < MAX_IFACES) | ||
| return dhcp_clients[iface_id] != NULL; | ||
| return false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and understand the structure
find . -type f -name "client.c" | head -20Repository: DPDK/grout
Length of output: 85
🏁 Script executed:
# Look at the directory structure for DHCP module
find . -type d -name "dhcp" | head -5Repository: DPDK/grout
Length of output: 68
🏁 Script executed:
# Search for dhcp_enabled() usage
rg "dhcp_enabled" --type cRepository: DPDK/grout
Length of output: 258
🏁 Script executed:
# Examine the client.c file structure
wc -l modules/dhcp/control/client.cRepository: DPDK/grout
Length of output: 87
🏁 Script executed:
# Read the client.c file to understand synchronization and callers
head -100 modules/dhcp/control/client.cRepository: DPDK/grout
Length of output: 2524
🏁 Script executed:
# Search for dhcp_start and dhcp_stop implementations
rg "dhcp_start|dhcp_stop" --type c -A 10 | head -100Repository: DPDK/grout
Length of output: 4969
🏁 Script executed:
# Look for threading/synchronization primitives in the codebase
rg "pthread|mutex|lock|atomic" modules/dhcp/ --type cRepository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Check the full context of dhcp_enabled() call in the datapath
rg "dhcp_enabled" -B 10 -A 10 modules/dhcp/datapath/dhcp_input.cRepository: DPDK/grout
Length of output: 588
🏁 Script executed:
# Look at the dhcp_input.c file structure
wc -l modules/dhcp/datapath/dhcp_input.cRepository: DPDK/grout
Length of output: 92
🏁 Script executed:
# Search for control input processing and threading model
rg "gr_control_input|gr_control_queue" --type c -B 3 -A 3 | head -80Repository: DPDK/grout
Length of output: 4072
🏁 Script executed:
# Look for any synchronization or thread-safe patterns in gr_api or control modules
find . -type f -name "*.h" -path "*/api/*" -o -name "*control*" | grep -E "\.h$" | head -10Repository: DPDK/grout
Length of output: 254
🏁 Script executed:
# Check if there's documentation or design notes about threading
find . -type f \( -name "README*" -o -name "*.md" -o -name "DESIGN*" \) | head -10Repository: DPDK/grout
Length of output: 113
🏁 Script executed:
# Examine how the control API handlers are invoked
rg "api_ctx|api_out" --type c | head -30Repository: DPDK/grout
Length of output: 2335
🏁 Script executed:
# Check the gr_control_input header to understand synchronization
find . -name "gr_control_input.h" -type fRepository: DPDK/grout
Length of output: 97
🏁 Script executed:
# Look at how other modules synchronize datapath/control access
rg "dhcp_clients\[" modules/dhcp/ --type c -B 2 -A 2Repository: DPDK/grout
Length of output: 2834
🏁 Script executed:
# Look for the full implementation of dhcp_start and dhcp_stop
cat -n modules/dhcp/control/client.c | sed -n '300,400p'Repository: DPDK/grout
Length of output: 3375
🏁 Script executed:
# Get the actual dhcp_start and dhcp_stop functions
rg "^static int dhcp_start" -A 30 modules/dhcp/control/client.cRepository: DPDK/grout
Length of output: 804
🏁 Script executed:
# Get dhcp_stop function
rg "^static int dhcp_stop" -A 30 modules/dhcp/control/client.cRepository: DPDK/grout
Length of output: 722
🏁 Script executed:
# Check if there are any memory barriers or synchronization in the datapath
rg "barrier|fence|atomic|sync" modules/dhcp/datapath/ --type cRepository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Look for the README to understand the overall architecture
head -100 README.mdRepository: DPDK/grout
Length of output: 3021
🏁 Script executed:
# Look at the gr_control_input.h header
cat -n modules/infra/datapath/gr_control_input.h | head -100Repository: DPDK/grout
Length of output: 519
🏁 Script executed:
# Check if there's any documentation on control/datapath synchronization
rg "control.*plane|datapath" README.md CONTRIBUTING.md -iRepository: DPDK/grout
Length of output: 513
🏁 Script executed:
# Look for how other modules handle control/datapath access
rg "static.*\*.*\[" modules/ --type c | grep -v "dhcp" | head -20Repository: DPDK/grout
Length of output: 463
🏁 Script executed:
# Check if there are other examples of shared state between control and datapath
rg "\.py|\.lua|\.c" devtools/ai-instructions.mdRepository: DPDK/grout
Length of output: 224
🏁 Script executed:
# Look at the full AI instructions for context
cat devtools/ai-instructions.mdRepository: DPDK/grout
Length of output: 4088
🏁 Script executed:
# Check if there's any message passing or event-based synchronization
rg "post_to_stack|gr_control_input_register" modules/dhcp/ -B 3 -A 3 --type cRepository: DPDK/grout
Length of output: 1150
🏁 Script executed:
# Look for any use of atomic operations or synchronization in similar modules
find modules/ -name "*.c" -type f -exec grep -l "atomic\|mutex\|lock\|barrier" {} \;Repository: DPDK/grout
Length of output: 832
🏁 Script executed:
# Check if API handlers are serialized by looking at how they're invoked
rg "api_ctx|api_out|api_handler" api/gr_api.h -A 5 -B 2Repository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Look at the main datapath loop to understand concurrency
find . -name "*datapath*loop*" -o -name "*loop*" | grep -E "\.(c|h)$" | head -5Repository: DPDK/grout
Length of output: 234
🏁 Script executed:
# Check how control plane API invocation works
rg "gr_api.*handler|api_handler" --type h | head -20Repository: DPDK/grout
Length of output: 464
🏁 Script executed:
# Find where API handlers are actually invoked
rg "gr_api_handler|callback" main/ --type c -B 3 -A 5 | head -80Repository: DPDK/grout
Length of output: 3440
🏁 Script executed:
# Look at main_loop to understand concurrency model
cat -n modules/infra/datapath/main_loop.c | head -80Repository: DPDK/grout
Length of output: 2429
🏁 Script executed:
# Check the control plane worker to see how requests are processed
cat -n modules/infra/control/worker.c | head -100Repository: DPDK/grout
Length of output: 3158
🏁 Script executed:
# Look for event_base usage and threading model
rg "event_base|thread|worker" modules/infra/control/worker.c --type c -B 2 -A 2 | head -100Repository: DPDK/grout
Length of output: 2293
🏁 Script executed:
# Search for any documentation or comments about concurrency
rg "concurrent|race|atomic|synchroni" modules/ --type c | head -30Repository: DPDK/grout
Length of output: 2525
🏁 Script executed:
# Look at how API requests are processed and dispatched
find . -name "*api*" -path "*/main/*" -type f | head -10Repository: DPDK/grout
Length of output: 79
🏁 Script executed:
# Check if there's an API processing loop that might serialize requests
rg "api_handler|lookup_api_handler" main/ --type c -B 5 -A 10 | head -100Repository: DPDK/grout
Length of output: 3728
🏁 Script executed:
# Look at how the control plane event loop processes API requests
rg "event_base|api.*process" main/ --type c -B 2 -A 5 | head -80Repository: DPDK/grout
Length of output: 2823
🏁 Script executed:
# Check if there's RCU usage documentation
rg "rte_rcu|RCU" api/ main/ --type c | head -20Repository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Look at how conntrack uses atomic/RCU for inspiration
head -100 modules/policy/control/conntrack.cRepository: DPDK/grout
Length of output: 2836
🏁 Script executed:
# Look at the full api.c to understand request processing
head -200 main/api.c | tail -150Repository: DPDK/grout
Length of output: 4501
🏁 Script executed:
# Check what happens when API request is received
rg "request_received|request_cb" main/api.c -B 5 -A 20Repository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Verify if control plane is single-threaded (event loop based)
rg "event_base_dispatch|event_loop" main/ --type cRepository: DPDK/grout
Length of output: 234
🏁 Script executed:
# Look at the request processing in api.c to understand flow
rg "request_received|handle_request" main/api.c -B 3 -A 15Repository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Specifically look at how bufferevent is processed - is it on event loop thread?
rg "bufferevent.*callback|read_cb|write_cb" main/api.c -B 2 -A 10 | head -80Repository: DPDK/grout
Length of output: 1147
🏁 Script executed:
# Check if there's RCU API available that dhcp should use
rg "gr_datapath_rcu|rte_rcu" api/ main/ --type hRepository: DPDK/grout
Length of output: 36
🏁 Script executed:
# Look for how RCU is used in the codebase
rg "rte_rcu_qsbr_synchronize" modules/ --type c -B 3 -A 3Repository: DPDK/grout
Length of output: 5538
🏁 Script executed:
# Check the gr_rcu header
find . -name "*gr_rcu*" -o -name "*rcu*" | grep -E "\.(h|c)$" | head -10Repository: DPDK/grout
Length of output: 87
🏁 Script executed:
# Read the gr_rcu header
cat modules/infra/datapath/gr_rcu.hRepository: DPDK/grout
Length of output: 214
🏁 Script executed:
# Check if dhcp_input_process is really called from datapath or control plane
rg "dhcp_input_process|dhcp_input_cb" modules/ --type c -B 5 -A 5Repository: DPDK/grout
Length of output: 3354
🏁 Script executed:
# Verify that the control plane API handlers run in the event loop (single-threaded)
head -200 main/api.c | grep -A 20 "read_cb"Repository: DPDK/grout
Length of output: 36
Add synchronization to prevent data race on dhcp_clients array.
dhcp_enabled() is called from the datapath (running on worker threads), but dhcp_clients[] is mutated in dhcp_start() / dhcp_stop() on the control plane thread without synchronization. This is a C data race and can cause stale reads or crashes. Use atomic operations for the pointer access and synchronize with RCU before freeing, following the pattern used in modules/policy/control/conntrack.c and modules/infra/control/nexthop.c.
Suggested fix (atomic pointer + RCU)
+#include <stdatomic.h>
+#include <gr_rcu.h>
-static struct dhcp_client *dhcp_clients[MAX_IFACES];
+static _Atomic(struct dhcp_client *) dhcp_clients[MAX_IFACES];
bool dhcp_enabled(uint16_t iface_id) {
if (iface_id < MAX_IFACES)
- return dhcp_clients[iface_id] != NULL;
+ return atomic_load_explicit(&dhcp_clients[iface_id], memory_order_acquire) != NULL;
return false;
}In dhcp_start(): Use atomic_store_explicit(&dhcp_clients[iface_id], client, memory_order_release).
In dhcp_stop(): Set the pointer to NULL before freeing, then call rte_rcu_qsbr_synchronize(gr_datapath_rcu(), RTE_QSBR_THRID_INVALID) before freeing the client.
🤖 Prompt for AI Agents
In `@modules/dhcp/control/client.c` around lines 32 - 35, dhcp_enabled currently
reads dhcp_clients[] without synchronization causing a data race; change
dhcp_enabled() to load the pointer with
atomic_load_explicit(&dhcp_clients[iface_id], memory_order_acquire) and compare
against NULL, and modify dhcp_start() to publish the new client with
atomic_store_explicit(&dhcp_clients[iface_id], client, memory_order_release); in
dhcp_stop() set the array slot to NULL with atomic_store_explicit before freeing
the client, then call rte_rcu_qsbr_synchronize(gr_datapath_rcu(),
RTE_QSBR_THRID_INVALID) to wait for readers (RCU) and only then free the client
to match the conntrack/nexthop patterns.
|
Tested with udhcpc. |
| } | ||
| memset(ð->src_addr, 0, sizeof(eth->src_addr)); | ||
| iface_get_eth_addr(iface, ð->dst_addr); | ||
| eth->ether_type = RTE_BE16(RTE_ETHER_TYPE_IPV4); |
There was a problem hiding this comment.
src addr is '00:00:00:00:00' ? Quid about vlan here ?
There was a problem hiding this comment.
I don't know which address to use as source. The vlan does not matter though.
2 participants
Fix DHCP packet handling when the client is not enabled on an interface by forwarding packets to the control plane interface instead of dropping them with an error.
Move the UDP port registration to the node callback for consistency with other datapath nodes.
Also add an unrelated patch for missing trace data for ARP input.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.