[Differential] [Changed Subscribers] D1765: PF: Handle fragmented IPv6 packets

[glebius (Gleb Smirnoff)]

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