From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 06E69A0597; Wed, 8 Apr 2020 17:45:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id DB1891C0C3; Wed, 8 Apr 2020 17:45:17 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by dpdk.org (Postfix) with ESMTP id 586C81C0C2 for ; Wed, 8 Apr 2020 17:45:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586360716; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lf4s1s4vHT47fycDHmDbH/dMy721IXINH5n8C+lrLq8=; b=MmOd7Z7yxJXn9pIGM+NXxu9lIYMDbhS7k3eq5SZY/fS0Gqw0wnQ1cAvHBuQQjF3G1t99eL QWN1Btuvt7NfuuZlhFEp9QXKXZ+Bi21wPHuNgjq4Xl5H1xXjaPMCc/WJXQwj1U0a1Cs2sO eHyHvRLcTXgCS2aUQgJ8kYF05EkBWno= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-359-JucXzQfPOga7XJ4Ag6EmbQ-1; Wed, 08 Apr 2020 11:45:13 -0400 X-MC-Unique: JucXzQfPOga7XJ4Ag6EmbQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E79698017F5; Wed, 8 Apr 2020 15:45:11 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-116-136.phx2.redhat.com [10.3.116.136]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A418419925; Wed, 8 Apr 2020 15:45:07 +0000 (UTC) From: Aaron Conole To: "Ananyev\, Konstantin" Cc: "dev\@dpdk.org" , Sunil Kumar Kori , "Burakov\, Anatoly" , Chas Williams , "Richardson\, Bruce" , "David Marchand" References: <20200401131849.2209336-1-aconole@redhat.com> <20200401183917.3620845-1-aconole@redhat.com> <20200401183917.3620845-2-aconole@redhat.com> Date: Wed, 08 Apr 2020 11:45:06 -0400 In-Reply-To: (Konstantin Ananyev's message of "Wed, 8 Apr 2020 12:37:24 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" "Ananyev, Konstantin" writes: >> >> >> The IPv4 specification says that each fragment must at least the s= ize of >> >> >> an IP header plus 8 octets. When attempting to run ipfrag using a >> >> >> smaller size, the fragment library will return successful completi= on, >> >> >> even though it is a violation of RFC791 (and updates). >> >> >> >> >> >> Signed-off-by: Aaron Conole >> >> >> --- >> >> >> lib/librte_ip_frag/rte_ipv4_fragmentation.c | 6 ++++++ >> >> >> 1 file changed, 6 insertions(+) >> >> >> >> >> >> diff --git a/lib/librte_ip_frag/rte_ipv4_fragmentation.c b/lib/lib= rte_ip_frag/rte_ipv4_fragmentation.c >> >> >> index 9e9f986cc5..4baaf6355c 100644 >> >> >> --- a/lib/librte_ip_frag/rte_ipv4_fragmentation.c >> >> >> +++ b/lib/librte_ip_frag/rte_ipv4_fragmentation.c >> >> >> @@ -76,6 +76,12 @@ rte_ipv4_fragment_packet(struct rte_mbuf *pkt_i= n, >> >> >> =09uint16_t fragment_offset, flag_offset, frag_size; >> >> >> =09uint16_t frag_bytes_remaining; >> >> >> >> >> >> +=09/* >> >> >> +=09 * Ensure the IP fragmentation size is at least iphdr length += 8 octets >> >> >> +=09 */ >> >> >> +=09if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + 8*sizeo= f(char)))) >> >> >> +=09=09return -EINVAL; >> >> >> + >> >> > >> >> > Same comment as for ipv6: ipv4 min MTU is 68B. >> >> >> >> I can change it. I suspected that if I went with 68 here and 1280 in >> >> the v6 code, I would get pushback, but I should have just submitted i= t >> >> that way to begin. >> >> >> >> > Why do we need extra checking here? >> >> >> >> These are error conditions to submit to fragmentation module. Someon= e >> >> needs to do the check - either it is done in the application or the >> >> library. If the library doesn't, and the application writer doesn't >> >> know they must write these checks (it isn't documented anywhere), the= n >> >> we get non compliant behavior. By putting it in the library, we can >> >> clearly signal the application writer such a case has occurred. >> >> >> >> Should we not do error checking? >> > >> > It depends I think... >> > In many data-path functions we skip parameter checking. >> > These fragment() functions are data-path too. >> > Agree, it is not stated clearly in these functions formal comments, >> > as it should be. >>=20 >> I'll add documentation as another patch. >>=20 >> > After another thought - these functions are quite heavy-weighed anyway= , >> > so probably formal parameter checking, something like: >> > if (pkt_in =3D=3D NULL || pkts_out =3D=3D NULL || pool_direct =3D=3D N= ULL || >> > =09=09pool_indirect =3D=3D NULL || mtu < MIN_MTU) >> > =09return -EINVAL; >> > >> > wouldn't introduce any real slowdown. >>=20 >> Agreed. >>=20 >> > About more intense checking - like parsing all extension >> > headers, etc. - I think it would be too much overhead. >> > Again for that there is a special function that user can call directly= : >> > rte_ipv6_frag_get_ipv6_fragment_header >> > (though its current implementation also checks only first extension he= ader). >> > So, I think we just need to document that it is a user responsibility = to >> > provide not fragmented yet packet, without any pre-fragment headers. >>=20 >> Makes sense. Then again, the v6 frag code will need to preserve many of >> the headers anyway, so since we have to read them, maybe it makes >> sense to do the check here anyway. WDYT? > > If we want to make this function fully compliant to what rfc8200 says, > then yes - extra changes is required in current implementation: > 1. somehow obtain information about pre-fragment extensions length > 2. use info from #1 to put fragment header at proper location. > And extra testing of course. I think we should. I know there are projects relying on it. > Probably safer and easier, for that patch just add formal parameter check= ing. > And if you feel like that - have the hard part as a separate patch. Okay, I'll resubmit the series with minimal ipv6 unit tests, and then submit another series which brings the frag header behavior in line. >>=20 >> > Konstantin >> > >> >> >> >> >> =09/* >> >> >> =09 * Ensure the IP payload length of all fragments is aligned to= a >> >> >> =09 * multiple of 8 bytes as per RFC791 section 2.3. >> >> >> -- >> >> >> 2.25.1