On Wed, May 31, 2017 at 10:50 +0200, Alexandr Nedvedicky wrote:
> Hello Mike,
>
> I'd like to ask you to take a one more look to change I'm going
> to commit.
Looks much better w/o SPLs. I did't find any issues with
the diff and I'm OK with you going forward with this.
> I did one more check of changes with respect to WITH_PF_LOCK
> and found one more bit to fix. We need to keep
> pf_purge_expired_fragments() under protection of NET_LOCK()
>
Fair enough.
> diff -r 6abbb123112a .hgtags
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/.hgtags Wed May 31 10:42:50 2017 +0200
> @@ -0,0 +1,1 @@
> +d545881e2652dbc0c057691a39a095bce92f441f pf-lock.baseline
Please be careful and don't include VCS's goo in diffs.
It'd be nice if you could manage to instruct mercurial
to remove a/ and b/ parts.
> @@ -1208,15 +1208,22 @@ pf_purge_thread(void *v)
>
> NET_LOCK(s);
>
> + PF_LOCK();
> /* process a fraction of the state table every second */
> pf_purge_expired_states(1 + (pf_status.states
> / pf_default_rule.timeout[PFTM_INTERVAL]));
>
> /* purge other expired types every PFTM_INTERVAL seconds */
> if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) {
> - pf_purge_expired_fragments();
> pf_purge_expired_src_nodes(0);
> pf_purge_expired_rules();
> + }
> + PF_UNLOCK();
> + /*
> + * Fragments don't require PF_LOCK(), they use their own mutex.
> + */
> + if (nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) {
> + pf_purge_expired_fragments();
> nloops = 0;
> }
>
Minor nit: change comment above to say "they use their own lock."
to remove the reference to a specific lock implementation.
> @@ -1320,7 +1327,6 @@ pf_remove_state(struct pf_state *cur)
> }
> RB_REMOVE(pf_state_tree_id, &tree_id, cur);
> #if NPFLOW > 0
> - if (cur->state_flags & PFSTATE_PFLOW)
> export_pflow(cur);
If you're removing this "if" statemenet, please remove the tab
from the next line.
> @@ -6692,6 +6699,9 @@ pf_test(sa_family_t af, int fwdir, struc
> }
> pd.m->m_pkthdr.pf.flags |= PF_TAG_PROCESSED;
>
> + /* lock the look-up/write section of pf_test() */
> + PF_LOCK();
> +
> switch (pd.virtual_proto) {
>
> case PF_VPROTO_FRAGMENT: {
Minor nit: spell "lookup" without a dash.
> @@ -6711,7 +6721,7 @@ pf_test(sa_family_t af, int fwdir, struc
> REASON_SET(&reason, PFRES_NORM);
> DPFPRINTF(LOG_NOTICE,
> "dropping IPv6 packet with ICMPv4 payload");
> - goto done;
> + goto unlock;
> }
> action = pf_test_state_icmp(&pd, &s, &reason);
> if (action == PF_PASS || action == PF_AFRT) {
> @@ -6736,7 +6746,7 @@ pf_test(sa_family_t af, int fwdir, struc
> REASON_SET(&reason, PFRES_NORM);
> DPFPRINTF(LOG_NOTICE,
> "dropping IPv4 packet with ICMPv6 payload");
> - goto done;
> + goto unlock;
> }
> action = pf_test_state_icmp(&pd, &s, &reason);
> if (action == PF_PASS || action == PF_AFRT) {
> @@ -6761,7 +6771,7 @@ pf_test(sa_family_t af, int fwdir, struc
> pqid = 1;
> action = pf_normalize_tcp(&pd);
> if (action == PF_DROP)
> - goto done;
> + goto unlock;
> }
> action = pf_test_state(&pd, &s, &reason);
> if (action == PF_PASS || action == PF_AFRT) {
> @@ -6788,6 +6798,15 @@ pf_test(sa_family_t af, int fwdir, struc
> break;
> }
>
> +unlock:
> + PF_UNLOCK();
> +
> + /*
> + * At the moment, we rely on NET_LOCK() to prevent removal of items
> + * we've collected above ('s', 'r', 'anchor' and 'ruleset'). They'll
> + * have to be refcounted when NET_LOCK() is gone.
> + */
> +
> done:
> if (action != PF_DROP) {
> if (s) {
> @@ -6991,6 +7010,7 @@ done:
> }
>
> *m0 = pd.m;
> +
> return (action);
> }
>
> diff -r 6abbb123112a src/sys/net/pf_ioctl.c
> --- a/src/sys/net/pf_ioctl.c Wed May 31 10:21:18 2017 +0200
> +++ b/src/sys/net/pf_ioctl.c Wed May 31 10:42:50 2017 +0200
> @@ -129,6 +129,10 @@ struct {
> TAILQ_HEAD(pf_tags, pf_tagname) pf_tags =
> TAILQ_HEAD_INITIALIZER(pf_tags),
> pf_qids = TAILQ_HEAD_INITIALIZER(pf_qids);
>
> +#ifdef WITH_PF_LOCK
> +struct rwlock pf_lock = RWLOCK_INITIALIZER("pf_lock");
> +#endif /* WITH_PF_LOCK */
> +
> #if (PF_QNAME_SIZE != PF_TAG_NAME_SIZE)
> #error PF_QNAME_SIZE must be equal to PF_TAG_NAME_SIZE
> #endif
Where do you want to define this WITH_PF_LOCK?
> diff -r 6abbb123112a src/sys/net/pfvar_priv.h
> --- a/src/sys/net/pfvar_priv.h Wed May 31 10:21:18 2017 +0200
> +++ b/src/sys/net/pfvar_priv.h Wed May 31 10:42:50 2017 +0200
> @@ -94,6 +98,36 @@ struct pf_pdesc {
> } hdr;
> };
>
> +#ifdef WITH_PF_LOCK
> +extern struct rwlock pf_lock;
> +
> +#define PF_LOCK() do { \
> + NET_ASSERT_LOCKED(); \
> + rw_enter_write(&pf_lock); \
> + } while (0)
> +
> +#define PF_UNLOCK() do { \
> + PF_ASSERT_LOCKED(); \
> + rw_exit_write(&pf_lock); \
> + } while (0)
> +
> +#define PF_ASSERT_LOCKED() do { \
> + if (rw_status(&pf_lock) != RW_WRITE) \
> + splassert_fail(RW_WRITE, \
> + rw_status(&pf_lock),__func__);\
> + } while (0)
> +
> +#define PF_ASSERT_UNLOCKED() do { \
> + if (rw_status(&pf_lock) == RW_WRITE) \
> + splassert_fail(0, rw_status(&pf_lock), __func__);\
> + } while (0)
> +#else /* !WITH_PF_LOCK */
> +#define PF_LOCK() (void)(0)
> +#define PF_UNLOCK() (void)(0)
> +#define PF_ASSERT_LOCKED() (void)(0)
> +#define PF_ASSERT_UNLOCKED() (void)(0)
> +#endif /* WITH_PF_LOCK */
> +
> #endif /* _KERNEL */
>
> #endif /* _NET_PFVAR_PRIV_H_ */
I suggest to make this look a tiny bit prettier:
#define PF_LOCK() do { \
NET_ASSERT_LOCKED(); \
rw_enter_write(&pf_lock); \
} while (0)
#define PF_UNLOCK() do { \
PF_ASSERT_LOCKED(); \
rw_exit_write(&pf_lock); \
} while (0)
#define PF_ASSERT_LOCKED() do { \
if (rw_status(&pf_lock) != RW_WRITE) \
splassert_fail(RW_WRITE, \
rw_status(&pf_lock), __func__);\
} while (0)
#define PF_ASSERT_UNLOCKED() do { \
if (rw_status(&pf_lock) == RW_WRITE) \
splassert_fail(0, \
rw_status(&pf_lock), __func__);\
} while (0)
