Re: PF Once rules are not removed from main anchor

2014-07-02 Thread Mike Belopuhov 
On 21 June 2014 15:36, Alexandr Nedvedicky alexandr.nedvedi...@oracle.com 
wrote:
 Hello,

 I'm not sure it is the right place to submit patches. Let me know if there is
 better/more appropriate address for this.

 during our testing we've found the once rules are not removed,
 when used in main anchor.


Correct.  I've addedd the ruleset pointer check to prevent that
shortly before the release since it caused panics...

 during debugging we found the rules in main anchor have member anchor set to
 NULL (pf_rule::anchor). This makes pf_purge_rule() function to bail out
 to early without removing the rule from ruleset.

 patch below fixed problem for us.


However, this solution is not correct for us.  Perhaps you have some
other changes in your tree to make it work.

 Index: pf_ioctl.c
 ===
 RCS file: /cvs/src/sys/net/pf_ioctl.c,v
 retrieving revision 1.272
 diff -u -r1.272 pf_ioctl.c
 --- pf_ioctl.c  22 Apr 2014 14:41:03 -  1.272
 +++ pf_ioctl.c  20 Jun 2014 14:26:22 -
 @@ -312,7 +312,7 @@
  {
 u_int32_tnr;

 -   if (ruleset == NULL || ruleset-anchor == NULL)
 +   if (ruleset == NULL)
 return;


ruleset-anchor is useless since nothing really checks for it if
ruleset is NULL.

 pf_rm_rule(ruleset-rules.active.ptr, rule);
 @@ -325,7 +325,10 @@
 ruleset-rules.active.ticket++;

 pf_calc_skip_steps(ruleset-rules.active.ptr);
 -   pf_remove_if_empty_ruleset(ruleset);
 +
 +   if (ruleset != pf_main_ruleset) {
 +   pf_remove_if_empty_ruleset(ruleset);
 +   }
  }


You don't need to check ruleset against pf_main_ruleset since
the first thing pf_remove_if_empty_ruleset does is bail if
ruleset == pf_main_ruleset.  This bit confused me quite a bit.

What really is a problem for us is that when pf_purge_rule is
called on a main ruleset from pf_test_rule the first argument
(ruleset) is NULL and not pf_main_ruleset, which would be
sensible.  The only other user of it, AFAICT, is pflog_packet
but it checks for ruleset-anchor before it does it's thing.
I don't see any reason why we can't start our rule set iteration
with 'ruleset' set to pf_main_ruleset (regress tests agree).

OK?

diff --git sys/net/pf.c sys/net/pf.c
index 71f85d1..562901d 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -3163,10 +3163,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, 
struct pf_state **sm,
}
break;
 #endif /* INET6 */
}
 
+   ruleset = pf_main_ruleset;
r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr);
while (r != NULL) {
r-evaluations++;
PF_TEST_ATTRIB((pfi_kif_match(r-kif, pd-kif) == r-ifnot),
r-skip[PF_SKIP_IFP].ptr);
diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
index 5ad762c..86e987f 100644
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -308,24 +308,19 @@ pf_rm_rule(struct pf_rulequeue *rulequeue, struct pf_rule 
*rule)
 }
 
 void
 pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule)
 {
-   u_int32_tnr;
+   u_int32_tnr = 0;
 
-   if (ruleset == NULL || ruleset-anchor == NULL)
-   return;
+   KASSERT(ruleset != NULL  rule != NULL);
 
pf_rm_rule(ruleset-rules.active.ptr, rule);
ruleset-rules.active.rcount--;
-
-   nr = 0;
TAILQ_FOREACH(rule, ruleset-rules.active.ptr, entries)
rule-nr = nr++;
-
ruleset-rules.active.ticket++;
-
pf_calc_skip_steps(ruleset-rules.active.ptr);
pf_remove_if_empty_ruleset(ruleset);
 }
 
 u_int16_t

Re: PF Once rules are not removed from main anchor

2014-07-02 Thread Alexandr Nedvedicky 
Hello,

