Re: NET_LOCK and pf_consistency_lock removal

2017-01-29 Thread Alexander Bluhm
On Wed, Jan 25, 2017 at 09:53:34AM +0100, Sebastian Benoit wrote:
> this removes the pf_consistency_lock and protects the users with NET_LOCK()
> 
> I ran into a crash when using pfctl -k, and mpi suggested this change (and
> likes the diff).
> 
> pf hackers: ok?

OK bluhm@

> +pf_purge_expired_src_nodes(void)
...
>   if (cur->states == 0 && cur->expire <= time_uptime) {
> - if (! locked) {
> - rw_enter_write(_consistency_lock);
> - next = RB_NEXT(pf_src_tree,
> - _src_tracking, cur);
> - locked = 1;
> - }
> + next = RB_NEXT(pf_src_tree,
> + _src_tracking, cur);

This statement fits in a single line now.



Re: NET_LOCK and pf_consistency_lock removal

2017-01-25 Thread Martin Pieuchot
On 25/01/17(Wed) 09:53, Sebastian Benoit wrote:
> Hi,
> 
> this removes the pf_consistency_lock and protects the users with NET_LOCK()
> 
> I ran into a crash when using pfctl -k, and mpi suggested this change (and
> likes the diff).

The reason I suggested this diff is that pfioctl() will need the
NET_LOCK() anyway.  So better keep things simple until we're going to
redesign PF for a MP world.

Note that the crash benno@ talked about is just for pflow(4) users.

ok mpi@

> diff --git sys/net/pf.c sys/net/pf.c
> index 2ae434e405d..a8ade533484 100644
> --- sys/net/pf.c
> +++ sys/net/pf.c
> @@ -1154,26 +1154,20 @@ pf_state_export(struct pfsync_state *sp, struct 
> pf_state *st)
>  /* END state table stuff */
>  
>  void
> -pf_purge_expired_rules(int locked)
> +pf_purge_expired_rules(void)
>  {
>   struct pf_rule  *r;
>  
> + NET_ASSERT_LOCKED();
> +
>   if (SLIST_EMPTY(_rule_gcl))
>   return;
>  
> - if (!locked)
> - rw_enter_write(_consistency_lock);
> - else
> - rw_assert_wrlock(_consistency_lock);
> -
>   while ((r = SLIST_FIRST(_rule_gcl)) != NULL) {
>   SLIST_REMOVE(_rule_gcl, r, pf_rule, gcle);
>   KASSERT(r->rule_flag & PFRULE_EXPIRED);
>   pf_purge_rule(r);
>   }
> -
> - if (!locked)
> - rw_exit_write(_consistency_lock);
>  }
>  
>  void
> @@ -1194,7 +1188,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);
> + pf_purge_expired_rules();
>   nloops = 0;
>   }
>  
> @@ -1241,27 +1235,21 @@ pf_state_expires(const struct pf_state *state)
>  }
>  
>  void
> -pf_purge_expired_src_nodes(int waslocked)
> +pf_purge_expired_src_nodes(void)
>  {
>   struct pf_src_node  *cur, *next;
> - int  locked = waslocked;
> +
> + NET_ASSERT_LOCKED();
>  
>   for (cur = RB_MIN(pf_src_tree, _src_tracking); cur; cur = next) {
>   next = RB_NEXT(pf_src_tree, _src_tracking, cur);
>  
>   if (cur->states == 0 && cur->expire <= time_uptime) {
> - if (! locked) {
> - rw_enter_write(_consistency_lock);
> - next = RB_NEXT(pf_src_tree,
> - _src_tracking, cur);
> - locked = 1;
> - }
> + next = RB_NEXT(pf_src_tree,
> + _src_tracking, cur);
>   pf_remove_src_node(cur);
>   }
>   }
> -
> - if (locked && !waslocked)
> - rw_exit_write(_consistency_lock);
>  }
>  
>  void
> @@ -1334,13 +1322,12 @@ pf_remove_divert_state(struct pf_state_key *sk)
>   }
>  }
>  
> -/* callers should hold the write_lock on pf_consistency_lock */
>  void
>  pf_free_state(struct pf_state *cur)
>  {
>   struct pf_rule_item *ri;
>  
> - splsoftassert(IPL_SOFTNET);
> + NET_ASSERT_LOCKED();
>  
>  #if NPFSYNC > 0
>   if (pfsync_state_in_use(cur))
> @@ -1375,7 +1362,8 @@ pf_purge_expired_states(u_int32_t maxcheck)
>  {
>   static struct pf_state  *cur = NULL;
>   struct pf_state *next;
> - int  locked = 0;
> +
> + NET_ASSERT_LOCKED();
>  
>   while (maxcheck--) {
>   /* wrap to start of list when we hit the end */
> @@ -1390,25 +1378,14 @@ pf_purge_expired_states(u_int32_t maxcheck)
>  
>   if (cur->timeout == PFTM_UNLINKED) {
>   /* free removed state */
> - if (! locked) {
> - rw_enter_write(_consistency_lock);
> - locked = 1;
> - }
>   pf_free_state(cur);
>   } else if (pf_state_expires(cur) <= time_uptime) {
>   /* remove and free expired state */
>   pf_remove_state(cur);
> - if (! locked) {
> - rw_enter_write(_consistency_lock);
> - locked = 1;
> - }
>   pf_free_state(cur);
>   }
>   cur = next;
>   }
> -
> - if (locked)
> - rw_exit_write(_consistency_lock);
>  }
>  
>  int
> diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
> index 9b278c907f5..25c8bd1 100644
> --- sys/net/pf_ioctl.c
> +++ sys/net/pf_ioctl.c
> @@ -111,7 +111,6 @@ void   pf_qid2qname(u_int16_t, char 
> *);
>  void  pf_qid_unref(u_int16_t);
>  
>  struct pf_rulepf_default_rule, pf_default_rule_new;
> -struct rwlock pf_consistency_lock = 
> RWLOCK_INITIALIZER("pfcnslk");
>  
>  struct {
>   char   

NET_LOCK and pf_consistency_lock removal

2017-01-25 Thread Sebastian Benoit
Hi,

this removes the pf_consistency_lock and protects the users with NET_LOCK()

I ran into a crash when using pfctl -k, and mpi suggested this change (and
likes the diff).

pf hackers: ok?

diff --git sys/net/pf.c sys/net/pf.c
index 2ae434e405d..a8ade533484 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -1154,26 +1154,20 @@ pf_state_export(struct pfsync_state *sp, struct 
pf_state *st)
 /* END state table stuff */
 
 void
-pf_purge_expired_rules(int locked)
+pf_purge_expired_rules(void)
 {
struct pf_rule  *r;
 
+   NET_ASSERT_LOCKED();
+
if (SLIST_EMPTY(_rule_gcl))
return;
 
-   if (!locked)
-   rw_enter_write(_consistency_lock);
-   else
-   rw_assert_wrlock(_consistency_lock);
-
while ((r = SLIST_FIRST(_rule_gcl)) != NULL) {
SLIST_REMOVE(_rule_gcl, r, pf_rule, gcle);
KASSERT(r->rule_flag & PFRULE_EXPIRED);
pf_purge_rule(r);
}
-
-   if (!locked)
-   rw_exit_write(_consistency_lock);
 }
 
 void
@@ -1194,7 +1188,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);
+   pf_purge_expired_rules();
nloops = 0;
}
 
