[dpdk-dev] ipv4 fragmentation bug?

[Александр Киселев]

I am sorry for the late reply.

I am not sure anymore about is it a bug I found or the author of
rte_ipv4_fragment_packet() realy
wanted to constraint the size of mtu writing lines:

frag_size = (uint16_t)(mtu_size - sizeof(struct ipv4_hdr));
/* Fragment size should be a multiply of 8. */
assert((frag_size & IPV4_HDR_FO_MASK) == 0);

So, if we assume that any mtu size is valid then it's a bug and the
function must be rewriten.
Otherwise, since mtu_size is an input parameter of the function,
validation should be stronger than
just a assertion or it should be noted in the documentation that
not all values for the paremater mtu_size are valid.

I can write a patch. I just need a confirmation since
I am not sure about the networking background regarding MTU.
I tried to find anything about MTU in the RFC, but so far nothing.
According to RFC all mtu sizes are valid.



>2016-08-22 14:31 GMT+03:00 Thomas Monjalon <thomas.monjalon at 6wind.com>:
>Hi,

> 2016-08-15 20:30 GMT+03:00 Александр Киселев <kiselev99 at gmail.com>:
> > While playing with function rte_ipv4_fragment_packet I found that it
> > incorrectly fragments packets.
> > For example if the function takes 1200 bytes packet and mtu size 1000 it
> > will produces two fragments. And when those fragments are reassembled back
> > the resulting packet will be 4 bytes shorter than it should be.
> >
> > I played with linux ping program and it reports that a reply is truncated.
> >     1204 bytes from 192.168.125.1: icmp_seq=1 ttl=64 (truncated)
> >
> > Looking at the source of rte_ipv4_fragment_packet I discovered the cause
> > of the above behavior.
> >
> > Function makes the following assumption and the whole calculations are
> > bases on that assumption.
> >
> > /* Fragment size should be a multiply of 8. */
> > IP_FRAG_ASSERT((frag_size & IPV4_HDR_FO_MASK) == 0);
> >
> > The problem is that this assert doesn’t make any sense. It's true that
> > fragment size should be a multiply of 8, but what this line real checks is
> > that
> > the size of mtu minus 20 bytes should be multiply of 8. In other words
> > it constrains the size of the mtu. So, if I take valid mtu value, say
> > 1504,
> > it will produce incorrect fragments when asserts are off.

>Thanks for reporting.
>
>2016-08-15 20:48, Александр Киселев:
>> I'am sorry. Looks like having an mtu value multiply of 8 is a good practice.
>>
>> But mtu value 1504 is also widely used in qinq linux interfaces.

>Please, would like to write a patch for master branch?
>Or do you prefer to delegate it to someone reading this thread?



--
Alexander Kiselev