From owner-svn-src-head@FreeBSD.ORG Thu Apr 2 21:20:08 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 473B99B7; Thu, 2 Apr 2015 21:20:08 +0000 (UTC) Received: from cyrus.watson.org (cyrus.watson.org [198.74.231.69]) by mx1.freebsd.org (Postfix) with ESMTP id 01FB67E6; Thu, 2 Apr 2015 21:20:08 +0000 (UTC) Received: from [10.0.1.17] (host81-157-243-31.range81-157.btcentralplus.com [81.157.243.31]) by cyrus.watson.org (Postfix) with ESMTPSA id CDE9546BB1; Thu, 2 Apr 2015 17:20:06 -0400 (EDT) Content-Type: text/plain; charset=windows-1252 Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2070.6\)) Subject: Re: svn commit: r280971 - in head: contrib/ipfilter/tools share/man/man4 sys/contrib/ipfilter/netinet sys/netinet sys/netipsec sys/netpfil/pf From: "Robert N. M. Watson" In-Reply-To: <551DAC9E.9010303@selasky.org> Date: Thu, 2 Apr 2015 22:20:05 +0100 Content-Transfer-Encoding: quoted-printable Message-Id: <358EC58D-1F92-411E-ADEB-8072020E9EB3@FreeBSD.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> <551D8143.4060509@selasky.org> <551D8945.8050906@selasky.org> <8900318B-8155-4131-A0C3-3DE169782EFC@FreeBSD.org> <551D8C6C.9060504@selasky.org> <551DA5EA.1080908@selasky.org> <551DAC9E.9010303@selasky.org> To: Hans Petter Selasky X-Mailer: Apple Mail (2.2070.6) Cc: Mateusz Guzik , Ian Lepore , svn-src-all@freebsd.org, src-committers@freebsd.org, Gleb Smirnoff , svn-src-head@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Apr 2015 21:20:08 -0000 On 2 Apr 2015, at 21:54, Hans Petter Selasky 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