| Message ID | SJ0PR20MB6079CDF58C773C952081BA36FA112@SJ0PR20MB6079.namprd20.prod.outlook.com |
|---|---|
| State | Superseded |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev,v3] ipf: Fix the over-sized reassembly. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Faicker Mo <faicker.mo@zenlayer.com> writes: > Fix the coredump of ovs_assert, The backtrace is as follows, > #8 0x0000561ee52084bb in ovs_assert_failure (where=<>, function=<>, condition=<>) at ../lib/util.c:89 > #9 0x0000561ee50f8ab2 in dp_packet_set_size (b=<>, v=<>) at ../lib/dp-packet.h:687 > #10 dp_packet_set_size (v=<>, b=0x7f7f88143e80) at ../lib/dp-packet.h:687 > #11 dp_packet_put_uninit (size=395, b=0x7f7f88143e80) at ../lib/dp-packet.c:355 > #12 dp_packet_put (b=0x7f7f88143e80, p=0x7f7f88143ce2, size=395) at ../lib/dp-packet.c:376 > #13 0x0000561ee512c147 in ipf_reassemble_v4_frags (ipf_list=0x7f7f88110810) at ../lib/ipf.c:430 > > The mbuf data_len is a uint16_t field, which includes the ether header. > So does IPv6. > > Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.") > > Signed-off-by: Faicker Mo <faicker.mo@zenlayer.com> > --- > v3: > - Address comments from Aaron: > - Test case fixed. Thanks. Okay just one quick nit below. > --- > lib/ipf.c | 6 ++++-- > tests/system-kmod-macros.at | 8 ++++++++ > tests/system-traffic.at | 12 ++++++++++-- > tests/system-userspace-macros.at | 8 ++++++++ > 4 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/lib/ipf.c b/lib/ipf.c > index 59e232355..e994be928 100644 > --- a/lib/ipf.c > +++ b/lib/ipf.c > @@ -410,11 +410,12 @@ ipf_reassemble_v4_frags(struct ipf_list *ipf_list) > dp_packet_set_size(pkt, dp_packet_size(pkt) - dp_packet_l2_pad_size(pkt)); > struct ip_header *l3 = dp_packet_l3(pkt); > int len = ntohs(l3->ip_tot_len); > + int orig_len = dp_packet_size(pkt); > > int rest_len = frag_list[ipf_list->last_inuse_idx].end_data_byte - > frag_list[1].start_data_byte + 1; > > - if (len + rest_len > IPV4_PACKET_MAX_SIZE) { > + if (orig_len + rest_len > UINT16_MAX) { Sorry, I missed it the first time around, but I don't think we should be changing from the IPV4_PACKET_MAX_SIZE constant here. UINT16_MAX is a sort of implementation detail of dp_packet. But the packet really should never exceed IPV4_PACKET_MAX_SIZE value. Same for the IPV6_PACKET_MAX_DATA value. They are in fact the same value, so it shouldn't make any difference from the code emitted. Readability wise, I find it nicer. > ipf_print_reass_packet( > "Unsupported big reassembled v4 packet; v4 hdr:", l3); > dp_packet_delete(pkt); > @@ -459,11 +460,12 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) > dp_packet_set_size(pkt, dp_packet_size(pkt) - dp_packet_l2_pad_size(pkt)); > struct ovs_16aligned_ip6_hdr *l3 = dp_packet_l3(pkt); > int pl = ntohs(l3->ip6_plen) - sizeof(struct ovs_16aligned_ip6_frag); > + int orig_len = dp_packet_size(pkt); > > int rest_len = frag_list[ipf_list->last_inuse_idx].end_data_byte - > frag_list[1].start_data_byte + 1; > > - if (pl + rest_len > IPV6_PACKET_MAX_DATA) { > + if (orig_len + rest_len > UINT16_MAX) { > ipf_print_reass_packet( > "Unsupported big reassembled v6 packet; v6 hdr:", l3); > dp_packet_delete(pkt); > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at > index a48bd532a..7a7a19f7e 100644 > --- a/tests/system-kmod-macros.at > +++ b/tests/system-kmod-macros.at > @@ -202,6 +202,14 @@ m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL], > > ]) > > +# OVS_CHECK_FRAG_LARGE > +# > +# This check isn't valid for kernel > +m4_define([OVS_CHECK_FRAG_LARGE], > +[ > + > +]) > + > # OVS_CHECK_MIN_KERNEL([minversion], [minsublevel]) > # > # Skip test if kernel version falls below minversion.minsublevel > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 16de8da20..95507cb57 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -4603,7 +4603,11 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -W 2 10.1.1.2 | FORMAT_PING > dnl Check userspace conntrack fragmentation counters. > DPCTL_CHECK_FRAGMENTATION_PASS() > > -OVS_TRAFFIC_VSWITCHD_STOP > +dnl Ipv4 max packet size fragmentation dropped. > +NS_EXEC([at_ns0], [ping -s 65507 -q -c 1 -W 0.5 10.1.1.2]) > +OVS_CHECK_FRAG_LARGE() > + > +OVS_TRAFFIC_VSWITCHD_STOP(["/Unsupported big reassembled v4 packet/d"]) > AT_CLEANUP > > AT_SETUP([conntrack - IPv4 fragmentation expiry]) > @@ -4897,7 +4901,11 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -W 2 fc00::2 | FORMAT_PING > 3 packets transmitted, 3 received, 0% packet loss, time 0ms > ]) > > -OVS_TRAFFIC_VSWITCHD_STOP > +dnl Ipv6 max packet size fragmentation dropped. > +NS_EXEC([at_ns0], [ping6 -s 65487 -q -c 1 -W 0.5 fc00::2]) > +OVS_CHECK_FRAG_LARGE() > + > +OVS_TRAFFIC_VSWITCHD_STOP(["/Unsupported big reassembled v6 packet/d"]) > AT_CLEANUP > > AT_SETUP([conntrack - IPv6 fragmentation expiry]) > diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at > index c1be97347..49b277a08 100644 > --- a/tests/system-userspace-macros.at > +++ b/tests/system-userspace-macros.at > @@ -298,6 +298,14 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m | FORMAT_FRAG_LIST()], [], [dnl > ]) > ]) > > +# OVS_CHECK_FRAG_LARGE() > +# > +# The userspace needs to check that ipf larger fragments have occurred. > +m4_define([OVS_CHECK_FRAG_LARGE], > +[ > + OVS_WAIT_UNTIL([grep -Eq 'Unsupported big reassembled (v4|v6) packet' ovs-vswitchd.log]) > +]) > + > # OVS_CHECK_MIN_KERNEL([minversion], [maxversion]) > # > # The userspace skips all tests that check kernel version. > -- > 2.34.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/ipf.c b/lib/ipf.c index 59e232355..e994be928 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -410,11 +410,12 @@ ipf_reassemble_v4_frags(struct ipf_list *ipf_list) dp_packet_set_size(pkt, dp_packet_size(pkt) - dp_packet_l2_pad_size(pkt)); struct ip_header *l3 = dp_packet_l3(pkt); int len = ntohs(l3->ip_tot_len); + int orig_len = dp_packet_size(pkt); int rest_len = frag_list[ipf_list->last_inuse_idx].end_data_byte - frag_list[1].start_data_byte + 1; - if (len + rest_len > IPV4_PACKET_MAX_SIZE) { + if (orig_len + rest_len > UINT16_MAX) { ipf_print_reass_packet( "Unsupported big reassembled v4 packet; v4 hdr:", l3); dp_packet_delete(pkt); @@ -459,11 +460,12 @@ ipf_reassemble_v6_frags(struct ipf_list *ipf_list) dp_packet_set_size(pkt, dp_packet_size(pkt) - dp_packet_l2_pad_size(pkt)); struct ovs_16aligned_ip6_hdr *l3 = dp_packet_l3(pkt); int pl = ntohs(l3->ip6_plen) - sizeof(struct ovs_16aligned_ip6_frag); + int orig_len = dp_packet_size(pkt); int rest_len = frag_list[ipf_list->last_inuse_idx].end_data_byte - frag_list[1].start_data_byte + 1; - if (pl + rest_len > IPV6_PACKET_MAX_DATA) { + if (orig_len + rest_len > UINT16_MAX) { ipf_print_reass_packet( "Unsupported big reassembled v6 packet; v6 hdr:", l3); dp_packet_delete(pkt); diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index a48bd532a..7a7a19f7e 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -202,6 +202,14 @@ m4_define([DPCTL_CHECK_FRAGMENTATION_FAIL], ]) +# OVS_CHECK_FRAG_LARGE +# +# This check isn't valid for kernel +m4_define([OVS_CHECK_FRAG_LARGE], +[ + +]) + # OVS_CHECK_MIN_KERNEL([minversion], [minsublevel]) # # Skip test if kernel version falls below minversion.minsublevel diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 16de8da20..95507cb57 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -4603,7 +4603,11 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -W 2 10.1.1.2 | FORMAT_PING dnl Check userspace conntrack fragmentation counters. DPCTL_CHECK_FRAGMENTATION_PASS() -OVS_TRAFFIC_VSWITCHD_STOP +dnl Ipv4 max packet size fragmentation dropped. +NS_EXEC([at_ns0], [ping -s 65507 -q -c 1 -W 0.5 10.1.1.2]) +OVS_CHECK_FRAG_LARGE() + +OVS_TRAFFIC_VSWITCHD_STOP(["/Unsupported big reassembled v4 packet/d"]) AT_CLEANUP AT_SETUP([conntrack - IPv4 fragmentation expiry]) @@ -4897,7 +4901,11 @@ NS_CHECK_EXEC([at_ns0], [ping6 -s 3200 -q -c 3 -i 0.3 -W 2 fc00::2 | FORMAT_PING 3 packets transmitted, 3 received, 0% packet loss, time 0ms ]) -OVS_TRAFFIC_VSWITCHD_STOP +dnl Ipv6 max packet size fragmentation dropped. +NS_EXEC([at_ns0], [ping6 -s 65487 -q -c 1 -W 0.5 fc00::2]) +OVS_CHECK_FRAG_LARGE() + +OVS_TRAFFIC_VSWITCHD_STOP(["/Unsupported big reassembled v6 packet/d"]) AT_CLEANUP AT_SETUP([conntrack - IPv6 fragmentation expiry]) diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at index c1be97347..49b277a08 100644 --- a/tests/system-userspace-macros.at +++ b/tests/system-userspace-macros.at @@ -298,6 +298,14 @@ AT_CHECK([ovs-appctl dpctl/ipf-get-status -m | FORMAT_FRAG_LIST()], [], [dnl ]) ]) +# OVS_CHECK_FRAG_LARGE() +# +# The userspace needs to check that ipf larger fragments have occurred. +m4_define([OVS_CHECK_FRAG_LARGE], +[ + OVS_WAIT_UNTIL([grep -Eq 'Unsupported big reassembled (v4|v6) packet' ovs-vswitchd.log]) +]) + # OVS_CHECK_MIN_KERNEL([minversion], [maxversion]) # # The userspace skips all tests that check kernel version.
Fix the coredump of ovs_assert, The backtrace is as follows, #8 0x0000561ee52084bb in ovs_assert_failure (where=<>, function=<>, condition=<>) at ../lib/util.c:89 #9 0x0000561ee50f8ab2 in dp_packet_set_size (b=<>, v=<>) at ../lib/dp-packet.h:687 #10 dp_packet_set_size (v=<>, b=0x7f7f88143e80) at ../lib/dp-packet.h:687 #11 dp_packet_put_uninit (size=395, b=0x7f7f88143e80) at ../lib/dp-packet.c:355 #12 dp_packet_put (b=0x7f7f88143e80, p=0x7f7f88143ce2, size=395) at ../lib/dp-packet.c:376 #13 0x0000561ee512c147 in ipf_reassemble_v4_frags (ipf_list=0x7f7f88110810) at ../lib/ipf.c:430 The mbuf data_len is a uint16_t field, which includes the ether header. So does IPv6. Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.") Signed-off-by: Faicker Mo <faicker.mo@zenlayer.com> --- v3: - Address comments from Aaron: - Test case fixed. --- lib/ipf.c | 6 ++++-- tests/system-kmod-macros.at | 8 ++++++++ tests/system-traffic.at | 12 ++++++++++-- tests/system-userspace-macros.at | 8 ++++++++ 4 files changed, 30 insertions(+), 4 deletions(-) -- 2.34.1