Faicker Mo <[email protected]> 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 <[email protected]>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
