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(&pf_rule_gcl))
> return;
>
> - if (!locked)
> - rw_enter_write(&pf_consistency_lock);
> - else
> - rw_assert_wrlock(&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
> @@ -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, &tree_src_tracking); cur; cur = next) {
> next = RB_NEXT(pf_src_tree, &tree_src_tracking, cur);
>
> if (cur->states == 0 && cur->expire <= time_uptime) {
> - if (! locked) {
> - rw_enter_write(&pf_consistency_lock);
> - next = RB_NEXT(pf_src_tree,
> - &tree_src_tracking, cur);
> - locked = 1;
> - }
> + next = RB_NEXT(pf_src_tree,
> + &tree_src_tracking, cur);
> pf_remove_src_node(cur);
> }
> }
> -
> - if (locked && !waslocked)
> - rw_exit_write(&pf_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(&pf_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(&pf_consistency_lock);
> - locked = 1;
> - }
> pf_free_state(cur);
> }
> cur = next;
> }
> -
> - if (locked)
> - rw_exit_write(&pf_consistency_lock);
> }
>
> int
> diff --git sys/net/pf_ioctl.c sys/net/pf_ioctl.c
> index 9b278c907f5..444425c8bd1 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_rule pf_default_rule, pf_default_rule_new;
> -struct rwlock pf_consistency_lock =
> RWLOCK_INITIALIZER("pfcnslk");
>
> struct {
> char statusif[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(&pf_consistency_lock);
> - else
> - rw_enter_read(&pf_consistency_lock);
> -
> - s = splsoftnet();
> + NET_LOCK(s);
> switch (cmd) {
>
> case DIOCSTART:
> @@ -2388,11 +2382,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> break;
> }
> fail:
> - splx(s);
> - if (flags & FWRITE)
> - rw_exit_write(&pf_consistency_lock);
> - else
> - rw_exit_read(&pf_consistency_lock);
> + NET_UNLOCK(s);
> return (error);
> }
>
> diff --git sys/net/pf_norm.c sys/net/pf_norm.c
> index 6bf9681e37f..06c8643b5e4 100644
> --- sys/net/pf_norm.c
> +++ sys/net/pf_norm.c
> @@ -176,6 +176,8 @@ pf_purge_expired_fragments(void)
> struct pf_fragment *frag;
> int32_t expire;
>
> + NET_ASSERT_LOCKED();
> +
> expire = time_uptime - pf_default_rule.timeout[PFTM_FRAG];
> while ((frag = TAILQ_LAST(&pf_fragqueue, pf_fragqueue)) != NULL) {
> if (frag->fr_timeout > expire)
> diff --git sys/net/pfvar.h sys/net/pfvar.h
> index 9896bf82eca..85ce5074657 100644
> --- sys/net/pfvar.h
> +++ sys/net/pfvar.h
> @@ -1614,9 +1614,9 @@ extern void
> pf_tbladdr_remove(struct pf_addr_wrap *);
> extern void pf_tbladdr_copyout(struct pf_addr_wrap *);
> extern void pf_calc_skip_steps(struct pf_rulequeue *);
> extern void pf_purge_thread(void *);
> -extern void pf_purge_expired_src_nodes(int);
> +extern void pf_purge_expired_src_nodes();
> extern void pf_purge_expired_states(u_int32_t);
> -extern void pf_purge_expired_rules(int);
> +extern void pf_purge_expired_rules();
> 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 *);
> @@ -1790,7 +1790,6 @@ int pf_addr_compare(struct pf_addr *,
> struct pf_addr *,
>
> extern struct pf_status pf_status;
> extern struct pool pf_frent_pl, pf_frag_pl;
> -extern struct rwlock pf_consistency_lock;
>
> struct pf_pool_limit {
> void *pp;
>