thanks for clarifying things.

 However, this solution is not correct for us.  Perhaps you have some
 other changes in your tree to make it work.
 

yes, that's correct. We had to make PF SMP friendly. We don't want packet to
remove the ONCE rule from its ruleset. Instead the pf_test_rule(), marks rule
as deleted and schedules it for garbage collection, so the pf_purge_rule() is
never executed by pf_test_rule(). 

looks like your patch still fits well with our implementation, I'll give it a
try.


thanks and regards
sasha

[alexandr.nedvedi...@oracle.com: PF Once rules are not removed from main anchor]

2014-06-21 Thread Alexandr Nedvedicky 
Hello,

resending to better alias as recommended by p...@benzedrine.cx subscribers.

regards
sasha

- Forwarded message from Alexandr Nedvedicky 
alexandr.nedvedi...@oracle.com -

From: Alexandr Nedvedicky alexandr.nedvedi...@oracle.com
To: p...@benzedrine.cx
Subject: PF Once rules are not removed from main anchor

Hello,

I'm not sure it is the right place to submit patches. Let me know if there is
better/more appropriate address for this.

during our testing we've found the once rules are not removed,
when used in main anchor.

during debugging we found the rules in main anchor have member anchor set to
NULL (pf_rule::anchor). This makes pf_purge_rule() function to bail out
to early without removing the rule from ruleset.

patch below fixed problem for us.

regards
sasha


 cut here to get patch ---
Index: pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.272
diff -u -r1.272 pf_ioctl.c
--- pf_ioctl.c  22 Apr 2014 14:41:03 -  1.272
+++ pf_ioctl.c  20 Jun 2014 14:26:22 -
@@ -312,7 +312,7 @@
 {
u_int32_tnr;
 
-   if (ruleset == NULL || ruleset-anchor == NULL)
+   if (ruleset == NULL)
return;
 
pf_rm_rule(ruleset-rules.active.ptr, rule);
@@ -325,7 +325,10 @@
ruleset-rules.active.ticket++;
 
pf_calc_skip_steps(ruleset-rules.active.ptr);
-   pf_remove_if_empty_ruleset(ruleset);
+
+   if (ruleset != pf_main_ruleset) {
+   pf_remove_if_empty_ruleset(ruleset);
+   }
 }
 
 u_int16_t




Index: pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.272
diff -u -r1.272 pf_ioctl.c
--- pf_ioctl.c  22 Apr 2014 14:41:03 -  1.272
+++ pf_ioctl.c  20 Jun 2014 14:26:22 -
@@ -312,7 +312,7 @@
 {
u_int32_tnr;

-   if (ruleset == NULL || ruleset-anchor == NULL)
+   if (ruleset == NULL)
return;

pf_rm_rule(ruleset-rules.active.ptr, rule);
@@ -325,7 +325,10 @@
ruleset-rules.active.ticket++;

pf_calc_skip_steps(ruleset-rules.active.ptr);
-   pf_remove_if_empty_ruleset(ruleset);
+
+   if (ruleset != pf_main_ruleset) {
+   pf_remove_if_empty_ruleset(ruleset);
+   }
 }

 u_int16_t



- End forwarded message -
Index: pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.272
diff -u -r1.272 pf_ioctl.c
--- pf_ioctl.c  22 Apr 2014 14:41:03 -  1.272
+++ pf_ioctl.c  20 Jun 2014 14:26:22 -
@@ -312,7 +312,7 @@
 {
u_int32_tnr;

-   if (ruleset == NULL || ruleset-anchor == NULL)
+   if (ruleset == NULL)
return;

pf_rm_rule(ruleset-rules.active.ptr, rule);
@@ -325,7 +325,10 @@
ruleset-rules.active.ticket++;

pf_calc_skip_steps(ruleset-rules.active.ptr);
-   pf_remove_if_empty_ruleset(ruleset);
+
+   if (ruleset != pf_main_ruleset) {
+   pf_remove_if_empty_ruleset(ruleset);
+   }
 }

 u_int16_t