@@ -1241,27 +1235,21 @@ pf_state_expires(const struct pf_state *state)
 }
 
 void
-pf_purge_expired_src_nodes(int waslocked)
+pf_purge_expired_src_nodes(void)
 {
struct pf_src_node  *cur, *next;
-   int  locked = waslocked;
+
+   NET_ASSERT_LOCKED();
 
for (cur = RB_MIN(pf_src_tree, _src_tracking); cur; cur = next) {
next = RB_NEXT(pf_src_tree, _src_tracking, cur);
 
if (cur->states == 0 && cur->expire <= time_uptime) {
-   if (! locked) {
-   rw_enter_write(_consistency_lock);
-   next = RB_NEXT(pf_src_tree,
-   _src_tracking, cur);
-   locked = 1;
-   }
+   next = RB_NEXT(pf_src_tree,
+   _src_tracking, cur);
pf_remove_src_node(cur);
}
}
-
-   if (locked && !waslocked)
-   rw_exit_write(_consistency_lock);
 }
 
 void
@@ -1334,13 +1322,12 @@ pf_remove_divert_state(struct pf_state_key *sk)
}
 }
 
-/* callers should hold the write_lock on pf_consistency_lock */
 void
 pf_free_state(struct pf_state *cur)
 {
struct pf_rule_item *ri;
 
-   splsoftassert(IPL_SOFTNET);
+   NET_ASSERT_LOCKED();
 
 #if NPFSYNC > 0
if (pfsync_state_in_use(cur))
@@ -1375,7 +1362,8 @@ pf_purge_expired_states(u_int32_t maxcheck)
 {
static struct pf_state  *cur = NULL;
struct pf_state *next;
-   int  locked = 0;
+
+   NET_ASSERT_LOCKED();
 
while (maxcheck--) {
/* wrap to start of list when we hit the end */
@@ -1390,25 +1378,14 @@ pf_purge_expired_states(u_int32_t maxcheck)
 
if (cur->timeout == PFTM_UNLINKED) {
/* free removed state */
-   if (! locked) {
-   rw_enter_write(_consistency_lock);
-   locked = 1;
-   }
pf_free_state(cur);
} else if (pf_state_expires(cur) <= time_uptime) {
/* remove and free expired state */
pf_remove_state(cur);
-   if (! locked) {
-   rw_enter_write(_consistency_lock);
-   locked = 1;
-   }
pf_free_state(cur);
}
cur = next;
}
-
-   if (locked)
-   rw_exit_write(_consistency_lock);
 }
 
 int
diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
index 9b278c907f5..25c8bd1 100644
--- sys/net/pf_ioctl.c
+++ sys/net/pf_ioctl.c
@@ -111,7 +111,6 @@ void pf_qid2qname(u_int16_t, char 
*);
 voidpf_qid_unref(u_int16_t);
 
 struct pf_rule  pf_default_rule, pf_default_rule_new;
-struct rwlock   pf_consistency_lock = RWLOCK_INITIALIZER("pfcnslk");
 
 struct {
charstatusif[IFNAMSIZ];
@@ -987,12 +986,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, 
struct proc *p)
return (EACCES);
}
 
-   if (flags & FWRITE)
-   rw_enter_write(_consistency_lock);
-   else
-   rw_enter_read(_consistency_lock);
-
-   s = splsoftnet();
+   NET_LOCK(s);
switch (cmd) {
 
case DIOCSTART:
@@ -2388,11