pflog: Correctly check if bpf peers are present
ClosedPublic
Actions

Authored by zlei on Fri, Jun 7, 5:15 PM.

Details

Reviewers

Commits

rGcbea6b60da00: pflog: Correctly check if bpf peers are present
rG954e548b7e88: pflog: Correctly check if bpf peers are present
rGebc2bab04823: pflog: Correctly check if bpf peers are present

Summary

On creating the pflog(4) interface, pflog_clone_create() does an
unconditional bpfattach(). Use bpf_peers_present() which was introduced
in commit 16d878cc99ef [1] to check the presence of bpf peers.

This will save a little CPU cycles. There should be no functional
change.

16d878cc99ef Fix the following bpf(4) race condition which can result in a panic

MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

zlei created this revision.Fri, Jun 7, 5:15 PM

Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro and 2 others. · View Herald TranscriptFri, Jun 7, 5:15 PM

zlei requested review of this revision.Fri, Jun 7, 5:15 PM

zlei added inline comments.Fri, Jun 7, 5:19 PM

sys/netpfil/pf/if_pflog.c
272	IIUC `in_cksum()` would modify the mbuf. Is that intended for pflog(4) ?
277	And these two counters... I'm not sure if they should be increased only when bpf peers present.

kp accepted this revision.Fri, Jun 7, 5:27 PM

kp added inline comments.

sys/netpfil/pf/if_pflog.c
272	I don't think it's expected to modify the mbuf. in_cksum() maps to in_cksum_skip(), which is implemented in sys/netinet/in_cksum.c and as far as I can see it only reads the mbuf. Arguably in_cksum_skip() should take a const struct mbuf * rather than a struct mbuf *, but I think it can't because m_apply() could be used to modify the mbuf (so in_cksum_skip() can't declare m to be const and then pass it to m_apply()).
277	I think it could be argued either way. On the one hand, the interface does conceptually see all of these packets, but on the other hand, it does nothing if there are no attached bpf peers. Given that, I'd just keep things as they are.