Date: Thu, 12 Feb 2015 20:59:13 +0000 From: "glebius (Gleb Smirnoff)" <phabric-noreply@FreeBSD.org> To: freebsd-net@freebsd.org Subject: [Differential] [Changed Subscribers] D1765: PF: Handle fragmented IPv6 packets Message-ID: <7639ebd9d955942a3d48076026732098@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-cqydhpifjxaajzlrtfep-req@FreeBSD.org> References: <differential-rev-PHID-DREV-cqydhpifjxaajzlrtfep-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7639ebd9d955942a3d48076026732098>