[dpdk-dev] ipv4 fragmentation bug?

[dpdk-dev] ipv4 fragmentation bug?

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

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.

P.S.
I am using DPDK v 2.2.0

-- 
--
Kiselev Alexander

Re: [dpdk-dev] ipv4 fragmentation bug?

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

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.

2016-08-15 20:30 GMT+03:00 Александр Киселев <kiselev99@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.
>
> P.S.
> I am using DPDK v 2.2.0
>
> --
> --
> Kiselev Alexander
>



-- 
--
Kiselev Alexander

Re: [dpdk-dev] ipv4 fragmentation bug?

[Thomas Monjalon]

Hi,

> 2016-08-15 20:30 GMT+03:00 Александр Киселев <kiselev99@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?

Re: [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@6wind.com>:
>Hi,

> 2016-08-15 20:30 GMT+03:00 Александр Киселев <kiselev99@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