eth_output: fix trace corruption and stale mac on error by rjarry · Pull Request #554 · DPDK/grout

rjarry · 2026-03-13T15:13:50Z

The eth_output trace data was corrupted because writing vlan_id=0 into the mbuf data overwrites the start of eth_output_mbuf_data->dst, causing destination ethernet addresses to start with zeroes in traces. Move the vlan_id reset after inserting traces to avoid this.

Also ensure that src_mac is zeroed on error paths (no headroom or unresolved mac address) so traces don't show stale data from a previous loop iteration.

Summary by CodeRabbit

Bug Fixes
- Failed packet preparation now clears the source MAC and resets the interface ID to prevent sending packets with invalid source addressing.
- VLAN ID is always set before packets are queued, preventing mis-tagged packets.
- Packet tracing behavior unchanged aside from ensuring VLAN initialization happens prior to enqueue.

coderabbitai · 2026-03-13T15:18:54Z

📝 Walkthrough

Walkthrough

Changes in modules/infra/datapath/eth_output.c adjust error handling and metadata ordering. On prepend failure and on iface_get_eth_addr failure the code resets last_iface_id to undef and zeroes src_mac. The vlan_id assignment to the mbuf was moved to after the tracing block so it is set unconditionally before enqueue. No other changes to dst_addr, src_addr, ether_type, edge selection, or tracing logic.

📝 Coding Plan

