Date: Thu, 2 Apr 2015 22:20:05 +0100 From: "Robert N. M. Watson" <rwatson@FreeBSD.org> To: Hans Petter Selasky <hps@selasky.org> Cc: Mateusz Guzik <mjguzik@gmail.com>, Ian Lepore <ian@freebsd.org>, svn-src-all@freebsd.org, src-committers@freebsd.org, Gleb Smirnoff <glebius@FreeBSD.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf Message-ID: <358EC58D-1F92-411E-ADEB-8072020E9EB3@FreeBSD.org> In-Reply-To: <551DAC9E.9010303@selasky.org> References: <201504012226.t31MQedN044443@svn.freebsd.org> <1427929676.82583.103.camel@freebsd.org> <20150402123522.GC64665@FreeBSD.org> <20150402133751.GA549@dft-labs.eu> <20150402134217.GG64665@FreeBSD.org> <20150402135157.GB549@dft-labs.eu> <1427983109.82583.115.camel@freebsd.org> <20150402142318.GC549@dft-labs.eu> <20150402143420.GI64665@FreeBSD.org> <20150402153805.GD549@dft-labs.eu> <alpine.BSF.2.11.1504021657440.27263@fledge.watson.org> <551D8143.4060509@selasky.org> <551D8945.8050906@selasky.org> <8900318B-8155-4131-A0C3-3DE169782EFC@FreeBSD.org> <551D8C6C.9060504@selasky.org> <alpine.BSF.2.11.1504021939390.64391@fledge.watson.org> <551DA5EA.1080908@selasky.org> <551DAC9E.9010303@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2 Apr 2015, at 21:54, Hans Petter Selasky <hps@selasky.org> wrote:
>>> Please go read about how IP fragmentation works. Having an =
identical IP
>>> ID in ip_fragment() is the point of the function!
>>=20
>> rwatson: You're right, the more fragment flag gets set there, I
>> overlooked that bit. Sorry.
>>=20
>> 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 =3D=3D 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.
>=20
> 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?
Are you solving an actual problem you've observed, or is this a =
speculative proposal? RFC 791 requires that the minimum fragment size be =
8 octets, and ip_input() drops fragments below that size, so you =
shouldn't (in principle) be able to get a fragment in the reassembly =
code that has the properties you've described. If you are, it's a bug =
elsewhere in the stack. If you are worried, you could add an assertion =
at the top of the reassembly function that the size is >=3D 8 octets.
I think you would benefit a great deal from reading Stevens Volume I =
(second edition) before proceeding with further changes to the TCP/IP =
code stack. A number of the proposals you have made are contradictory to =
fundamental design choices in the protocol. Although there isn't a =
second edition of Volume II, it might still be a good idea to skim =
through the pertinent sections there as well -- it applies to a much =
(much) earlier version of the stack, but includes a detailed exposition =
of how the implementation tracks the protocol.
Robert
>> * only n will ever be stored. (n =3D maxfragsperpacket.)
>> *
>> */
>> next =3D 0;
> last =3D -1;
>> for (p =3D NULL, q =3D fp->ipq_frags; q; p =3D q, q =3D =
q->m_nextpkt) {
>> if (ntohs(GETIP(q)->ip_off) !=3D next ||
> + ntohs(GETIP(q)->ip_off) =3D=3D last
>> ) {
>> if (fp->ipq_nfrags > V_maxfragsperpacket) {
>> IPSTAT_ADD(ips_fragdropped, =
fp->ipq_nfrags);
>> ip_freef(head, fp);
>> }
>> goto done;
>> }
> last =3D next;
>> next +=3D ntohs(GETIP(q)->ip_len);
>> }
>=20
> --HPS
>=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?358EC58D-1F92-411E-ADEB-8072020E9EB3>