Page MenuHomeFreeBSD
Differential D47953

pf: Force logging if pf_create_state() fails
Needs ReviewPublic
Actions

Authored by markj on Dec 6 2024, 10:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 5:36 AM
Unknown Object (File)
Thu, Dec 26, 7:53 AM
Unknown Object (File)
Thu, Dec 26, 6:42 AM
Unknown Object (File)
Wed, Dec 25, 10:50 PM
Unknown Object (File)
Dec 11 2024, 4:41 PM
Unknown Object (File)
Dec 10 2024, 7:49 PM
Subscribers
farrokhi
franco_opnsense.org
glebius
imp
melifaro
vegeta_tuxpowered.net

Details

Reviewers
kp
vegeta_tuxpowered.net
Summary

Currently packets are logged before pf_create_state() is called, so we
might log a packet as passed that is subsequently dropped due to state
creation failure. In particular, the drop is not logged, which is
wrong.

Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.

Add a regression test.

Co-authored by: Franco Fichtner <franco@opnsense.org>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 61050
Build 57934: arc lint + arc unit

Event Timeline

markj created this revision.Dec 6 2024, 10:06 PM
Herald added subscribers: vegeta_tuxpowered.net, glebius, melifaro and 2 others. · View Herald TranscriptDec 6 2024, 10:06 PM
Harbormaster completed remote builds in B60994: Diff 147626.Dec 6 2024, 10:06 PM
markj requested review of this revision.Dec 6 2024, 10:06 PM
markj added a reviewer: vegeta_tuxpowered.net.Dec 6 2024, 10:07 PM
kp added a comment.Dec 9 2024, 10:25 AM

Does this need an MFC tag too?

tests/sys/netpfil/pf/pflog.sh
139

That description doesn't seem to really describe the point of this test.

151

Why the alias?

vegeta_tuxpowered.net added a comment.Dec 9 2024, 11:08 AM

Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.

This seems more like a workaround than a fix of underlying design issues. Since I'm responsible for the mess of pf_rule_actions and match rules, I might be able to propose an alternative solution. Can you wait a few days?

markj marked 2 inline comments as done.Dec 9 2024, 3:28 PM
In D47953#1094057, @vegeta_tuxpowered.net wrote:

Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.

This seems more like a workaround than a fix of underlying design issues.

This is certainly true.

Since I'm responsible for the mess of pf_rule_actions and match rules, I might be able to propose an alternative solution. Can you wait a few days?

Sure, thank you, I'm not in any particular rush. I will note that the problem appears to exist in OpenBSD as well, by code inspection.

markj updated this revision to Diff 147734.Dec 9 2024, 3:28 PM
  • Add a useful test description.
  • Remove an unneeded interface alias.
Harbormaster completed remote builds in B61050: Diff 147734.Dec 9 2024, 3:28 PM
franco_opnsense.org added a subscriber: franco_opnsense.org.Mon, Dec 16, 5:52 PM
markj added a comment.Fri, Dec 20, 4:02 PM
In D47953#1094057, @vegeta_tuxpowered.net wrote:

Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.

This seems more like a workaround than a fix of underlying design issues. Since I'm responsible for the mess of pf_rule_actions and match rules, I might be able to propose an alternative solution. Can you wait a few days?

Just a gentle poke: do you have a specific change in mind?

Revision Contents
Changeset List

PathSize
sys/
netpfil/
pf/
1 line
tests/
sys/
netpfil/
pf/
64 lines

Diff 147734

View Options

sys/netpfil/pf/pf.c

Loading...
View Options

tests/sys/netpfil/pf/pflog.sh

Loading...