[PWCI] "[ovs-dev,v2] odp-execute: Fix packet length check for TSO packets."#14
[PWCI] "[ovs-dev,v2] odp-execute: Fix packet length check for TSO packets."#14
Conversation
Moving Russell to emeritus status at his request. Thanks, Russell, for all your contributions and support! Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
OVS adds matches on the DF/CSUM/etc bits of the tunnel info flags, but the DF bit is never actually extracted from the outer IP header during the tunnel decapsulation. This is not a huge problem, as we'll only match on what was parsed out, since matching on DF flag is not exposed through OpenFlow to the users. And since the bit is never extracted, we'll just always match on -df, which sort of "works", because the bit is never extracted and so it is never set. However, this causes misleading -df matches in the datapath flow dumps even if the packets actually have the DF bit set, which it is by default. Fix that by actually extracting the bit from the outer header while decapsulating tunneled traffic. Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
TSO packets were incorrectly treated as too big by the check_pkt_len action with the userspace datapath. Adjust the check by looking at the requested segment size. Fixes: 29cf9c1 ("userspace: Add TCP Segmentation Offload support") Reported-at: https://issues.redhat.com/browse/FDP-1631 Signed-off-by: David Marchand <david.marchand@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideImplements TSO-aware packet length validation by computing a per-segment maximum based on TCP payload offset and fixes tunnel flag handling by setting the IPv4 “dont fragment” bit and OR’ing GRE checksum flags to preserve existing flags. Class diagram for updated dp_packet TSO and payload handlingclassDiagram
class dp_packet {
+get_send_len()
+get_tso_segsz()
+get_tcp_payload()
+get_inner_tcp_payload()
+get_l2_pad_size()
+get_tunnel()
+get_data()
}
class odp_execute_check_pkt_len {
+size
+TSO-aware length check
}
dp_packet <.. odp_execute_check_pkt_len: uses
Class diagram for updated flow_tnl flag handling in tunnel parsingclassDiagram
class flow_tnl {
+ip_tos
+ip_ttl
+flags
}
class ip_extract_tnl_md {
+sets FLOW_TNL_F_DONT_FRAGMENT if IPv4 DF bit is set
}
class parse_gre_header {
+flags |= FLOW_TNL_F_CSUM
}
flow_tnl <.. ip_extract_tnl_md: modified
flow_tnl <.. parse_gre_header: modified
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughUpdates tunnel flag handling to set DF for IPv4 and preserve GRE flags, adds TSO-aware packet-length checking, and adjusts tests to reflect DF (+df-csum) in tunnel expectations. Introduces a new userspace offload test for TSO with check_pkt_len. Maintainers list moves one entry to Emeritus. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Ingress as Ingress Packet
participant Exec as odp_execute_check_pkt_len
participant Parser as L3/L4 Parser
participant TSO as TSO Context
Ingress->>Exec: Execute actions (check_pkt_len)
Exec->>Parser: Determine base send length, L2 padding
Parser-->>Exec: Base size (send_len - l2pad)
alt TSO enabled
Exec->>TSO: Retrieve TSO segment size
alt Tunnelled TCP
Exec->>Parser: Locate inner TCP payload offset
Parser-->>Exec: Inner payload offset
else Non-tunnel TCP
Exec->>Parser: Locate TCP payload offset
Parser-->>Exec: TCP payload offset
end
Exec->>Exec: segsize = payload_offset + tso_seg_size
Exec->>Exec: size = min(size, segsize)
end
Exec-->>Ingress: Compare size vs threshold -> branch (gt / le)
sequenceDiagram
autonumber
participant Rx as Packet RX
participant Tnl as ip_extract_tnl_md
participant GRE as parse_gre_header
participant Flow as Flow Tunnel MD
Rx->>Tnl: Parse IPv4 header
Tnl->>Tnl: Check DF bit (ip_frag_off & IP_DF)
Tnl->>Flow: Set FLOW_TNL_F_DONT_FRAGMENT if DF
Rx->>GRE: Parse GRE header (if GRE)
GRE->>Flow: tnl->flags |= FLOW_TNL_F_CSUM (preserve other flags)
GRE-->>Rx: Return updated tunnel flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![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 (3)
tests/dpif-netdev.at (3)
2257-2260: Nit: prefer explicit nxm notation for clarity (optional)check_pkt_larger(514)->NXM_NX_REG0[0] is correct; for readability you could use reg0[0] consistently with other tests (if accepted by parser).
2270-2273: Stable match of check_pkt_len actionDump assertion is tight; to reduce brittleness across minor formatting changes, consider matching only the action substring.
Apply this diff if desired:
-AT_CHECK([ovs-appctl dpctl/dump-flows | strip_used | strip_stats], [0], [dnl -flow-dump from the main thread: -recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:check_pkt_len(size=514,gt(3),le(2)) -]) +AT_CHECK([ovs-appctl dpctl/dump-flows | strip_used | strip_stats | grep 'actions:check_pkt_len(size=514,gt(3),le(2))$'])
2275-2301: pcap counters rely on clean startCounts assume new pcaps. If flakiness appears, explicitly clear pcap files after bridge creation.
Example:
add_of_ports --pcap br1 `seq 1 3` +rm -f p2-tx.pcap p3-tx.pcap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
MAINTAINERS.rst(1 hunks)lib/netdev-native-tnl.c(2 hunks)lib/odp-execute.c(1 hunks)tests/dpif-netdev.at(1 hunks)tests/nsh.at(2 hunks)tests/packet-type-aware.at(8 hunks)tests/tunnel-push-pop.at(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/odp-execute.c (1)
lib/dp-packet.h (5)
dp_packet_get_tso_segsz(716-720)dp_packet_get_tso_segsz(783-787)dp_packet_get_inner_tcp_payload(560-574)dp_packet_get_tcp_payload(544-558)dp_packet_data(830-835)
lib/netdev-native-tnl.c (1)
include/sparse/netinet/in.h (1)
htons(158-161)
⏰ 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). (20)
- GitHub Check: linux gcc check-offloads 100-
- GitHub Check: linux gcc m32 --disable-ssl
- GitHub Check: linux gcc --disable-ssl
- GitHub Check: linux gcc dpdk-shared
- GitHub Check: linux gcc --enable-afxdp
- GitHub Check: linux gcc dpdk-shared --enable-shared
- GitHub Check: linux clang test --enable-shared
- GitHub Check: linux gcc test --enable-shared
- GitHub Check: linux clang c99
- GitHub Check: linux gcc test
- GitHub Check: linux clang --enable-afxdp
- GitHub Check: linux gcc test -ljemalloc
- GitHub Check: linux gcc check check-dpdk dpdk
- GitHub Check: clang-analyze-cache
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-13-5-snap
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-14-3-snap
- GitHub Check: osx clang --disable-ssl
- GitHub Check: linux rpm fedora
- GitHub Check: linux deb no dpdk
- GitHub Check: build oss-fuzz fuzzers
🔇 Additional comments (15)
MAINTAINERS.rst (1)
94-95: Move to Emeritus looks correct.Entry for Russell Bryant appears only under Emeritus; no remaining active entry visible in this diff. Good housekeeping.
lib/netdev-native-tnl.c (2)
155-157: Correctly preserve IPv4 DF into tunnel flags.Using ip->ip_frag_off & htons(IP_DF) and OR-ing FLOW_TNL_F_DONT_FRAGMENT ensures DF is propagated without clobbering other flags. Matches emit path in netdev_tnl_ip_build_header().
474-474: Fix: don’t overwrite existing tunnel flags when GRE has CSUM.Changing to tnl->flags |= FLOW_TNL_F_CSUM preserves previously-set flags (e.g., DF, KEY). This resolves subtle flag-loss bugs on GRE pop.
tests/tunnel-push-pop.at (1)
688-688: Test expectation aligned with preserved flags.flags(+df-csum+key) matches the DF detection and GRE/UDP checksum flag preservation. Good update.
lib/odp-execute.c (1)
800-818: Approve TSO-aware check_pkt_len clamp
Global search showscheck_pkt_lenand TSO paths in lib/odp-execute.c, ofproto, netdev modules and extensive coverage in system-tso and dpif-netdev tests—no missing call sites.tests/nsh.at (2)
722-724: DF and CSUM flags expectation flip looks correctThe tunnel header in the preceding tnl_push shows IPv4 frag=0x4000 (DF). Expecting flags(+df-csum+key) on the recirculated tunnel context is consistent. LGTM.
776-779: Consistent DF expectation across subsequent recircsThe additional recirc steps also now show flags(+df-csum+key), matching DF propagation. Looks good.
tests/packet-type-aware.at (7)
327-328: GRE recirc now shows DF+CSUM — OKThe flags(+df-csum) on the tunnel context after tnl_pop(gre_sys) align with setting DF on the outer IPv4 header. Approve.
345-346: DF+CSUM on N1→N2 pathExpectation updated to flags(+df-csum) looks consistent with the push header (frag=0x4000). LGTM.
363-364: N2→N1 GRE recirc DF flagSame rationale; update is correct.
381-383: Chained GRE hops preserve DFDF shown on both hops is accurate given DF in outer headers. Looks good.
400-402: IPv6/IPv4 mix: DF only on IPv4 outer — OKIPv6 legs lack DF (not applicable), IPv4 legs show flags(+df-csum). Expectations match behavior.
419-420: DF+CSUM on L3 GREMatches earlier changes; consistent.
1018-1020: MPLS over GRE: DF retained after packet_type changeflags(+df-csum) on both pre- and post-recirculation flows align with DF in the push path; OK.
tests/dpif-netdev.at (1)
2248-2304: New TSO-aware check_pkt_len test covers key boundary cases
- Default (no TSO): le path to port 2.
- TSO=460: 54+460=514 → le path (2).
- TSO=500: 54+500=554 → gt path (3).
Helper macros/functions add_of_br, add_of_ports, strip_stats and strip_used are defined in tests/ofproto-macros.at and available in this context.
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
3 participants
NOTE: This is an auto submission for "[ovs-dev,v2] odp-execute: Fix packet length check for TSO packets.".
See "http://patchwork.ozlabs.org/project/openvswitch/list/?series=473083" for details.
Summary by Sourcery
Fix packet length validation for TCP Segmentation Offload and refine tunnel metadata flags handling
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Tests
Documentation