Re: removing expired once rules in pf_purge_thread()
Locked
Re: removing expired once rules in pf_purge_thread()
 
|
If I understand this patch: Rule removal requires the consistency lock but to acquire it one must be able to sleep. pf_test_rule() cannot sleep as it executes in a (soft) interrupt handler, so it passes the expired rule to a thread which can. However, the code to remove a 'once' rule may wish to also remove its parent rule, now computed in the other thread. The updated patch below avoids passing that state between threads in struct pf_rule by explictly passing the parent rule to be purged when necessary. There also seem to me a couple of issues, one hairier than the other: - r->rule_flag should be updated atomically with respect to other (potential) updates (included in the attached patch). - Multiple rule-matching threads may match the same 'once' rule before one of them reaches the code to set PFRULE_EXPIRED. A thread may even match a 'once' rule after PFRULE_EXPIRED is set, if that occurs during the 'once' rule's candidacy as a potential match. (This issue is not addressed by the attached patch.) This creates two problems: a) rules may be purged more than once b) what to do with packets matched by expired 'once' rules In general, we do not know whether or not a 'once' rule candidate is the match until a new candidate rule replaces it or the rule matching process completes. Serialising this window of candicacy would prevent a 'once' rule becoming a candidate, and possibly the match, for multiple packets at once. That's one approach. A weaker approach instead permits multiple matches of one 'once' rule but processes these serially in a critical section afterwards. The first thread to reach it is deemed the first match (note this isn't necessarily the first matching packet received at the interface!). It then atomically marks it PFRULE_EXPIRED and places it on the purge queue. Packets for matches of expired 'once' rule, it seems to me, can be dropped as if we never saw them. An alternative might be to retry them against the updated rules. Another possibility would be to require 'once' rules to be 'quick'. This closes the candidacy window and makes its serialisation, to preclude multiple matches, more feasible. Yet another possibility is to drop 'once' rules as too complex to implement for multiprocessor but I have no idea if this is viable. best, Richard. Tested via pf.conf set skip on lo pass block in proto icmp anchor selfdestruct { pass in proto icmp once no state } Index: net/pf.c =================================================================== --- net.orig/pf.c +++ net/pf.c @@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key, RB_GENERATE(pf_state_tree_id, pf_state, entry_id, pf_state_compare_id); +SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl = + SLIST_HEAD_INITIALIZER(pf_rule_gcl); + __inline int pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af) { @@ -1137,6 +1140,24 @@ pf_state_export(struct pfsync_state *sp, /* END state table stuff */ void +pf_purge_expired_rules(void) +{ + struct pf_rule *r; + + if (SLIST_EMPTY(&pf_rule_gcl)) { + return; + } + + rw_enter_write(&pf_consistency_lock); + while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) { + SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle); + KASSERT(r->rule_flag & PFRULE_EXPIRED); + pf_purge_rule(r); + } + rw_exit_write(&pf_consistency_lock); +} + +void pf_purge_thread(void *v) { int nloops = 0, s; @@ -1154,6 +1175,7 @@ pf_purge_thread(void *v) if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) { pf_purge_expired_fragments(); pf_purge_expired_src_nodes(0); + pf_purge_expired_rules(); nloops = 0; } @@ -3135,6 +3157,10 @@ pf_test_rule(struct pf_pdesc *pd, struct ruleset = &pf_main_ruleset; r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr); while (r != NULL) { + if (r->rule_flag & PFRULE_EXPIRED) { + r = TAILQ_NEXT(r, entries); + goto nextrule; + } r->evaluations++; PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot), r->skip[PF_SKIP_IFP].ptr); @@ -3433,8 +3459,18 @@ pf_test_rule(struct pf_pdesc *pd, struct } #endif /* NPFSYNC > 0 */ - if (r->rule_flag & PFRULE_ONCE) - pf_purge_rule(ruleset, r, aruleset, a); + if (r->rule_flag & PFRULE_ONCE) { + int only_rule = (TAILQ_NEXT(r, entries) == NULL && + TAILQ_PREV(r, pf_rulequeue, entries) == NULL); + + if (only_rule && a) { + atomic_setbits_int(&a->rule_flag, PFRULE_EXPIRED); + SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle); + } + + atomic_setbits_int(&r->rule_flag, PFRULE_EXPIRED); + SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); + } #ifdef INET6 if (rewrite && skw->af != sks->af) Index: net/pf_ioctl.c =================================================================== --- net.orig/pf_ioctl.c +++ net/pf_ioctl.c @@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu } void -pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule, - struct pf_ruleset *aruleset, struct pf_rule *arule) +pf_purge_rule(struct pf_rule *rule) { u_int32_t nr = 0; + struct pf_ruleset *ruleset; - KASSERT(ruleset != NULL && rule != NULL); + KASSERT((rule != NULL) && (rule->myruleset != NULL)); + ruleset = rule->myruleset; pf_rm_rule(ruleset->rules.active.ptr, rule); ruleset->rules.active.rcount--; @@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset rule->nr = nr++; ruleset->rules.active.ticket++; pf_calc_skip_steps(ruleset->rules.active.ptr); - - /* remove the parent anchor rule */ - if (nr == 0 && arule && aruleset) { - pf_rm_rule(aruleset->rules.active.ptr, arule); - aruleset->rules.active.rcount--; - TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries) - rule->nr = nr++; - aruleset->rules.active.ticket++; - pf_calc_skip_steps(aruleset->rules.active.ptr); - } } u_int16_t @@ -1209,6 +1200,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a } TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr, rule, entries); + rule->myruleset = ruleset; ruleset->rules.inactive.rcount++; break; } Index: net/pfvar.h =================================================================== --- net.orig/pfvar.h +++ net/pfvar.h @@ -563,6 +563,9 @@ struct pf_rule { struct pf_addr addr; u_int16_t port; } divert, divert_packet; + + SLIST_ENTRY(pf_rule) gcle; + struct pf_ruleset *myruleset; }; /* rule flags */ @@ -581,6 +584,7 @@ struct pf_rule { #define PFRULE_PFLOW 0x00040000 #define PFRULE_ONCE 0x00100000 /* one shot rule */ #define PFRULE_AFTO 0x00200000 /* af-to rule */ +#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */ #define PFSTATE_HIWAT 10000 /* default state table size */ #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */ @@ -1701,9 +1705,7 @@ extern void pf_addrcpy(struct pf_addr sa_family_t); void pf_rm_rule(struct pf_rulequeue *, struct pf_rule *); -void pf_purge_rule(struct pf_ruleset *, - struct pf_rule *, struct pf_ruleset *, - struct pf_rule *); +void pf_purge_rule(struct pf_rule *); struct pf_divert *pf_find_divert(struct mbuf *); int pf_setup_pdesc(struct pf_pdesc *, void *, sa_family_t, int, struct pfi_kif *, |
Re: removing expired once rules in pf_purge_thread()
|
|
On 13 December 2015 at 18:56, Richard Procter
<[hidden email]> wrote: > > If I understand this patch: > > Rule removal requires the consistency lock but to acquire it one must be > able to sleep. pf_test_rule() cannot sleep as it executes in a (soft) > interrupt handler, so it passes the expired rule to a thread which can. > > However, the code to remove a 'once' rule may wish to also remove its > parent rule, now computed in the other thread. This is right: "once" rules remove it's parent anchor if it's empty. > The updated patch below > avoids passing that state between threads in struct pf_rule by explictly > passing the parent rule to be purged when necessary. > > There also seem to me a couple of issues, one hairier than the other: > > - r->rule_flag should be updated atomically with respect to other > (potential) updates (included in the attached patch). > > - Multiple rule-matching threads may match the same 'once' rule before > one of them reaches the code to set PFRULE_EXPIRED. A thread may even > match a 'once' rule after PFRULE_EXPIRED is set, if that occurs during > the 'once' rule's candidacy as a potential match. (This issue is not > addressed by the attached patch.) > > This creates two problems: > > a) rules may be purged more than once > b) what to do with packets matched by expired 'once' rules > > In general, we do not know whether or not a 'once' rule candidate is > the match until a new candidate rule replaces it or the rule matching > process completes. Serialising this window of candicacy would prevent a > 'once' rule becoming a candidate, and possibly the match, for multiple > packets at once. That's one approach. > > A weaker approach instead permits multiple matches of one 'once' rule > but processes these serially in a critical section afterwards. The > first thread to reach it is deemed the first match (note this isn't > necessarily the first matching packet received at the interface!). It > then atomically marks it PFRULE_EXPIRED and places it on the purge > queue. Packets for matches of expired 'once' rule, it seems to me, can > be dropped as if we never saw them. An alternative might be to retry > them against the updated rules. > > Another possibility would be to require 'once' rules to be 'quick'. > This closes the candidacy window and makes its serialisation, to > preclude multiple matches, more feasible. > > Yet another possibility is to drop 'once' rules as too complex to > implement for multiprocessor but I have no idea if this is viable. > It is. And I have said that before with an authority of the implementor of "once" rules: since we don't have any userland applications that would use this yet, we can ditch them for now and possibly devise a better approach later. Don't make your lives harder than they have to be! |
Re: removing expired once rules in pf_purge_thread()
|
|
Hello,
> > > > Another possibility would be to require 'once' rules to be 'quick'. > > This closes the candidacy window and makes its serialisation, to > > preclude multiple matches, more feasible. > > > > Yet another possibility is to drop 'once' rules as too complex to > > implement for multiprocessor but I have no idea if this is viable. > > > > It is. And I have said that before with an authority of the implementor > of "once" rules: since we don't have any userland applications that > would use this yet, we can ditch them for now and possibly devise a > better approach later. > > Don't make your lives harder than they have to be! > I'm just trying out patch improved by Richard. I like his idea to put anchor rule to garbage collector list. I'd like to give it a try on Solaris. There might be small changes in details. IMO once rules are fine, and offloading hard task to service/garbage collector thread works just fine. regards sasha |
Re: removing expired once rules in pf_purge_thread()
|
|
On 15 December 2015 at 15:36, Alexandr Nedvedicky
<[hidden email]> wrote: > Hello, > >> > >> > Another possibility would be to require 'once' rules to be 'quick'. >> > This closes the candidacy window and makes its serialisation, to >> > preclude multiple matches, more feasible. >> > >> > Yet another possibility is to drop 'once' rules as too complex to >> > implement for multiprocessor but I have no idea if this is viable. >> > >> >> It is. And I have said that before with an authority of the implementor >> of "once" rules: since we don't have any userland applications that >> would use this yet, we can ditch them for now and possibly devise a >> better approach later. >> >> Don't make your lives harder than they have to be! >> > > I'm just trying out patch improved by Richard. I like his idea to put anchor > rule to garbage collector list. I'd like to give it a try on Solaris. There > might be small changes in details. > > IMO once rules are fine, and offloading hard task to service/garbage collector > thread works just fine. > > regards > sasha It just occurred to me that another possibility would be a match-only rule that matches one but doesn't involve any purging machinery. Right now we install ftp-proxy rules as having maximum number of states equal to 1, however there's a time window between the moment the state is no longer there, but anchor is not gone yet and it can potentially create more states which is not a problem for an eavesdropper who can sniff the plaintext protocol. |
Re: removing expired once rules in pf_purge_thread()
|
|
Hello,
> It just occurred to me that another possibility would be a match-only > rule that matches one but doesn't involve any purging machinery. Right > now we install ftp-proxy rules as having maximum number of states equal > to 1, however there's a time window between the moment the state is no > longer there, but anchor is not gone yet and it can potentially create > more states which is not a problem for an eavesdropper who can sniff > the plaintext protocol. I'm not sure I'm following you. IMO the expire flag should solve that problem. As soon as rule is marked as expired it can not be matched by packet any more. I'm attaching yet another version of patch. The list of changes to patch sent by Richard is as follows: - atomic operations are gone as we don't need them right now. those will be introduced as soon as we will get there. pf_test()/pf_test_rule() is still processing a single packet at a time. - it's better to use TAILQ_EMPTY() instead of checking link members to determine if last rule is being removed from anchor `a` - I've also realized we have to call pf_purge_expired_rules() from pf_commit_rules(). We have to make sure expired rules are gone from active lists. I think this is the best approach for pf@Puffy currently. It will be changed as soon as ruleset locks and reference counting for rules will be merged in. I had to also reshuffle a consistency lock a bit in pf_purge_thread(). - last but not least: silly name got renamed (s/myruleset/ruleset) so what do you think? OK? regards sasha ---------8<----------------8<----------------8<---------------8<-------- Index: pf.c =================================================================== RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.960 diff -u -p -r1.960 pf.c --- pf.c 6 Dec 2015 10:03:23 -0000 1.960 +++ pf.c 16 Dec 2015 00:52:22 -0000 @@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key, RB_GENERATE(pf_state_tree_id, pf_state, entry_id, pf_state_compare_id); +SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl = + SLIST_HEAD_INITIALIZER(pf_rule_gcl); + __inline int pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af) { @@ -1137,6 +1140,18 @@ pf_state_export(struct pfsync_state *sp, /* END state table stuff */ void +pf_purge_expired_rules(void) +{ + struct pf_rule *r; + + while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) { + SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle); + KASSERT(r->rule_flag & PFRULE_EXPIRED); + pf_purge_rule(r); + } +} + +void pf_purge_thread(void *v) { int nloops = 0, s; @@ -1154,6 +1169,11 @@ pf_purge_thread(void *v) if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) { pf_purge_expired_fragments(); pf_purge_expired_src_nodes(0); + if (!SLIST_EMPTY(&pf_rule_gcl)) { + rw_enter_write(&pf_consistency_lock); + pf_purge_expired_rules(); + rw_exit_write(&pf_consistency_lock); + } nloops = 0; } @@ -3135,6 +3155,10 @@ pf_test_rule(struct pf_pdesc *pd, struct ruleset = &pf_main_ruleset; r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr); while (r != NULL) { + if (r->rule_flag & PFRULE_EXPIRED) { + r = TAILQ_NEXT(r, entries); + goto nextrule; + } r->evaluations++; PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot), r->skip[PF_SKIP_IFP].ptr); @@ -3433,8 +3457,15 @@ pf_test_rule(struct pf_pdesc *pd, struct } #endif /* NPFSYNC > 0 */ - if (r->rule_flag & PFRULE_ONCE) - pf_purge_rule(ruleset, r, aruleset, a); + if (r->rule_flag & PFRULE_ONCE) { + if ((a != NULL) && TAILQ_EMPTY(a->ruleset->rules.active.ptr)) { + a->rule_flag |= PFRULE_EXPIRED; + SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle); + } + + r->rule_flag |= PFRULE_EXPIRED; + SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); + } #ifdef INET6 if (rewrite && skw->af != sks->af) Index: pf_ioctl.c =================================================================== RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.297 diff -u -p -r1.297 pf_ioctl.c --- pf_ioctl.c 3 Dec 2015 13:30:18 -0000 1.297 +++ pf_ioctl.c 16 Dec 2015 00:52:25 -0000 @@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu } void -pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule, - struct pf_ruleset *aruleset, struct pf_rule *arule) +pf_purge_rule(struct pf_rule *rule) { u_int32_t nr = 0; + struct pf_ruleset *ruleset; - KASSERT(ruleset != NULL && rule != NULL); + KASSERT((rule != NULL) && (rule->ruleset != NULL)); + ruleset = rule->ruleset; pf_rm_rule(ruleset->rules.active.ptr, rule); ruleset->rules.active.rcount--; @@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset rule->nr = nr++; ruleset->rules.active.ticket++; pf_calc_skip_steps(ruleset->rules.active.ptr); - - /* remove the parent anchor rule */ - if (nr == 0 && arule && aruleset) { - pf_rm_rule(aruleset->rules.active.ptr, arule); - aruleset->rules.active.rcount--; - TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries) - rule->nr = nr++; - aruleset->rules.active.ticket++; - pf_calc_skip_steps(aruleset->rules.active.ptr); - } } u_int16_t @@ -775,6 +766,9 @@ pf_commit_rules(u_int32_t ticket, char * int s, error; u_int32_t old_rcount; + /* make sure any expired rules get removed from active rules first */ + pf_purge_expired_rules(); + rs = pf_find_ruleset(anchor); if (rs == NULL || !rs->rules.inactive.open || ticket != rs->rules.inactive.ticket) @@ -1209,6 +1203,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a } TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr, rule, entries); + rule->ruleset = ruleset; ruleset->rules.inactive.rcount++; break; } Index: pfvar.h =================================================================== RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.426 diff -u -p -r1.426 pfvar.h --- pfvar.h 3 Dec 2015 14:05:28 -0000 1.426 +++ pfvar.h 16 Dec 2015 00:52:28 -0000 @@ -563,6 +563,9 @@ struct pf_rule { struct pf_addr addr; u_int16_t port; } divert, divert_packet; + + SLIST_ENTRY(pf_rule) gcle; + struct pf_ruleset *ruleset; }; /* rule flags */ @@ -581,6 +584,7 @@ struct pf_rule { #define PFRULE_PFLOW 0x00040000 #define PFRULE_ONCE 0x00100000 /* one shot rule */ #define PFRULE_AFTO 0x00200000 /* af-to rule */ +#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */ #define PFSTATE_HIWAT 10000 /* default state table size */ #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */ @@ -1669,6 +1673,7 @@ extern struct pool pf_state_scrub_pl; extern void pf_purge_thread(void *); extern void pf_purge_expired_src_nodes(int); extern void pf_purge_expired_states(u_int32_t); +extern void pf_purge_expired_rules(void); extern void pf_remove_state(struct pf_state *); extern void pf_remove_divert_state(struct pf_state_key *); extern void pf_free_state(struct pf_state *); @@ -1701,9 +1706,7 @@ extern void pf_addrcpy(struct pf_addr sa_family_t); void pf_rm_rule(struct pf_rulequeue *, struct pf_rule *); -void pf_purge_rule(struct pf_ruleset *, - struct pf_rule *, struct pf_ruleset *, - struct pf_rule *); +void pf_purge_rule(struct pf_rule *); struct pf_divert *pf_find_divert(struct mbuf *); int pf_setup_pdesc(struct pf_pdesc *, void *, sa_family_t, int, struct pfi_kif *, |
Re: removing expired once rules in pf_purge_thread()
|
|
In reply to this post by Mike Belopuhov-5
On Tue, 15 Dec 2015, Mike Belopuhov wrote: > > Yet another possibility is to drop 'once' rules as too complex to > > implement for multiprocessor but I have no idea if this is viable. > > It is. And I have said that before with an authority of the implementor > of "once" rules: since we don't have any userland applications that > would use this yet, we can ditch them for now and possibly devise a > better approach later. > Don't make your lives harder than they have to be! I tend to agree! And I can't see a way to reimplement it for a multithreaded pf without introducing downsides. Quoting pf.conf(5) "once - Creates a one shot rule that will remove itself from an active ruleset after the first match." A 'first' match presupposes a total ordering. This comes for free when pf is single-threaded but concurrent rule matching must take the trouble to re-impose it somewhere. (I assume that pf aims to do concurrent matching of packets against the ruleset.) One approach serialises the candidacy of 'once' rules during rule matching. This ensures an unambiguous first match but costs pf a latent glass jaw. In the worst case, a 'once' rule heads the ruleset and matches any packet. Its candidacy therefore covers the entire rule-matching process and so fully serialises it. The other approach serialises the packets matched by 'once' rules, after rule processing is done. This doesn't create a glass jaw but does permit the 'once' rule to match more than once. To my taste neither alternative is especially palatable. That said, a 'quick' 'once' rule has no candidacy window so it can be serialised by atomically testing/setting a flag when evaluating it. This also both avoids the glass jaw and provides at most one first match. (Note that this issue of determining the first match is independent of whether a 'once' rule, having matched, is purged.) On Wed, 16 Dec 2015, Mike Belopuhov wrote: > It just occurred to me that another possibility would be a match-only > rule that matches one but doesn't involve any purging machinery. That is an interesting idea. Like a 'quick' rule, a match rule has no candidacy window. However a 'once' match rule can no longer influence pass/block state --- I can't see how it is usefull? best, Richard. |
Re: removing expired once rules in pf_purge_thread()
|
|
Hello,
On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote: > > On Tue, 15 Dec 2015, Mike Belopuhov wrote: > > > > Yet another possibility is to drop 'once' rules as too complex to > > > implement for multiprocessor but I have no idea if this is viable. > > > > It is. And I have said that before with an authority of the implementor > > of "once" rules: since we don't have any userland applications that > > would use this yet, we can ditch them for now and possibly devise a > > better approach later. > > > Don't make your lives harder than they have to be! > > I tend to agree! And I can't see a way to reimplement it for a > multithreaded pf without introducing downsides. > > Quoting pf.conf(5) "once - Creates a one shot rule that will remove itself > from an active ruleset after the first match." > > A 'first' match presupposes a total ordering. This comes for free when pf > is single-threaded but concurrent rule matching must take the trouble to > re-impose it somewhere. (I assume that pf aims to do concurrent matching > of packets against the ruleset.) pf@solaris allows the race here. The price for correct behavior, which is to allow one and only one packet to hit the rule is too high (given the once rules are kind of special case). What has to be granted is there is one 'winner' only, which puts the rule to garbage collector list. The pragmatic approach wins here. regards sasha |
Re: removing expired once rules in pf_purge_thread()
|
|
On 16/12/15(Wed) 10:19, Alexandr Nedvedicky wrote:
> On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote: > > > > On Tue, 15 Dec 2015, Mike Belopuhov wrote: > > > > > > Yet another possibility is to drop 'once' rules as too complex to > > > > implement for multiprocessor but I have no idea if this is viable. > > > > > > It is. And I have said that before with an authority of the implementor > > > of "once" rules: since we don't have any userland applications that > > > would use this yet, we can ditch them for now and possibly devise a > > > better approach later. > > > > > Don't make your lives harder than they have to be! > > > > I tend to agree! And I can't see a way to reimplement it for a > > multithreaded pf without introducing downsides. Guys, if none of you can come with a valid reason to keep "once" rules please kill them. There's so much work to do to make pf(4) runnable on multiple CPUs in parallel that bikescheding/turd-polishing bits that are not used are IMHO not the way to go. |
Re: removing expired once rules in pf_purge_thread()
|
|
On Wed, Dec 16, 2015 at 12:35:41PM +0100, Martin Pieuchot wrote:
> On 16/12/15(Wed) 10:19, Alexandr Nedvedicky wrote: > > On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote: > > > > > > On Tue, 15 Dec 2015, Mike Belopuhov wrote: > > > > > > > > Yet another possibility is to drop 'once' rules as too complex to > > > > > implement for multiprocessor but I have no idea if this is viable. > > > > > > > > It is. And I have said that before with an authority of the implementor > > > > of "once" rules: since we don't have any userland applications that > > > > would use this yet, we can ditch them for now and possibly devise a > > > > better approach later. > > > > > > > Don't make your lives harder than they have to be! > > > > > > I tend to agree! And I can't see a way to reimplement it for a > > > multithreaded pf without introducing downsides. > > Guys, if none of you can come with a valid reason to keep "once" rules > please kill them. > > There's so much work to do to make pf(4) runnable on multiple CPUs in > parallel that bikescheding/turd-polishing bits that are not used are > IMHO not the way to go. The patch I've sent recently works just fine for Solaris. So ONCE rules are no issue. I like the idea of once rules as they make life potentially easier for application proxies. regards sasha |
Re: removing expired once rules in pf_purge_thread()
|
|
On 16/12/15(Wed) 13:30, Alexandr Nedvedicky wrote:
> On Wed, Dec 16, 2015 at 12:35:41PM +0100, Martin Pieuchot wrote: > > On 16/12/15(Wed) 10:19, Alexandr Nedvedicky wrote: > > > On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote: > > > > > > > > On Tue, 15 Dec 2015, Mike Belopuhov wrote: > > > > > > > > > > Yet another possibility is to drop 'once' rules as too complex to > > > > > > implement for multiprocessor but I have no idea if this is viable. > > > > > > > > > > It is. And I have said that before with an authority of the implementor > > > > > of "once" rules: since we don't have any userland applications that > > > > > would use this yet, we can ditch them for now and possibly devise a > > > > > better approach later. > > > > > > > > > Don't make your lives harder than they have to be! > > > > > > > > I tend to agree! And I can't see a way to reimplement it for a > > > > multithreaded pf without introducing downsides. > > > > Guys, if none of you can come with a valid reason to keep "once" rules > > please kill them. > > > > There's so much work to do to make pf(4) runnable on multiple CPUs in > > parallel that bikescheding/turd-polishing bits that are not used are > > IMHO not the way to go. > > The patch I've sent recently works just fine for Solaris. So ONCE rules are no > issue. I like the idea of once rules as they make life potentially easier for > application proxies. If "potentially easier" is good enough for you then please move forward :) |
Re: removing expired once rules in pf_purge_thread()
|
|
In reply to this post by Alexandr Nedvedicky
On Wed, Dec 16, 2015 at 02:31 +0100, Alexandr Nedvedicky wrote:
> Hello, > > > It just occurred to me that another possibility would be a match-only > > rule that matches one but doesn't involve any purging machinery. Right > > now we install ftp-proxy rules as having maximum number of states equal > > to 1, however there's a time window between the moment the state is no > > longer there, but anchor is not gone yet and it can potentially create > > more states which is not a problem for an eavesdropper who can sniff > > the plaintext protocol. > > I'm not sure I'm following you. IMO the expire flag should solve > that problem. As soon as rule is marked as expired it can not be > matched by packet any more. > Sure. I'm just saying that we could change the logic and remove the purging part, while keeping only the "match once". > I'm attaching yet another version of patch. The list of changes to > patch sent by Richard is as follows: > > - atomic operations are gone as we don't need them > right now. those will be introduced as soon as we will get > there. pf_test()/pf_test_rule() is still processing a single > packet at a time. > > - it's better to use TAILQ_EMPTY() instead of checking link > members to determine if last rule is being removed from > anchor `a` > > - I've also realized we have to call pf_purge_expired_rules() > from pf_commit_rules(). We have to make sure expired rules are gone > from active lists. I think this is the best approach for pf@Puffy > currently. It will be changed as soon as ruleset locks and > reference counting for rules will be merged in. I had to also > reshuffle a consistency lock a bit in pf_purge_thread(). > > - last but not least: silly name got renamed (s/myruleset/ruleset) > > so what do you think? OK? > I think it's almost OK, some comments inline. > regards > sasha > > ---------8<----------------8<----------------8<---------------8<-------- > Index: pf.c > =================================================================== > RCS file: /cvs/src/sys/net/pf.c,v > retrieving revision 1.960 > diff -u -p -r1.960 pf.c > --- pf.c 6 Dec 2015 10:03:23 -0000 1.960 > +++ pf.c 16 Dec 2015 00:52:22 -0000 > @@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key, > RB_GENERATE(pf_state_tree_id, pf_state, > entry_id, pf_state_compare_id); > > +SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl = > + SLIST_HEAD_INITIALIZER(pf_rule_gcl); > + > __inline int > pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af) > { > @@ -1137,6 +1140,18 @@ pf_state_export(struct pfsync_state *sp, > /* END state table stuff */ > > void > +pf_purge_expired_rules(void) > +{ > + struct pf_rule *r; > + > + while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) { > + SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle); > + KASSERT(r->rule_flag & PFRULE_EXPIRED); > + pf_purge_rule(r); > + } > +} > + > +void > pf_purge_thread(void *v) > { > int nloops = 0, s; > @@ -1154,6 +1169,11 @@ pf_purge_thread(void *v) > if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) { > pf_purge_expired_fragments(); > pf_purge_expired_src_nodes(0); > + if (!SLIST_EMPTY(&pf_rule_gcl)) { > + rw_enter_write(&pf_consistency_lock); > + pf_purge_expired_rules(); > + rw_exit_write(&pf_consistency_lock); > + } > nloops = 0; > } > Please follow the example of pf_purge_expired_src_nodes and fold all of this including the rwlocks inside pf_purge_expired_rules. > @@ -3135,6 +3155,10 @@ pf_test_rule(struct pf_pdesc *pd, struct > ruleset = &pf_main_ruleset; > r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr); > while (r != NULL) { > + if (r->rule_flag & PFRULE_EXPIRED) { > + r = TAILQ_NEXT(r, entries); > + goto nextrule; > + } > r->evaluations++; > PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot), > r->skip[PF_SKIP_IFP].ptr); > @@ -3433,8 +3457,15 @@ pf_test_rule(struct pf_pdesc *pd, struct > } > #endif /* NPFSYNC > 0 */ > > - if (r->rule_flag & PFRULE_ONCE) > - pf_purge_rule(ruleset, r, aruleset, a); > + if (r->rule_flag & PFRULE_ONCE) { > + if ((a != NULL) && TAILQ_EMPTY(a->ruleset->rules.active.ptr)) { > + a->rule_flag |= PFRULE_EXPIRED; > + SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle); > + } > + > + r->rule_flag |= PFRULE_EXPIRED; > + SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); > + } > I like the generalised approach to a/r purging. I have a few questions however: Since you're not removing the rule from the ruleset at this point what does "pfctl -sr" display in this case? What happens if you list the anchor? And what should the user see? Should we omit exporting those rules into userland via ioctl? What happens with pfsync in this case? > #ifdef INET6 > if (rewrite && skw->af != sks->af) > Index: pf_ioctl.c > =================================================================== > RCS file: /cvs/src/sys/net/pf_ioctl.c,v > retrieving revision 1.297 > diff -u -p -r1.297 pf_ioctl.c > --- pf_ioctl.c 3 Dec 2015 13:30:18 -0000 1.297 > +++ pf_ioctl.c 16 Dec 2015 00:52:25 -0000 > @@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu > } > > void > -pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule, > - struct pf_ruleset *aruleset, struct pf_rule *arule) > +pf_purge_rule(struct pf_rule *rule) > { > u_int32_t nr = 0; > + struct pf_ruleset *ruleset; > > - KASSERT(ruleset != NULL && rule != NULL); > + KASSERT((rule != NULL) && (rule->ruleset != NULL)); > + ruleset = rule->ruleset; > > pf_rm_rule(ruleset->rules.active.ptr, rule); > ruleset->rules.active.rcount--; > @@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset > rule->nr = nr++; > ruleset->rules.active.ticket++; > pf_calc_skip_steps(ruleset->rules.active.ptr); > - > - /* remove the parent anchor rule */ > - if (nr == 0 && arule && aruleset) { > - pf_rm_rule(aruleset->rules.active.ptr, arule); > - aruleset->rules.active.rcount--; > - TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries) > - rule->nr = nr++; > - aruleset->rules.active.ticket++; > - pf_calc_skip_steps(aruleset->rules.active.ptr); > - } > } > > u_int16_t > @@ -775,6 +766,9 @@ pf_commit_rules(u_int32_t ticket, char * > int s, error; > u_int32_t old_rcount; > > + /* make sure any expired rules get removed from active rules first */ > + pf_purge_expired_rules(); > + After folding rwlocks inside pf_purge_expired_rules make sure to pass a flag to it to indicate that the rwlock is already taken. > rs = pf_find_ruleset(anchor); > if (rs == NULL || !rs->rules.inactive.open || > ticket != rs->rules.inactive.ticket) > @@ -1209,6 +1203,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > } > TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr, > rule, entries); > + rule->ruleset = ruleset; > ruleset->rules.inactive.rcount++; > break; > } > Index: pfvar.h > =================================================================== > RCS file: /cvs/src/sys/net/pfvar.h,v > retrieving revision 1.426 > diff -u -p -r1.426 pfvar.h > --- pfvar.h 3 Dec 2015 14:05:28 -0000 1.426 > +++ pfvar.h 16 Dec 2015 00:52:28 -0000 > @@ -563,6 +563,9 @@ struct pf_rule { > struct pf_addr addr; > u_int16_t port; > } divert, divert_packet; > + > + SLIST_ENTRY(pf_rule) gcle; > + struct pf_ruleset *ruleset; > }; > > /* rule flags */ > @@ -581,6 +584,7 @@ struct pf_rule { > #define PFRULE_PFLOW 0x00040000 > #define PFRULE_ONCE 0x00100000 /* one shot rule */ > #define PFRULE_AFTO 0x00200000 /* af-to rule */ > +#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */ > > #define PFSTATE_HIWAT 10000 /* default state table size */ > #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */ > @@ -1669,6 +1673,7 @@ extern struct pool pf_state_scrub_pl; > extern void pf_purge_thread(void *); > extern void pf_purge_expired_src_nodes(int); > extern void pf_purge_expired_states(u_int32_t); > +extern void pf_purge_expired_rules(void); > extern void pf_remove_state(struct pf_state *); > extern void pf_remove_divert_state(struct pf_state_key *); > extern void pf_free_state(struct pf_state *); > @@ -1701,9 +1706,7 @@ extern void pf_addrcpy(struct pf_addr > sa_family_t); > void pf_rm_rule(struct pf_rulequeue *, > struct pf_rule *); > -void pf_purge_rule(struct pf_ruleset *, > - struct pf_rule *, struct pf_ruleset *, > - struct pf_rule *); > +void pf_purge_rule(struct pf_rule *); > struct pf_divert *pf_find_divert(struct mbuf *); > int pf_setup_pdesc(struct pf_pdesc *, void *, > sa_family_t, int, struct pfi_kif *, |
Re: removing expired once rules in pf_purge_thread()
|
|
hello,
> > > It just occurred to me that another possibility would be a match-only > > > rule that matches one but doesn't involve any purging machinery. Right > > > now we install ftp-proxy rules as having maximum number of states equal > > > to 1, however there's a time window between the moment the state is no > > > longer there, but anchor is not gone yet and it can potentially create > > > more states which is not a problem for an eavesdropper who can sniff > > > the plaintext protocol. > > > > I'm not sure I'm following you. IMO the expire flag should solve > > that problem. As soon as rule is marked as expired it can not be > > matched by packet any more. > > > > Sure. I'm just saying that we could change the logic and remove > the purging part, while keeping only the "match once". > garbage collection. > > Please follow the example of pf_purge_expired_src_nodes and fold > all of this including the rwlocks inside pf_purge_expired_rules. > fixed > > I like the generalised approach to a/r purging. I have a few > questions however: > > Since you're not removing the rule from the ruleset at this point > what does "pfctl -sr" display in this case? new version of the patch makes pfctl -sr aware of PFRULE_EXPIRED flag. The expired rule will be shown in verbose mode only. > What happens if you > list the anchor? And what should the user see? > > Should we omit exporting those rules into userland via ioctl? I think we should export those rules to userland and let userland to interpret what it gets from kernel. Changing DIOCGETRULE might not be that easy. > > What happens with pfsync in this case? I keep forgetting about pfsync... After looking at current pf_purge_rule() implementation in CVS it seems to me it just does not care about pfsync at all. The patch I'd like to commit keeps things same with respect to pfsync. from my very limited understanding of pfsync, only main rulesets are synced up between nodes. When pf_rules_commit() loads new rules to active list it calls pf_setup_pfsync_matching(). the function calculates a checksum on ruleset. It's still not clear to me how/when (if ever) are rules synced up. thanks and regards sasha --------8<---------------8<---------------8<------------------8<-------- Index: sys/net/pf.c =================================================================== RCS file: /cvs/src/sys/net/pf.c,v retrieving revision 1.960 diff -u -p -r1.960 pf.c --- sys/net/pf.c 6 Dec 2015 10:03:23 -0000 1.960 +++ sys/net/pf.c 16 Dec 2015 16:28:28 -0000 @@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key, RB_GENERATE(pf_state_tree_id, pf_state, entry_id, pf_state_compare_id); +SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl = + SLIST_HEAD_INITIALIZER(pf_rule_gcl); + __inline int pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af) { @@ -1137,6 +1140,27 @@ pf_state_export(struct pfsync_state *sp, /* END state table stuff */ void +pf_purge_expired_rules(int locked) +{ + struct pf_rule *r; + + if (SLIST_EMPTY(&pf_rule_gcl)) + return; + + if (!locked) + rw_enter_write(&pf_consistency_lock); + + while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) { + SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle); + KASSERT(r->rule_flag & PFRULE_EXPIRED); + pf_purge_rule(r); + } + + if (!locked) + rw_exit_write(&pf_consistency_lock); +} + +void pf_purge_thread(void *v) { int nloops = 0, s; @@ -1154,6 +1178,7 @@ pf_purge_thread(void *v) if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) { pf_purge_expired_fragments(); pf_purge_expired_src_nodes(0); + pf_purge_expired_rules(0); nloops = 0; } @@ -3135,6 +3160,10 @@ pf_test_rule(struct pf_pdesc *pd, struct ruleset = &pf_main_ruleset; r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr); while (r != NULL) { + if (r->rule_flag & PFRULE_EXPIRED) { + r = TAILQ_NEXT(r, entries); + goto nextrule; + } r->evaluations++; PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot), r->skip[PF_SKIP_IFP].ptr); @@ -3433,8 +3462,15 @@ pf_test_rule(struct pf_pdesc *pd, struct } #endif /* NPFSYNC > 0 */ - if (r->rule_flag & PFRULE_ONCE) - pf_purge_rule(ruleset, r, aruleset, a); + if (r->rule_flag & PFRULE_ONCE) { + if ((a != NULL) && TAILQ_EMPTY(a->ruleset->rules.active.ptr)) { + a->rule_flag |= PFRULE_EXPIRED; + SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle); + } + + r->rule_flag |= PFRULE_EXPIRED; + SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); + } #ifdef INET6 if (rewrite && skw->af != sks->af) Index: sys/net/pf_ioctl.c =================================================================== RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.297 diff -u -p -r1.297 pf_ioctl.c --- sys/net/pf_ioctl.c 3 Dec 2015 13:30:18 -0000 1.297 +++ sys/net/pf_ioctl.c 16 Dec 2015 16:28:28 -0000 @@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu } void -pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule, - struct pf_ruleset *aruleset, struct pf_rule *arule) +pf_purge_rule(struct pf_rule *rule) { u_int32_t nr = 0; + struct pf_ruleset *ruleset; - KASSERT(ruleset != NULL && rule != NULL); + KASSERT((rule != NULL) && (rule->ruleset != NULL)); + ruleset = rule->ruleset; pf_rm_rule(ruleset->rules.active.ptr, rule); ruleset->rules.active.rcount--; @@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset rule->nr = nr++; ruleset->rules.active.ticket++; pf_calc_skip_steps(ruleset->rules.active.ptr); - - /* remove the parent anchor rule */ - if (nr == 0 && arule && aruleset) { - pf_rm_rule(aruleset->rules.active.ptr, arule); - aruleset->rules.active.rcount--; - TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries) - rule->nr = nr++; - aruleset->rules.active.ticket++; - pf_calc_skip_steps(aruleset->rules.active.ptr); - } } u_int16_t @@ -775,6 +766,9 @@ pf_commit_rules(u_int32_t ticket, char * int s, error; u_int32_t old_rcount; + /* Make sure any expired rules get removed from active rules first. */ + pf_purge_expired_rules(1); + rs = pf_find_ruleset(anchor); if (rs == NULL || !rs->rules.inactive.open || ticket != rs->rules.inactive.ticket) @@ -1209,6 +1203,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a } TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr, rule, entries); + rule->ruleset = ruleset; ruleset->rules.inactive.rcount++; break; } Index: sys/net/pfvar.h =================================================================== RCS file: /cvs/src/sys/net/pfvar.h,v retrieving revision 1.426 diff -u -p -r1.426 pfvar.h --- sys/net/pfvar.h 3 Dec 2015 14:05:28 -0000 1.426 +++ sys/net/pfvar.h 16 Dec 2015 16:28:28 -0000 @@ -563,6 +563,9 @@ struct pf_rule { struct pf_addr addr; u_int16_t port; } divert, divert_packet; + + SLIST_ENTRY(pf_rule) gcle; + struct pf_ruleset *ruleset; }; /* rule flags */ @@ -581,6 +584,7 @@ struct pf_rule { #define PFRULE_PFLOW 0x00040000 #define PFRULE_ONCE 0x00100000 /* one shot rule */ #define PFRULE_AFTO 0x00200000 /* af-to rule */ +#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */ #define PFSTATE_HIWAT 10000 /* default state table size */ #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */ @@ -1669,6 +1673,7 @@ extern struct pool pf_state_scrub_pl; extern void pf_purge_thread(void *); extern void pf_purge_expired_src_nodes(int); extern void pf_purge_expired_states(u_int32_t); +extern void pf_purge_expired_rules(int); extern void pf_remove_state(struct pf_state *); extern void pf_remove_divert_state(struct pf_state_key *); extern void pf_free_state(struct pf_state *); @@ -1701,9 +1706,7 @@ extern void pf_addrcpy(struct pf_addr sa_family_t); void pf_rm_rule(struct pf_rulequeue *, struct pf_rule *); -void pf_purge_rule(struct pf_ruleset *, - struct pf_rule *, struct pf_ruleset *, - struct pf_rule *); +void pf_purge_rule(struct pf_rule *); struct pf_divert *pf_find_divert(struct mbuf *); int pf_setup_pdesc(struct pf_pdesc *, void *, sa_family_t, int, struct pfi_kif *, Index: sbin/pfctl/pfctl_parser.c =================================================================== RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v retrieving revision 1.306 diff -u -p -r1.306 pfctl_parser.c --- sbin/pfctl/pfctl_parser.c 3 Sep 2015 12:46:47 -0000 1.306 +++ sbin/pfctl/pfctl_parser.c 16 Dec 2015 16:28:28 -0000 @@ -700,8 +700,12 @@ print_rule(struct pf_rule *r, const char int verbose = opts & (PF_OPT_VERBOSE2 | PF_OPT_DEBUG); char *p; + if ((r->rule_flag & PFRULE_EXPIRED) && (!verbose)) + return; + if (verbose) printf("@%d ", r->nr); + if (r->action > PF_MATCH) printf("action(%d)", r->action); else if (anchor_call[0]) { @@ -1119,6 +1123,9 @@ print_rule(struct pf_rule *r, const char printf(" "); print_pool(&r->route, 0, 0, r->af, PF_POOL_ROUTE, verbose); } + + if (r->rule_flag & PFRULE_EXPIRED) + printf("[ rule expired ]"); } void |
Re: removing expired once rules in pf_purge_thread()
|
|
In reply to this post by Alexandr Nedvedicky
On Wed, 16 Dec 2015, Alexandr Nedvedicky wrote: > On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote: > > Quoting pf.conf(5) "once - Creates a one shot rule that will remove itself > > from an active ruleset after the first match." > > > > A 'first' match presupposes a total ordering. This comes for free when pf > > is single-threaded but concurrent rule matching must take the trouble to > > re-impose it somewhere. (I assume that pf aims to do concurrent matching > > of packets against the ruleset.) > > pf@solaris allows the race here. > The price for correct behavior, which is to allow one and only one > packet to hit the rule is too high (given the once rules are kind of > special case). I agree, at least, I do for 'once' rules that aren't 'quick'. > What has to be granted is there is one 'winner' only, which puts the > rule to garbage collector list. The pragmatic approach wins here. Right. I'll just note though that the patch as it stands allows multiple winners: consider the window between testing PFRULE_EXPIRED and setting it (quoted below). Multiple threads may execute in that window before one of them closes it by setting PFRULE_EXPIRED. Each of them will delete the same rule in turn, causing a probable crash. At least, SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); crashes my machine. Whether that's a realistic issue, I don't know. I have though been bitten by enough edge cases like this to be very wary of them. best, Richard. On Wed, 16 Dec 2015, Alexandr Nedvedicky wrote: @@ -3135,6 +3160,10 @@ pf_test_rule(struct pf_pdesc *pd, struct ruleset = &pf_main_ruleset; r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr); while (r != NULL) { + if (r->rule_flag & PFRULE_EXPIRED) { + r = TAILQ_NEXT(r, entries); + goto nextrule; + } r->evaluations++; PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot), r->skip[PF_SKIP_IFP].ptr); > @@ -3433,8 +3462,15 @@ pf_test_rule(struct pf_pdesc *pd, struct > } > #endif /* NPFSYNC > 0 */ > > - if (r->rule_flag & PFRULE_ONCE) > - pf_purge_rule(ruleset, r, aruleset, a); > + if (r->rule_flag & PFRULE_ONCE) { > + if ((a != NULL) && TAILQ_EMPTY(a->ruleset->rules.active.ptr)) { > + a->rule_flag |= PFRULE_EXPIRED; > + SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle); > + } > + > + r->rule_flag |= PFRULE_EXPIRED; > + SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); > + } > > #ifdef INET6 > if (rewrite && skw->af != sks->af) |
Re: removing expired once rules in pf_purge_thread()
|
|
Hello Richard,
> > What has to be granted is there is one 'winner' only, which puts the > > rule to garbage collector list. The pragmatic approach wins here. > > Right. I'll just note though that the patch as it stands allows multiple > winners: consider the window between testing PFRULE_EXPIRED and setting it > (quoted below). Multiple threads may execute in that window before one of > them closes it by setting PFRULE_EXPIRED. Each of them will delete the > same rule in turn, causing a probable crash. At least, > > SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); > SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); > > crashes my machine. > > Whether that's a realistic issue, I don't know. I have though been bitten > by enough edge cases like this to be very wary of them. I think it's not realistic with current PF at OpenBSD. The pf_test() function does not run concurrently, so there can be no such race. FYI: PF@solaris uses mutex there to protect the insertion to gcl. Code goes as follows at Solaris: if (!(r->rule_flags & PFRULE_EXPIRED)) { mutex_enter(&gcl_mutex); /* retest under exclusive protection */ if (!(r->rule_flags & PFRULE_EXPIRED)) { r->rule_flag |= PFRULE_EXPIRED; SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); } } regards sasha |
Re: removing expired once rules in pf_purge_thread()
|
|
Hi Sasha, On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote: > > Right. I'll just note though that the patch as it stands allows > > multiple winners [...] Whether that's a realistic issue, I don't know. > > I have though been bitten by enough edge cases like this to be very > > wary of them. > > I think it's not realistic with current PF at OpenBSD. The pf_test() function > does not run concurrently, so there can be no such race. Fair enough :) I presumed that an upcoming concurrent pf_test() would rely on this patch. > FYI: PF@solaris uses mutex there to protect the insertion to gcl. Code goes as > follows at Solaris: > > if (!(r->rule_flags & PFRULE_EXPIRED)) { > mutex_enter(&gcl_mutex); > /* retest under exclusive protection */ > if (!(r->rule_flags & PFRULE_EXPIRED)) { > r->rule_flag |= PFRULE_EXPIRED; > SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); > } > } I was wondering about that. For this patch presumably it's thought unnecessary to mutex the SLIST ops (or for that matter to preserve rule lifetime) for one-shot rules as the purge thread runs so infrequently. BTW, the purge queue exists to pass a result between threads; an alternative is to recompute it in the purge thread by searching for expired rules: no need for the queue, its mutexes, etc. Easier to preserve rule lifetime, too. May need another anchor stack though. I might have a go next year when the code is more settled. Ok, enough from me on this. best, Richard. |
Re: removing expired once rules in pf_purge_thread()
|
|
> On 17 Dec 2015, at 13:30, Richard Procter <[hidden email]> wrote: > > > Hi Sasha, > > On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote: > >>> Right. I'll just note though that the patch as it stands allows >>> multiple winners [...] Whether that's a realistic issue, I don't know. >>> I have though been bitten by enough edge cases like this to be very >>> wary of them. >> >> I think it's not realistic with current PF at OpenBSD. The pf_test() function >> does not run concurrently, so there can be no such race. > > Fair enough :) I presumed that an upcoming concurrent pf_test() would rely > on this patch. > >> FYI: PF@solaris uses mutex there to protect the insertion to gcl. Code goes as >> follows at Solaris: >> >> if (!(r->rule_flags & PFRULE_EXPIRED)) { >> mutex_enter(&gcl_mutex); >> /* retest under exclusive protection */ >> if (!(r->rule_flags & PFRULE_EXPIRED)) { >> r->rule_flag |= PFRULE_EXPIRED; >> SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); >> } >> } > > I was wondering about that. For this patch presumably it's thought > unnecessary to mutex the SLIST ops (or for that matter to preserve rule > lifetime) for one-shot rules as the purge thread runs so infrequently. right now pf doesnt run concurrently so extra locking is unnecessary. when it does run concurrently then it will need a mutex, despite how infrequently the gc runs. > > BTW, the purge queue exists to pass a result between threads; an > alternative is to recompute it in the purge thread by searching for > expired rules: no need for the queue, its mutexes, etc. Easier to preserve > rule lifetime, too. May need another anchor stack though. I might have a > go next year when the code is more settled. i believe the purge thread exists because once upon a time pool_free from interrupt context wasnt the best. i might be misremembering that though. currently that thread only looks at states. traversing the ruleset is possible i guess. if the semantic of the once rules is that they only match once, and if pf will run on multiple cpus, then coordinating between the cpus will require atomicity or serialisation of some sort. dlg > > Ok, enough from me on this. > > best, > Richard. > |
Re: removing expired once rules in pf_purge_thread()
|
|
Hello,
On Tue, Aug 30, 2016 at 10:53:56AM +1000, David Gwynne wrote: > > > On 17 Dec 2015, at 13:30, Richard Procter <[hidden email]> wrote: > > > > > > Hi Sasha, > > > > On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote: > > > >>> Right. I'll just note though that the patch as it stands allows > >>> multiple winners [...] Whether that's a realistic issue, I don't know. > >>> I have though been bitten by enough edge cases like this to be very > >>> wary of them. > >> > >> I think it's not realistic with current PF at OpenBSD. The pf_test() function > >> does not run concurrently, so there can be no such race. > > > > Fair enough :) I presumed that an upcoming concurrent pf_test() would rely > > on this patch. > > > >> FYI: PF@solaris uses mutex there to protect the insertion to gcl. Code goes as > >> follows at Solaris: > >> > >> if (!(r->rule_flags & PFRULE_EXPIRED)) { > >> mutex_enter(&gcl_mutex); > >> /* retest under exclusive protection */ > >> if (!(r->rule_flags & PFRULE_EXPIRED)) { > >> r->rule_flag |= PFRULE_EXPIRED; > >> SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle); > >> } > >> } > > > > I was wondering about that. For this patch presumably it's thought > > unnecessary to mutex the SLIST ops (or for that matter to preserve rule > > lifetime) for one-shot rules as the purge thread runs so infrequently. > > right now pf doesnt run concurrently so extra locking is unnecessary. when it > does run concurrently then it will need a mutex, despite how infrequently the > gc runs. I see. I'll remove the mutex and resend the patch. > > > > > BTW, the purge queue exists to pass a result between threads; an > > alternative is to recompute it in the purge thread by searching for > > expired rules: no need for the queue, its mutexes, etc. Easier to preserve > > rule lifetime, too. May need another anchor stack though. I might have a > > go next year when the code is more settled. > > i believe the purge thread exists because once upon a time pool_free from > interrupt context wasnt the best. i might be misremembering that though. PF at Solaris still keeps purge thread around, although it's entirely possible to let packet free memory. PF on Solaris does not have to deal with such constrains. > > currently that thread only looks at states. traversing the ruleset is > possible i guess. yes, ruleset traversal is possible, however gcl (garbage collector list) is meant as a shortcut. If packet hits ONCE rule it sets the flag to indicate rule has matched and inserts the rule to gcl. the purge thread then does not need to walk traverse all rulesets to collect 'dead' once rules. Instead it just walks the gcl to find out, which ONCE rules are dead to pull them out. There is also yet another option: remove gcl completely and just mark the ONCE rule got hit. we can leave dead ONCE rule to remain in ruleset until new rules will be loaded or system will be rebooted. > if the semantic of the once rules is that they only match > once, and if pf will run on multiple cpus, then coordinating between the cpus > will require atomicity or serialisation of some sort. PF on Solaris is willing to pay price of not having ONCE rules 100% atomic, there is currently no atomic op, which would set the flag to indicate rule matched packet. regards sasha |
«
Return to openbsd dev - tech
|
1 view|%1 views
| Free forum by Nabble | Edit this page |