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

[Aaron Conole <aconole@redhat.com>]

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