Hello,
I finally got to your patch. I'm fine with your to extend pfctl kill operation.
I've found just few nits in your change.
1) your manpage diff should include one more change below:
--------8<---------------8<---------------8<------------------8<--------
diff -r c435e977886a -r 40aa0af6704c src/sbin/pfctl/pfctl.8
--- a/src/sbin/pfctl/pfctl.8 Thu Apr 20 19:53:47 2017 +0200
+++ b/src/sbin/pfctl/pfctl.8 Thu Apr 20 22:55:43 2017 +0200
@@ -229,7 +229,7 @@
entries from the first host/network to the second.
.It Xo
.Fl k
-.Ar host | network | label | id
+.Ar host | network | label | key | id
.Xc
Kill all of the state entries matching the specified
.Ar host ,
--------8<---------------8<---------------8<------------------8<--------
2) pfctl.c, line exceeds 80 characters
--------8<---------------8<---------------8<------------------8<--------
diff -r c435e977886a -r 40aa0af6704c src/sbin/pfctl/pfctl.c
--- a/src/sbin/pfctl/pfctl.c Thu Apr 20 19:53:47 2017 +0200
+++ b/src/sbin/pfctl/pfctl.c Thu Apr 20 22:55:43 2017 +0200
@@ -716,8 +716,8 @@
i++;
}
if (i != 4)
- errx(1, "pfctl_key_kill_states: key must be \"protocol
host1:port1 "
- "direction host2:port2\" format");
+ errx(1, "pfctl_key_kill_states: key must be "
+ "\"protocol host1:port1 direction host2:port2\" format");
if ((p = getprotobyname(tokens[0])) == NULL)
errx(1, "invalid protocol: %s", tokens[0]);
--------8<---------------8<---------------8<------------------8<--------
3) pf_ioctl.c, there are two nits:
- I think we don't need to try to match label. at least the
pfctl_key_kill_states() does not set the label in key
- I think your if () branch should always break from switch ()
statement, regardless states got killed or not. Not doing so
will make kill operation to walk through entire state table.
I think it is not desired.
--------8<---------------8<---------------8<------------------8<--------
diff -r c435e977886a src/sys/net/pf_ioctl.c
--- a/src/sys/net/pf_ioctl.c Thu Apr 20 19:53:47 2017 +0200
+++ b/src/sys/net/pf_ioctl.c Thu Apr 20 23:55:09 2017 +0200
@@ -1504,19 +1504,14 @@
(!psk->psk_ifname[0] ||
(si->s->kif != pfi_all &&
!strcmp(psk->psk_ifname,
- si->s->kif->pfik_name))) &&
- (!psk->psk_label[0] ||
- (si->s->rule.ptr->label[0] &&
- !strcmp(psk->psk_label,
- si->s->rule.ptr->label)))) {
+ si->s->kif->pfik_name)))) {
pf_remove_state(si->s);
killed++;
}
}
- if (killed) {
+ if (killed)
psk->psk_killed = killed;
- break;
- }
+ break;
}
for (s = RB_MIN(pf_state_tree_id, &tree_id); s;
--------8<---------------8<---------------8<------------------8<--------
There is one more thing I was thinking of. The code, which you are adding to
pfioctl() essentially duplicates pf_find_state(). With little effort we can use
pf_find_state() to find states to remove. I just gave it a try, the patch is
further below. However I think we should leave it for another day. It would
be too big step for now.
Apart items 1 - 3, I'm OK with your change.
thanks and
regards
sasha
--------8<---------------8<---------------8<------------------8<--------
diff -r 40aa0af6704c -r 93e8fd17b3e1 src/sys/net/pf.c
--- a/src/sys/net/pf.c Thu Apr 20 22:55:43 2017 +0200
+++ b/src/sys/net/pf.c Thu Apr 20 23:35:15 2017 +0200
@@ -982,6 +982,9 @@ pf_compare_state_keys(struct pf_state_ke
}
}
+/*
+ * Note: m is NULL, when function is invoked on behalf of ioctl(2).
+ */
struct pf_state *
pf_find_state(struct pfi_kif *kif, struct pf_state_key_cmp *key, u_int dir,
struct mbuf *m)
@@ -999,7 +1002,7 @@ pf_find_state(struct pfi_kif *kif, struc
inp_sk = NULL;
pkt_sk = NULL;
sk = NULL;
- if (dir == PF_OUT) {
+ if ((m != NULL) && (dir == PF_OUT)) {
/* first if block deals with outbound forwarded packet */
pkt_sk = m->m_pkthdr.pf.statekey;
@@ -1031,12 +1034,12 @@ pf_find_state(struct pfi_kif *kif, struc
if (dir == PF_OUT && pkt_sk &&
pf_compare_state_keys(pkt_sk, sk, kif, dir) == 0)
pf_state_key_link(sk, pkt_sk);
- else if (dir == PF_OUT)
+ else if ((m != NULL) && (dir == PF_OUT))
pf_inp_link(m, m->m_pkthdr.pf.inp);
}
/* remove firewall data from outbound packet */
- if (dir == PF_OUT)
+ if ((m != NULL) && (dir == PF_OUT))
pf_pkt_addr_changed(m);
/* list is sorted, if-bound states before floating ones */
diff -r 40aa0af6704c -r 93e8fd17b3e1 src/sys/net/pf_ioctl.c
--- a/src/sys/net/pf_ioctl.c Thu Apr 20 22:55:43 2017 +0200
+++ b/src/sys/net/pf_ioctl.c Thu Apr 20 23:35:15 2017 +0200
@@ -1444,14 +1444,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCKILLSTATES: {
struct pf_state *s, *nexts;
- struct pf_state_item *si, *sit;
- struct pf_state_key *sk, key;
+ struct pf_state_key *sk;
+ struct pf_state_key_cmp key;
struct pf_addr *srcaddr, *dstaddr;
u_int16_t srcport, dstport;
struct pfioc_state_kill *psk = (struct pfioc_state_kill *)addr;
u_int i, killed = 0;
const int dirs[] = { PF_IN, PF_OUT };
int sidx, didx;
+ struct pfi_kif *kif;
if (psk->psk_pfcmp.id) {
if (psk->psk_pfcmp.creatorid == 0)
@@ -1470,6 +1471,23 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
key.af = psk->psk_af;
key.proto = psk->psk_proto;
key.rdomain = psk->psk_rdomain;
+ if (psk->psk_ifname[0]) {
+ kif = pfi_kif_find(psk->psk_ifname);
+ /*
+ * nothing to kill if kif does not exist
+ */
+ if (kif == NULL)
+ break;
+ } else {
+ /*
+ * administrator has not specified an
+ * interface, we want pf_find_state() to match
+ * floating states only. The pfi_all does not
+ * match if-bound states.
+ */
+ kif = pfi_all;
+ }
+
for (i = 0; i < nitems(dirs); i++) {
if (dirs[i] == PF_IN) {
@@ -1485,33 +1503,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
&psk->psk_dst.addr.v.a.addr, key.af);
key.port[sidx] = psk->psk_src.port[0];
key.port[didx] = psk->psk_dst.port[0];
-
- sk = RB_FIND(pf_state_tree, &pf_statetbl, &key);
- if (sk == NULL)
- continue;
-
- TAILQ_FOREACH_SAFE(si, &sk->states, entry, sit)
- if (((si->s->key[PF_SK_WIRE]->af ==
- si->s->key[PF_SK_STACK]->af &&
- sk == (dirs[i] == PF_IN ?
- si->s->key[PF_SK_WIRE] :
- si->s->key[PF_SK_STACK])) ||
- (si->s->key[PF_SK_WIRE]->af !=
- si->s->key[PF_SK_STACK]->af &&
- dirs[i] == PF_IN &&
- (sk == si->s->key[PF_SK_STACK] ||
- sk == si->s->key[PF_SK_WIRE]))) &&
- (!psk->psk_ifname[0] ||
- (si->s->kif != pfi_all &&
- !strcmp(psk->psk_ifname,
- si->s->kif->pfik_name))) &&
- (!psk->psk_label[0] ||
- (si->s->rule.ptr->label[0] &&
- !strcmp(psk->psk_label,
- si->s->rule.ptr->label)))) {
- pf_remove_state(si->s);
- killed++;
- }
+ s = pf_find_state(kif, &key, dirs[i], NULL);
+ if (s != NULL) {
+ pf_remove_state(s);
+ killed++;
+ }
}
if (killed)
psk->psk_killed = killed;
diff -r 40aa0af6704c -r 93e8fd17b3e1 src/sys/net/pfvar.h
--- a/src/sys/net/pfvar.h Thu Apr 20 22:55:43 2017 +0200
+++ b/src/sys/net/pfvar.h Thu Apr 20 23:35:15 2017 +0200
@@ -1618,6 +1618,9 @@ extern void
pf_purge_expired_src_node
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 struct pf_state *pf_find_state(struct pfi_kif *,
+ struct pf_state_key_cmp *, u_int,
+ struct mbuf *);
extern void pf_remove_divert_state(struct pf_state_key *);
extern void pf_free_state(struct pf_state *);
extern int pf_state_insert(struct pfi_kif *,
--------8<---------------8<---------------8<------------------8<--------
