On Wed, Aug 10, 2011 at 01:07:28PM +0200, Henning Brauer wrote: > this is indeed the way it was supposed to work.
I dissagree. This is not at all what my understanding was of how it was supposed to work. You'd have to talk to dhartmei about his original intent, but as far as I recall the current behaviour was a conscious decision at least when I implemented 'max-src-conn' and 'max-src-states'. I'll admit that the manpage is wrong, though. One of my greatest ongoing frustrations is the rampant inconsistency in the pf.conf syntax, so: 1) I strongly oppose changing the behaviour of the existing 'max' state option. Nothing else there, or in any of the other ( ) 'option' setions, applies until AFTER that rule has been selected as the last-matching rule, and I think we should keep it that way. 2) whatever you do to 'max' should also be done to 'max-src-conn' and 'max-src-states'. If we want this functionality, I think it should be done as a separate keyword, with the rest of the parameters: pass in on em0 from any to $webhost states = 0 or pass in on em0 from any to $webhost src-states > 500 rdr-to $tarpit But it's not clear to me that this is actually what's wanted here. mikeb, can you explain a little more clearly what you're trying to accomplish with these 'match only one state' rules? P.S. Is it really worth slowing down the inner evaluation pf_test_rule() loop for this relatively little-used feature? > * David Gwynne <[email protected]> [2011-08-10 05:44]: > > this reflects my understanding of how this was supposed to work. i have > > written rules with the expectation that they work this way. > > > > On 10/08/2011, at 4:56 AM, Mike Belopuhov wrote: > > > > > Hi, > > > > > > I'd like to propose the following change in the pf ruleset evaluation: > > > > > > Index: pf.c > > > =================================================================== > > > RCS file: /home/cvs/src/sys/net/pf.c,v > > > retrieving revision 1.770 > > > diff -u -p -r1.770 pf.c > > > --- pf.c 3 Aug 2011 12:28:40 -0000 1.770 > > > +++ pf.c 9 Aug 2011 15:50:42 -0000 > > > @@ -2924,6 +2924,11 @@ pf_test_rule(struct pf_rule **rm, struct > > > PF_TEST_ATTRIB((r->match_tag && !pf_match_tag(m, r, &tag)), > > > TAILQ_NEXT(r, entries)); > > > PF_TEST_ATTRIB((r->rcv_kif && !pf_match_rcvif(m, r)), > > > + TAILQ_NEXT(r, entries)); > > > + > > > + /* see if a rule has reached a maximum number of states */ > > > + PF_TEST_ATTRIB((r->anchor == NULL && r->keep_state && > > > + r->max_states && r->states_cur >= r->max_states), > > > TAILQ_NEXT(r, entries)); > > > > > > /* FALLTHROUGH */ > > > > > > > > > I have an application that dynamically inserts rules like that into > > > the anchor: > > > > > > anchor "foo/*" all { > > > anchor "7" all { > > > pass in quick on rdomain 0 inet proto udp from 10.0.2.2 to 10.0.1.2 > > > port > > = 759 flags S/SA keep state (max 1) rtable 0 prio 0 > > > } > > > anchor "8" all { > > > pass in quick on rdomain 0 inet proto udp from 10.0.2.2 to 10.0.1.2 > > > port > > = 759 flags S/SA keep state (max 1) rtable 0 prio 0 > > > } > > > } > > > > > > as you can see source and destination addresses as well as a destination > > > port are the same, but these rules serve for two different concurrent > > > connections (source ports are different). > > > > > > Currently this won't work because the first rule matches the second > > > connection and when pf_create_state bails out we are already not in > > > the rule evaluation loop and drop the packet. > > > > > > I consider this to be a misfeature and prefer the ruleset evaluation > > > to continue, hence the diff. What do you think? Any objections? > > > > > > And just to be clear, this is NOT considered for the 5.0 release.
