[dpdk-dev] [PATCH v3 1/4] ip_frag: ensure minimum v4 fragmentation length

Ananyev, Konstantin konstantin.ananyev at intel.com
Tue Apr 7 16:14:34 CEST 2020


> 
> "Ananyev, Konstantin" <konstantin.ananyev at 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 <aconole at 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.
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.
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.
 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



More information about the dev mailing list