Generate coding plan for human review comments

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.

Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@modules/infra/datapath/eth_output.c`:
- Around line 40-42: The src_mac buffer is zeroed on error paths (e.g., where
memset(&src_mac, 0, sizeof(src_mac)) and edge = NO_HEADROOM; goto next; are
used) but last_iface_id is not cleared, which lets a later packet skip
iface_get_eth_addr() and emit a zero source MAC; update those error paths (both
the block around the memset at Line 40 and the similar block at Lines 47–49) to
also invalidate last_iface_id (set it to an impossible/invalid value) whenever
src_mac is cleared so that subsequent packets will call iface_get_eth_addr() to
repopulate src_mac before use.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 94560fd3-fc98-4424-b577-3125f9dca632

📥 Commits

Reviewing files that changed from the base of the PR and between 4b2eb2c and 5ad2368.

📒 Files selected for processing (1)

modules/infra/datapath/eth_output.c

coderabbitai

♻️ Duplicate comments (1)

modules/infra/datapath/eth_output.c (1)
47-50: ⚠️ Potential issue | 🔴 Critical

Invalidate iface cache on MAC lookup failure.

On Line 48, src_mac is zeroed but last_iface_id is not invalidated. If a later packet arrives on the previously cached iface, the lookup can be skipped and a zero source MAC can be emitted. Reset last_iface_id in this error path before goto next.
Proposed fix
 		if (priv->iface->id != last_iface_id) {
 			if (iface_get_eth_addr(priv->iface, &src_mac) < 0) {
 				memset(&src_mac, 0, sizeof(src_mac));
+				last_iface_id = GR_IFACE_ID_UNDEF;
 				edge = NO_MAC;
 				goto next;
 			}
 			last_iface_id = priv->iface->id;
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modules/infra/datapath/eth_output.c` around lines 47 - 50, The error path
after iface_get_eth_addr failing zeroes src_mac but doesn't invalidate the iface
cache, so subsequent packets may reuse last_iface_id and emit a zero MAC; update
the failure branch in the same block to reset/clear priv->last_iface_id (e.g.,
set to an invalid sentinel used elsewhere such as -1 or INVALID_IFACE) before
setting edge = NO_MAC and jumping to the existing goto next, ensuring
iface_get_eth_addr, src_mac, last_iface_id and the goto next flow are updated
consistently.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@modules/infra/datapath/eth_output.c`:
- Around line 47-50: The error path after iface_get_eth_addr failing zeroes
src_mac but doesn't invalidate the iface cache, so subsequent packets may reuse
last_iface_id and emit a zero MAC; update the failure branch in the same block
to reset/clear priv->last_iface_id (e.g., set to an invalid sentinel used
elsewhere such as -1 or INVALID_IFACE) before setting edge = NO_MAC and jumping
to the existing goto next, ensuring iface_get_eth_addr, src_mac, last_iface_id
and the goto next flow are updated consistently.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4ba86a5c-52b4-48da-82a3-859d1acbb704

📥 Commits

Reviewing files that changed from the base of the PR and between 5e160d7 and dd84937.

📒 Files selected for processing (1)

modules/infra/datapath/eth_output.c

Writing vlan_id=0 in the mbuf_data overwrites the start of eth_output_mbuf_data->dst. This causes partially truncated traces where the destination ethernet address starts with zeroes. --------- 15:28:50.562920770 cpu 0 --------- port_rx-p0q0: RX_L4_CKSUM_UNKNOWN RX_IP_CKSUM_GOOD RX_OUTER_L4_CKSUM_UNKNOWN TX_L4_NO_CKSUM iface_input: iface=p0 mode=VRF eth_input: 02:e3:ad:74:50:9b > 96:57:00:fb:bf:73 type=IP(0x0800) ip_input: 16.0.0.1 > 16.1.0.1 ttl=64 proto=ICMP(1) ip_forward: ip_output: 16.0.0.1 > 16.1.0.1 ttl=63 proto=ICMP(1) eth_output: 1e:58:43:26:80:7e > 00:00:27:14:8a:4f type=IP(0x0800) ^^^^^ iface_output: iface=p1 port_output: port_tx-p1q0: RX_L4_CKSUM_UNKNOWN RX_IP_CKSUM_GOOD RX_OUTER_L4_CKSUM_UNKNOWN TX_L4_NO_CKSUM Zero the vlan_id *after* inserting the traces. Fixes: 6e56b0c ("iface_output: split out of eth_output") Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com> Reviewed-by: David Marchand <david.marchand@redhat.com>

When there isn't enough headroom to prepend the ethernet header or if the output interface mac address cannot be resolved, make sure to reset src_mac to zeroes to avoid tracing whatever is left in that variable. Signed-off-by: Robin Jarry <rjarry@redhat.com> Reviewed-by: Christophe Fontaine <cfontain@redhat.com> Reviewed-by: David Marchand <david.marchand@redhat.com>

coderabbitai

♻️ Duplicate comments (1)

modules/infra/datapath/eth_output.c (1)
47-51: ⚠️ Potential issue | 🔴 Critical

Missing last_iface_id invalidation on iface_get_eth_addr failure.

When iface_get_eth_addr fails here, src_mac is zeroed but last_iface_id is not reset. A subsequent packet on a previously cached iface will hit the cache (line 46 check passes), skip iface_get_eth_addr, and use the zeroed src_mac.
 		if (iface_get_eth_addr(priv->iface, &src_mac) < 0) {
+			last_iface_id = GR_IFACE_ID_UNDEF;
 			memset(&src_mac, 0, sizeof(src_mac));
 			edge = NO_MAC;
 			goto next;
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modules/infra/datapath/eth_output.c` around lines 47 - 51, When
iface_get_eth_addr(priv->iface, &src_mac) fails you currently zero src_mac but
do not invalidate the cached iface ID, so subsequent packets may use a stale
last_iface_id and the zeroed src_mac; update the error handling in the failure
branch inside eth_output.c to also reset/invalidate last_iface_id (the variable
used for the cache check) — e.g., set last_iface_id to an invalid sentinel
(matching where it’s compared) before jumping to next so the cache miss forces a
fresh iface lookup on the next packet.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@modules/infra/datapath/eth_output.c`:
- Around line 47-51: When iface_get_eth_addr(priv->iface, &src_mac) fails you
currently zero src_mac but do not invalidate the cached iface ID, so subsequent
packets may use a stale last_iface_id and the zeroed src_mac; update the error
handling in the failure branch inside eth_output.c to also reset/invalidate
last_iface_id (the variable used for the cache check) — e.g., set last_iface_id
to an invalid sentinel (matching where it’s compared) before jumping to next so
the cache miss forces a fresh iface lookup on the next packet.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4bf9d89a-deac-42e6-8560-e949b119bbc3

📥 Commits

Reviewing files that changed from the base of the PR and between dd84937 and 3a55a93.

📒 Files selected for processing (1)

modules/infra/datapath/eth_output.c

coderabbitai Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread modules/infra/datapath/eth_output.c

christophefontaine approved these changes Mar 13, 2026

View reviewed changes

grout-bot force-pushed the eth-out-trace-fix branch from 5ad2368 to 5e160d7 Compare March 13, 2026 17:20

rjarry force-pushed the eth-out-trace-fix branch from 5e160d7 to dd84937 Compare March 13, 2026 18:24

coderabbitai Bot reviewed Mar 13, 2026

View reviewed changes

david-marchand approved these changes Mar 14, 2026

View reviewed changes

rjarry added 2 commits March 14, 2026 09:00

grout-bot force-pushed the eth-out-trace-fix branch from dd84937 to 3a55a93 Compare March 14, 2026 09:00

coderabbitai Bot reviewed Mar 14, 2026

View reviewed changes

rjarry merged commit 5e97c2c into DPDK:main Mar 14, 2026
7 checks passed

rjarry deleted the eth-out-trace-fix branch March 14, 2026 09:37

Navigation Menu

Uh oh!

eth_output: fix trace corruption and stale mac on error - #554

eth_output: fix trace corruption and stale mac on error#554
rjarry merged 2 commits into
DPDK:mainfrom
rjarry:eth-out-trace-fix

rjarry commented Mar 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 13, 2026 •

edited

Loading

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

rjarry commented Mar 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rjarry commented Mar 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 13, 2026 •

edited

Loading