svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf

[Hans Petter Selasky]

On 04/02/15 22:26, Hans Petter Selasky wrote:
> On 04/02/15 20:46, Robert Watson wrote:
>> On Thu, 2 Apr 2015, Hans Petter Selasky wrote:
>>
>>>>> Does somebody here know what happens in these two cases:
>>>>>
>>>>> If we are transmitting using TSO, will the network adapter increment
>>>>> the IP ID field somehow? What happens if an outgoing IP packet
>>>>> resulting from a TSO packet get fragmented by a router?
>>>>
>>>> Quite possibly -- this is presumably specified by the NIC vendor, but
>>>> it would be good to do a bit of a survey and see what happens in
>>>> practice.
>>>>
>>>>> In ip_fragment() when we create fragments we should increment the
>>>>> ip_id value for each fragment?
>>>
>>> I'm asking because the code in FreeBSD, since the beginning probably,
>>> just copies the IP header, and use the same IP ID for all the
>>> fragments ! This just hit my mind after some recent work in this area.
>>
>> I honestly cannot believe you are proposing that.
>>
>> Please go read about how IP fragmentation works.  Having an identical IP
>> ID in ip_fragment() is the point of the function!
>>
>
> Hi,
>
> rwatson: You're right, the more fragment flag gets set there, I
> overlooked that bit. Sorry.
>
> glebius: Given that you admit there is a small chance of an IP ID
> collision in the previous e-mails exchanged in this thread, why don't we
> have checks for that in ip_reass() when receiving fragmented IP packets?
> For example when ip->ip_off == 0 we know the TCP and/or UDP port numbers
> for TCP and UDP payloads and can check if a new fragment is starting
> before the previous one is completed. Then we would know if a collision
> has happened and could discard that packet. Not ideal, but better than
> data corruption.
>

Hi,

I see from the code that if two frags have the same IP offset, the whole 
fragment list gets dropped, unless the IP payload is zero bytes long. 
Maybe a "last" variable should be added?

>          * only n will ever be stored. (n = maxfragsperpacket.)
>          *
>          */
>         next = 0;
           last = -1;
>         for (p = NULL, q = fp->ipq_frags; q; p = q, q = q->m_nextpkt) {
>                 if (ntohs(GETIP(q)->ip_off) != next  ||
+			ntohs(GETIP(q)->ip_off) == last
>			) {
>                         if (fp->ipq_nfrags > V_maxfragsperpacket) {
>                                 IPSTAT_ADD(ips_fragdropped, fp->ipq_nfrags);
>                                 ip_freef(head, fp);
>                         }
>                         goto done;
>                 }
		  last = next;
>                 next += ntohs(GETIP(q)->ip_len);
>         }

--HPS