[v3,1/4] ip_frag: ensure minimum v4 fragmentation length
Checks
Commit Message
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(+)
Comments
>
> 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.
Why do we need extra checking here?
> /*
> * 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
"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?
>> /*
>> * 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
>
> "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.
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
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:
>>
>> "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?
> 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
> >> >> 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.
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.
>
> > 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
"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
@@ -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;
+
/*
* Ensure the IP payload length of all fragments is aligned to a
* multiple of 8 bytes as per RFC791 section 2.3.