-
Notifications
You must be signed in to change notification settings - Fork 23
iface: return an error when mtu is too big on creation #455
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
260e390 to
e44437b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/infra/control/iface.c(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
modules/infra/control/iface.c
🧠 Learnings (1)
📚 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:
modules/infra/control/iface.c
🧬 Code graph analysis (1)
modules/infra/control/iface.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-25)
⏰ 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-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: deb
- GitHub Check: rpm
🔇 Additional comments (1)
modules/infra/control/iface.c (1)
148-149: Cleanup logic is correct.The
ifid_validflag properly gates theifaces[ifid]cleanup, ensuring the array entry is only cleared when a valid interface ID was assigned. This prevents clearing unassigned slots if failure occurs before interface ID allocation.
d08e1e0 to
239f27a
Compare
239f27a to
76169cf
Compare
98d2969 to
d2758b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/control/ctlplane.c (1)
405-410: Unchecked return value may use uninitialized MAC address.If
iface_get_eth_addrfails (returns negative, e.g.,EOPNOTSUPPfor interface types without a MAC),macremains uninitialized and garbage is written to the hardware address viamemcpy.- iface_get_eth_addr(iface, &mac); - memcpy(ifr.ifr_hwaddr.sa_data, mac.addr_bytes, sizeof(mac)); - if (ioctl(ioctl_sock, SIOCSIFHWADDR, &ifr) < 0) { - LOG(ERR, "ioctl(SIOCSIFHWADDR) %s", strerror(errno)); - goto err; + if (iface_get_eth_addr(iface, &mac) == 0) { + memcpy(ifr.ifr_hwaddr.sa_data, mac.addr_bytes, sizeof(mac)); + if (ioctl(ioctl_sock, SIOCSIFHWADDR, &ifr) < 0) { + LOG(ERR, "ioctl(SIOCSIFHWADDR) %s", strerror(errno)); + goto err; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
modules/dhcp/control/client.c(1 hunks)modules/dhcp/control/packet.c(1 hunks)modules/infra/api/iface.c(2 hunks)modules/infra/control/bond.c(7 hunks)modules/infra/control/ctlplane.c(3 hunks)modules/infra/control/gr_bond.h(1 hunks)modules/infra/control/gr_iface.h(1 hunks)modules/infra/control/iface.c(15 hunks)modules/infra/control/loopback.c(3 hunks)modules/infra/control/port.c(1 hunks)modules/infra/control/vlan.c(4 hunks)modules/infra/datapath/eth_input.c(1 hunks)modules/infra/datapath/eth_output.c(1 hunks)modules/ip/control/address.c(1 hunks)modules/ip/datapath/arp_output_reply.c(1 hunks)modules/ip/datapath/arp_output_request.c(2 hunks)modules/ip6/control/address.c(2 hunks)modules/ip6/control/router_advert.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/infra/control/port.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
modules/dhcp/control/client.cmodules/dhcp/control/packet.cmodules/ip/datapath/arp_output_request.cmodules/infra/control/gr_bond.hmodules/ip/control/address.cmodules/ip/datapath/arp_output_reply.cmodules/ip6/control/router_advert.cmodules/infra/datapath/eth_output.cmodules/infra/api/iface.cmodules/infra/datapath/eth_input.cmodules/ip6/control/address.cmodules/infra/control/vlan.cmodules/infra/control/gr_iface.hmodules/infra/control/loopback.cmodules/infra/control/ctlplane.cmodules/infra/control/iface.cmodules/infra/control/bond.c
🧠 Learnings (5)
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Applied to files:
modules/ip/datapath/arp_output_request.cmodules/ip/datapath/arp_output_reply.cmodules/infra/datapath/eth_output.cmodules/infra/datapath/eth_input.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/ip/control/address.cmodules/infra/control/iface.c
📚 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:
modules/ip6/control/address.cmodules/infra/control/gr_iface.hmodules/infra/control/loopback.cmodules/infra/control/iface.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/iface.c
🧬 Code graph analysis (12)
modules/dhcp/control/client.c (1)
modules/infra/control/gr_iface.h (1)
iface(16-25)
modules/dhcp/control/packet.c (2)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)modules/infra/control/gr_iface.h (1)
iface(16-25)
modules/ip/control/address.c (2)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)modules/infra/control/gr_iface.h (1)
iface(16-25)
modules/ip/datapath/arp_output_reply.c (2)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)modules/infra/control/gr_iface.h (1)
iface(16-25)
modules/ip6/control/router_advert.c (1)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)
modules/infra/datapath/eth_output.c (1)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)
modules/infra/datapath/eth_input.c (1)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)
modules/ip6/control/address.c (2)
modules/infra/control/iface.c (1)
iface_get_eth_addr(265-277)modules/infra/control/gr_iface.h (1)
iface(16-25)
modules/infra/control/vlan.c (1)
modules/infra/control/iface.c (4)
iface_del_eth_addr(324-336)iface_from_id(256-263)iface_get_eth_addr(265-277)iface_add_eth_addr(310-322)
modules/infra/control/loopback.c (3)
modules/infra/control/iface.c (2)
iface_destroy(399-437)iface_next(239-254)modules/infra/control/gr_iface.h (1)
iface(16-25)modules/infra/control/worker_test.c (1)
iface_next(49-58)
modules/infra/control/iface.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-25)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/nexthop.c (1)
nexthop_iface_cleanup(537-540)
modules/infra/control/bond.c (2)
modules/infra/control/iface.c (4)
iface_add_eth_addr(310-322)iface_del_eth_addr(324-336)iface_set_mtu(338-356)iface_get_eth_addr(265-277)modules/infra/control/gr_iface.h (1)
iface(16-25)
🔇 Additional comments (22)
modules/dhcp/control/packet.c (1)
116-119: LGTM!The API migration from ID-based to pointer-based
iface_get_eth_addris correct. The iface pointer is already validated at lines 110-114, and error handling remains intact.modules/dhcp/control/client.c (1)
43-44: LGTM!Correct migration to pointer-based API. The iface pointer is validated at lines 39-41, and error handling is preserved.
modules/ip/control/address.c (1)
101-102: LGTM!The pointer-based API call is correct with proper error handling.
modules/infra/control/gr_bond.h (1)
16-16: LGTM!Removing
constallows bond member operations to use the new mutable interface APIs.modules/ip/datapath/arp_output_request.c (1)
64-64: LGTM!The pointer-based API migration is correct. While
iface_from_idmight return NULL,iface_get_eth_addrhandles NULL internally and the error check at line 109 properly routes failures to the ERROR edge.Also applies to: 108-112
modules/ip6/control/router_advert.c (1)
145-152: LGTM!The API migration correctly passes the iface pointer directly. Error handling is preserved.
modules/infra/api/iface.c (2)
38-38: LGTM!Correct usage of pointer-based
iface_destroyAPI in the error path.
60-60: LGTM!Proper use of the pointer-based
iface_destroyAPI with the already-resolved iface pointer.modules/ip/datapath/arp_output_reply.c (1)
58-58: LGTM!The API migration is correct. The iface pointer is validated at lines 43-48.
modules/infra/datapath/eth_output.c (1)
72-75: LGTM!The migration to pointer-based
iface_get_eth_addris correct, and the error path properly routes toNO_MACedge on failure.modules/ip6/control/address.c (2)
213-214: LGTM!Correct migration to pointer-based API with proper error handling.
395-399: LGTM!Correct migration with proper conditional check on return value.
modules/infra/control/ctlplane.c (2)
83-84: LGTM!Correct migration to pointer-based
iface_destroy.
117-118: LGTM!Correct migration to pointer-based
iface_destroy.modules/infra/control/vlan.c (5)
52-52: LGTM!
cur_parentis validated at lines 47-48 before use here.
93-101: LGTM!
iface_get_eth_addrandiface_add_eth_addrreturnEINVALifparentis NULL, and error paths propagate correctly.
160-168: LGTM!
iface_add_eth_addrreturnsEINVALfor NULL parent, error propagates correctly.
170-178: LGTM!
iface_del_eth_addrreturnsEINVALfor NULL parent, error propagates correctly.
107-120: Verifyiface_del_subinterfacehandles NULL parent.At line 109,
iface_from_idcould return NULL if the parent was destroyed before the VLAN. Whileiface_del_eth_addrhandles NULL gracefully at line 114,iface_del_subinterface(parent, iface)at line 117 may not.#!/bin/bash # Check if iface_del_subinterface handles NULL first argument ast-grep --pattern $'void iface_del_subinterface($_, $_) { $$$ }' rg -n -A10 'void iface_del_subinterface'modules/infra/control/loopback.c (3)
100-102: LGTM!Correct migration to pointer-based
iface_destroy.
131-132: LGTM!Correct migration to pointer-based
iface_destroy.
192-196: LGTM!Iterator type correctly changed to non-const to allow passing to
iface_destroy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/infra/control/port.c (1)
231-252: Port left stopped ifrte_eth_dev_set_mtufails.If
rte_eth_dev_set_mtureturns an error other thanENOSYS/EOPNOTSUPP, the function exits at line 242 without restarting the port. The interface becomes inoperable until manually reconfigured or destroyed.ret = rte_eth_dev_set_mtu(p->port_id, mtu); switch (ret) { case 0: case -ENOSYS: case -EOPNOTSUPP: break; default: - return errno_log(-ret, "rte_eth_dev_set_mtu"); + errno_log(-ret, "rte_eth_dev_set_mtu"); + goto restart; } +restart: if ((ret = rte_eth_dev_start(p->port_id)) < 0) return errno_log(-ret, "rte_eth_dev_start"); p->started = true; + if (ret < 0) + return ret; iface->mtu = mtu;Alternatively, save the error from
rte_eth_dev_set_mtuand return it after restarting the port.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/infra/control/iface.c(15 hunks)modules/infra/control/port.c(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
modules/infra/control/port.cmodules/infra/control/iface.c
🧠 Learnings (5)
📓 Common learnings
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.
Learnt from: rjarry
Repo: DPDK/grout PR: 408
File: api/gr_net_types.h:78-84
Timestamp: 2025-11-24T12:50:09.337Z
Learning: In api/gr_net_types.h, ip4_addr_t values are stored in network byte order (big-endian). When using a union to cast to a byte array (uint8_t u8[4]), accessing u8[0] correctly reads the first octet (most significant byte) of the IP address on all architectures, because it directly accesses the memory layout rather than performing integer operations. This pattern is valid for checking properties like multicast (first octet in range 224-239).
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in the IP module takes ownership of the nexthop reference and automatically calls nexthop_decref(nh) on failure paths, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-09-22T09:21:51.749Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: modules/ip/control/address.c:102-110
Timestamp: 2025-09-22T09:21:51.749Z
Learning: The rib4_insert() function in modules/ip/control/route.c takes ownership of the nexthop reference by calling nexthop_incref(nh) at the beginning and properly calls nexthop_decref(nh) on any failure paths via the fail: label, so callers don't need to manually decrement the reference count when rib4_insert fails.
Applied to files:
modules/infra/control/iface.c
📚 Learning: 2025-09-25T07:52:17.403Z
Learnt from: rjarry
Repo: DPDK/grout PR: 312
File: frr/rt_grout.c:355-361
Timestamp: 2025-09-25T07:52:17.403Z
Learning: The nexthop_new() function in FRR's zebra codebase cannot fail and return NULL - it will abort() the process if memory allocation fails, so null checks after calling nexthop_new() are unnecessary.
Applied to files:
modules/infra/control/iface.c
📚 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:
modules/infra/control/iface.c
🧬 Code graph analysis (1)
modules/infra/control/iface.c (3)
modules/infra/control/gr_iface.h (1)
iface(16-25)api/gr_errno.h (1)
errno_set(9-12)modules/infra/control/nexthop.c (1)
nexthop_iface_cleanup(537-540)
⏰ 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 (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (1)
modules/infra/control/iface.c (1)
338-356: MTU normalization and validation logic looks correct.The MTU handling properly normalizes
0to1500and enforces the valid range1280..gr_config.max_mtu, returningERANGEfor out-of-bounds values. This addresses the PR objective of preventing invalid MTU values during interface creation.
The interface manipulation functions were taking interface IDs as uint16_t parameters and internally calling iface_from_id() to retrieve the struct iface pointer. This approach has several drawbacks. First, it adds unnecessary overhead since most callers already have the iface pointer available. Second, it creates redundant error checking as both the caller and the function need to validate the interface. Third, it obscures the API contract by hiding whether the function operates on the interface object or just uses it as a lookup key. Change these functions to accept struct iface pointers directly. Callers that only have an interface ID must now explicitly call iface_from_id() themselves, making the lookup cost visible at the call site. Functions that previously performed double lookups now do a single one. Error handling for NULL pointers is simplified to use errno_set(EINVAL) consistently. Update bond member structures to use non-const pointers since these manipulation functions require mutable access to properly update interface state. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
If any operation fails after the interface object was "initialized", make sure to call iface_destroy() to properly deallocate resources. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
When creating an interface, iface_set_mtu is not called. When the MTU value is larger than gr_config.max_mtu, it causes such spams in the logs: ERR: GROUT: iface_cp_poll: rte_pktmbuf_alloc No such file or directory ERR: GROUT: iface_cp_poll: rte_pktmbuf_alloc No such file or directory ERR: GROUT: iface_cp_poll: rte_pktmbuf_alloc No such file or directory Now that all the internal iface API takes pointers, we can call iface_set_mtu and iface_set_promisc respectively. Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
Reject any MTU smaller than 1280 to comply with the IPv6 standard. If MTU is set to 0, force a default 1500 bytes value. Simplify the port_set_mtu callback accordingly. We don't need to read the MTU from the DPDK port if it is 0 anymore. Link: https://datatracker.ietf.org/doc/html/rfc8200#section-5 Signed-off-by: Robin Jarry <[email protected]> Reviewed-by: Christophe Fontaine <[email protected]>
ad0599d to
62e4d52
Compare
When creating an interface, iface_set_mtu is not called. When the MTU value is larger than gr_config.max_mtu, it causes such spams in the logs:
Move the mtu and promisc configuration after the interface has been stored in the ifaces array. Reset the ifaces array value on error.