From owner-freebsd-net@FreeBSD.ORG Thu Feb 12 20:59:14 2015 Return-Path: Delivered-To: freebsd-net@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 467297FF for ; Thu, 12 Feb 2015 20:59:14 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [IPv6:2001:4f8:3:ffe0:406a:0:50:2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 0D723AD9 for ; Thu, 12 Feb 2015 20:59:14 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [127.0.1.5]) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9) with ESMTP id t1CKxDKa061444 for ; Thu, 12 Feb 2015 20:59:13 GMT (envelope-from root@phabric-backend.isc.freebsd.org) Received: (from root@localhost) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9/Submit) id t1CKxDCF061443; Thu, 12 Feb 2015 20:59:13 GMT (envelope-from root) Date: Thu, 12 Feb 2015 20:59:13 +0000 To: freebsd-net@freebsd.org From: "glebius (Gleb Smirnoff)" Subject: [Differential] [Changed Subscribers] D1765: PF: Handle fragmented IPv6 packets Message-ID: <7639ebd9d955942a3d48076026732098@localhost.localdomain> X-Priority: 3 Thread-Topic: D1765: PF: Handle fragmented IPv6 packets X-Herald-Rules: none X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: In-Reply-To: References: Thread-Index: MzdiMDI4N2YyYWVmNTFkOWE2N2EwODM4MzY0IFTdFCE= X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: , MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Feb 2015 20:59:14 -0000 glebius added a subscriber: glebius. glebius added a comment. Kristof, big thanks for working on this. See my comments. INLINE COMMENTS sys/netpfil/pf/pf.c:366 This function can also be used not only for fragment rbtree, but can also substitute the PF_ANEQ, PF_AEQ and could be considered to substitute even pf_match_addr(). So, lots of code can be generalized using this function. That's good. I'd suggest not to inline it, and rename it to pf_addr_cmp. "cmp" is a widely used abbreviation clear to anyone. sys/netpfil/pf/pf.c:396 Please add a panic() for default switch case. sys/netpfil/pf/pf_norm.c:64 Please use C99 types in new code instead of 80-ish BSD types: uint32_t, uint16_t, uint8_t. sys/netpfil/pf/pf_norm.c:72 When hacking pf, I strongly dislike this struct foo and struct foo_cmp, that must be kept synchronized. I skipped fixing that in 2012 and now I regret that. Let's now do it better in the new code. You can just embed pf_fragment_cmp into pf_fragment. Alternatively, you can do this trick: #define fr_cmp_offset fr_entry and in code you do: bcmp(a, b, offsetof(struct pf_fragment, fr_cmp_offset)) I prefer the trick over embedding, but that's up to you. sys/netpfil/pf/pf_norm.c:340 Empty line here is style(9) requirement, shouldn't be removed. sys/netpfil/pf/pf_norm.c:403 Let's put PF_FRAG_ASSERT() at the beginning of this function. Kinda documenting its locking requirements. sys/netpfil/pf/pf_norm.c:459 Dots in end of comments throught the function, please. Thanks :) sys/netpfil/pf/pf_norm.c:735 Please use KASSERT: m = m_getptr(m, hdrlen + offsetof(struct ip6_frag, ip6f_nxt), &off); KASSERT(m, ("%s: short mbuf chain", __func__)); sys/netpfil/pf/pf_norm.c:757 Same here. REVISION DETAIL https://reviews.freebsd.org/D1765 To: kristof Cc: glebius, freebsd-net