"Ananyev, Konstantin" <konstantin.anan...@intel.com> writes:
>> >> >> The IPv4 specification says that each fragment must at least the size >> >> >> of >> >> >> an IP header plus 8 octets. When attempting to run ipfrag using a >> >> >> smaller size, the fragment library will return successful completion, >> >> >> even though it is a violation of RFC791 (and updates). >> >> >> >> >> >> Signed-off-by: Aaron Conole <acon...@redhat.com> >> >> >> --- >> >> >> 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/librte_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_in, >> >> >> uint16_t fragment_offset, flag_offset, frag_size; >> >> >> uint16_t frag_bytes_remaining; >> >> >> >> >> >> + /* >> >> >> + * Ensure the IP fragmentation size is at least iphdr length + >> >> >> 8 octets >> >> >> + */ >> >> >> + if (unlikely(mtu_size < (sizeof(struct rte_ipv4_hdr) + >> >> >> 8*sizeof(char)))) >> >> >> + return -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 it >> >> that way to begin. >> >> >> >> > Why do we need extra checking here? >> >> >> >> These are error conditions to submit to fragmentation module. Someone >> >> 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), then >> >> 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. >> >> I'll add documentation as another patch. >> >> > After another thought - these functions are quite heavy-weighed anyway, >> > so probably formal parameter checking, something like: >> > if (pkt_in == NULL || pkts_out == NULL || pool_direct == NULL || >> > pool_indirect == NULL || mtu < MIN_MTU) >> > return -EINVAL; >> > >> > wouldn't introduce any real slowdown. >> >> Agreed. >> >> > 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 >> > header). >> > 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. >> >> 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 checking. > 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. >> >> > Konstantin >> > >> >> >> >> >> /* >> >> >> * Ensure the IP payload length of all fragments is aligned to a >> >> >> * multiple of 8 bytes as per RFC791 section 2.3. >> >> >> -- >> >> >> 2.25.1
