delete pf states by exact flow info and speed it up

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

delete pf states by exact flow info and speed it up

YASUOKA Masahiko-3
Hi,

I'd like to add "key" modifier to "pfctl -k" to delete pf states, like
below:

  # pfctl -k key -k 'tcp 10.0.0.1:80 <- 10.0.0.101:32123'

This deletion will be very fast if the RB tree of pf state table is
used to find the state.  The diff also includes the diff for that part
(DIOCKILLSTATES of pf_ioctl).

comments?

ok?

Index: sbin/pfctl/pfctl.8
===================================================================
RCS file: /var/cvs/openbsd/src/sbin/pfctl/pfctl.8,v
retrieving revision 1.167
diff -u -p -r1.167 pfctl.8
--- sbin/pfctl/pfctl.8 26 Jan 2017 18:45:12 -0000 1.167
+++ sbin/pfctl/pfctl.8 12 Apr 2017 06:12:39 -0000
@@ -235,6 +235,7 @@ Kill all of the state entries matching t
 .Ar host ,
 .Ar network ,
 .Ar label ,
+.Ar key ,
 or
 .Ar id .
 .Pp
@@ -266,7 +267,7 @@ To kill all states with the target
 .Pp
 .Dl # pfctl -k 0.0.0.0/0 -k host2
 .Pp
-It is also possible to kill states by rule label or state ID.
+It is also possible to kill states by rule label, state key or state ID.
 In this mode the first
 .Fl k
 argument is used to specify the type
@@ -276,6 +277,17 @@ from rules carrying the label
 .Dq foobar :
 .Pp
 .Dl # pfctl -k label -k foobar
+.Pp
+To kill one specific state by its key
+(protocol, host1, port1, direction, host2 and port2 in the same format
+of pfctl -s state),
+use the
+.Ar key
+modifier and as a second argument the state key.
+To kill a state whose protocol is TCP and originating from
+10.0.0.101:32123 to 10.0.0.1:80 use:
+.Pp
+.Dl # pfctl -k key -k 'tcp 10.0.0.1:80 <- 10.0.0.101:32123'
 .Pp
 To kill one specific state by its unique state ID
 (as shown by pfctl -s state -vv),
Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /var/cvs/openbsd/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.339
diff -u -p -r1.339 pfctl.c
--- sbin/pfctl/pfctl.c 27 Mar 2017 17:38:09 -0000 1.339
+++ sbin/pfctl/pfctl.c 12 Apr 2017 06:12:40 -0000
@@ -72,6 +72,8 @@ int pfctl_kill_src_nodes(int, const cha
 int pfctl_net_kill_states(int, const char *, int, int);
 int pfctl_label_kill_states(int, const char *, int, int);
 int pfctl_id_kill_states(int, int);
+int pfctl_key_kill_states(int, const char *, int, int);
+int pfctl_parse_host(char *, struct pf_rule_addr *);
 void pfctl_init_options(struct pfctl *);
 int pfctl_load_options(struct pfctl *);
 int pfctl_load_limit(struct pfctl *, unsigned int, unsigned int);
@@ -683,6 +685,122 @@ pfctl_id_kill_states(int dev, int opts)
  return (0);
 }
 
+int
+pfctl_key_kill_states(int dev, const char *iface, int opts, int rdomain)
+{
+ struct pfioc_state_kill psk;
+ char *s, *token, *tokens[4];
+ struct protoent *p;
+ u_int i, sidx, didx;
+
+ if (state_killers != 2 || (strlen(state_kill[1]) == 0)) {
+ warnx("no key specified");
+ usage();
+ }
+ memset(&psk, 0, sizeof(psk));
+
+ if (iface != NULL && strlcpy(psk.psk_ifname, iface,
+    sizeof(psk.psk_ifname)) >= sizeof(psk.psk_ifname))
+ errx(1, "invalid interface: %s", iface);
+
+ psk.psk_rdomain = rdomain;
+
+ s = strdup(state_kill[1]);
+ if (!s)
+ errx(1, "pfctl_key_kill_states: strdup");
+ i = 0;
+ while ((token = strsep(&s, " \t")) != NULL)
+ if (*token != '\0') {
+ if (i < 4)
+ tokens[i] = token;
+ i++;
+ }
+ if (i != 4)
+ 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]);
+ psk.psk_proto = p->p_proto;
+
+ if (strcmp(tokens[2], "->") == 0) {
+ sidx = 1;
+ didx = 3;
+ } else if (strcmp(tokens[2], "<-") == 0) {
+ sidx = 3;
+ didx = 1;
+ } else
+ errx(1, "invalid direction: %s", tokens[2]);
+
+ if (pfctl_parse_host(tokens[sidx], &psk.psk_src) == -1)
+ errx(1, "invalid host: %s", tokens[sidx]);
+ if (pfctl_parse_host(tokens[didx], &psk.psk_dst) == -1)
+ errx(1, "invalid host: %s", tokens[didx]);
+
+ if (ioctl(dev, DIOCKILLSTATES, &psk))
+ err(1, "DIOCKILLSTATES");
+
+ if ((opts & PF_OPT_QUIET) == 0)
+ fprintf(stderr, "killed %d states\n", psk.psk_killed);
+
+ return (0);
+}
+
+int
+pfctl_parse_host(char *str, struct pf_rule_addr *addr)
+{
+ char *s = NULL, *sbs, *sbe;
+ struct addrinfo hints, *ai;
+ struct sockaddr_in *sin4;
+ struct sockaddr_in6 *sin6;
+
+ s = strdup(str);
+ if (!s)
+ errx(1, "pfctl_parse_host: strdup");
+
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_socktype = SOCK_DGRAM; /* dummy */
+ hints.ai_flags = AI_NUMERICHOST;
+
+ if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
+ hints.ai_family = AF_INET6;
+ *(sbs++) = *sbe = '\0';
+ } else if ((sbs = strchr(s, ':')) != NULL) {
+ hints.ai_family = AF_INET;
+ *(sbs++) = '\0';
+ } else
+ goto error;
+
+ if (getaddrinfo(s, sbs, &hints, &ai) != 0)
+ goto error;
+
+ switch (ai->ai_family) {
+ case AF_INET:
+ sin4 = (struct sockaddr_in *)ai->ai_addr;
+ addr->addr.v.a.addr.v4 = sin4->sin_addr;
+ addr->port[0] = sin4->sin_port;
+ break;
+
+ case AF_INET6:
+ sin6 = (struct sockaddr_in6 *)ai->ai_addr;
+ addr->addr.v.a.addr.v6 = sin6->sin6_addr;
+ addr->port[0] = sin6->sin6_port;
+ break;
+ }
+ freeaddrinfo(ai);
+ free(s);
+
+ memset(&addr->addr.v.a.mask, 0xff, sizeof(struct pf_addr));
+ addr->port_op = PF_OP_EQ;
+ addr->addr.type = PF_ADDR_ADDRMASK;
+
+ return (0);
+
+ error:
+ free(s);
+ return (-1);
+}
+
 void
 pfctl_print_rule_counters(struct pf_rule *rule, int opts)
 {
@@ -2427,6 +2545,8 @@ main(int argc, char *argv[])
  pfctl_label_kill_states(dev, ifaceopt, opts, rdomain);
  else if (!strcmp(state_kill[0], "id"))
  pfctl_id_kill_states(dev, opts);
+ else if (!strcmp(state_kill[0], "key"))
+ pfctl_key_kill_states(dev, ifaceopt, opts, rdomain);
  else
  pfctl_net_kill_states(dev, ifaceopt, opts, rdomain);
  }

Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /var/cvs/openbsd/src/sys/net/pf_ioctl.c,v
retrieving revision 1.308
diff -u -p -r1.308 pf_ioctl.c
--- sys/net/pf_ioctl.c 17 Mar 2017 17:19:16 -0000 1.308
+++ sys/net/pf_ioctl.c 11 Apr 2017 05:07:18 -0000
@@ -1444,11 +1444,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 
  case DIOCKILLSTATES: {
  struct pf_state *s, *nexts;
- struct pf_state_key *sk;
+ struct pf_state_item *si, *sit;
+ struct pf_state_key *sk, key;
  struct pf_addr *srcaddr, *dstaddr;
  u_int16_t srcport, dstport;
  struct pfioc_state_kill *psk = (struct pfioc_state_kill *)addr;
- u_int killed = 0;
+ u_int i, killed = 0;
+ const int dirs[] = { PF_IN, PF_OUT };
+ int sidx, didx;
 
  if (psk->psk_pfcmp.id) {
  if (psk->psk_pfcmp.creatorid == 0)
@@ -1458,6 +1461,62 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
  psk->psk_killed = 1;
  }
  break;
+ }
+
+ if (psk->psk_af && psk->psk_proto &&
+    psk->psk_src.port_op == PF_OP_EQ &&
+    psk->psk_dst.port_op == PF_OP_EQ) {
+
+ key.af = psk->psk_af;
+ key.proto = psk->psk_proto;
+ key.rdomain = psk->psk_rdomain;
+
+ for (i = 0; i < nitems(dirs); i++) {
+ if (dirs[i] == PF_IN) {
+ sidx = 0;
+ didx = 1;
+ } else {
+ sidx = 1;
+ didx = 0;
+ }
+ PF_ACPY(&key.addr[sidx],
+    &psk->psk_src.addr.v.a.addr, key.af);
+ PF_ACPY(&key.addr[didx],
+    &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++;
+ }
+ }
+ if (killed) {
+ psk->psk_killed = killed;
+ break;
+ }
  }
 
  for (s = RB_MIN(pf_state_tree_id, &tree_id); s;

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

Alexandr Nedvedicky
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<--------

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

YASUOKA Masahiko-3
Hi,

On Fri, 21 Apr 2017 00:27:04 +0200
Alexandr Nedvedicky <[hidden email]> wrote:

> 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<--------

Yes, thanks.

>     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<--------

Also yes, I didn't notice this.

>     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

Right.  The condition is to delete with "key", matching the label is
not needed.

> - 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.

Indeed.  I overlooked this.

> --------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.

It seems good direction.

> Apart items 1 - 3, I'm OK with your change.

Thank you for your ok and useful feedbacks.

I'll use the updated diff attached below.

Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.308
diff -u -p -r1.308 pf_ioctl.c
--- sys/net/pf_ioctl.c 17 Mar 2017 17:19:16 -0000 1.308
+++ sys/net/pf_ioctl.c 21 Apr 2017 04:38:59 -0000
@@ -1444,11 +1444,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 
  case DIOCKILLSTATES: {
  struct pf_state *s, *nexts;
- struct pf_state_key *sk;
+ struct pf_state_item *si, *sit;
+ struct pf_state_key *sk, key;
  struct pf_addr *srcaddr, *dstaddr;
  u_int16_t srcport, dstport;
  struct pfioc_state_kill *psk = (struct pfioc_state_kill *)addr;
- u_int killed = 0;
+ u_int i, killed = 0;
+ const int dirs[] = { PF_IN, PF_OUT };
+ int sidx, didx;
 
  if (psk->psk_pfcmp.id) {
  if (psk->psk_pfcmp.creatorid == 0)
@@ -1457,6 +1460,57 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
  pf_remove_state(s);
  psk->psk_killed = 1;
  }
+ break;
+ }
+
+ if (psk->psk_af && psk->psk_proto &&
+    psk->psk_src.port_op == PF_OP_EQ &&
+    psk->psk_dst.port_op == PF_OP_EQ) {
+
+ key.af = psk->psk_af;
+ key.proto = psk->psk_proto;
+ key.rdomain = psk->psk_rdomain;
+
+ for (i = 0; i < nitems(dirs); i++) {
+ if (dirs[i] == PF_IN) {
+ sidx = 0;
+ didx = 1;
+ } else {
+ sidx = 1;
+ didx = 0;
+ }
+ PF_ACPY(&key.addr[sidx],
+    &psk->psk_src.addr.v.a.addr, key.af);
+ PF_ACPY(&key.addr[didx],
+    &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)))) {
+ pf_remove_state(si->s);
+ killed++;
+ }
+ }
+ if (killed)
+ psk->psk_killed = killed;
  break;
  }
 
Index: sbin/pfctl/pfctl.8
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.8,v
retrieving revision 1.167
diff -u -p -r1.167 pfctl.8
--- sbin/pfctl/pfctl.8 26 Jan 2017 18:45:12 -0000 1.167
+++ sbin/pfctl/pfctl.8 21 Apr 2017 04:39:59 -0000
@@ -229,12 +229,13 @@ option may be specified, which will kill
 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 ,
 .Ar network ,
 .Ar label ,
+.Ar key ,
 or
 .Ar id .
 .Pp
@@ -266,7 +267,7 @@ To kill all states with the target
 .Pp
 .Dl # pfctl -k 0.0.0.0/0 -k host2
 .Pp
-It is also possible to kill states by rule label or state ID.
+It is also possible to kill states by rule label, state key or state ID.
 In this mode the first
 .Fl k
 argument is used to specify the type
@@ -276,6 +277,17 @@ from rules carrying the label
 .Dq foobar :
 .Pp
 .Dl # pfctl -k label -k foobar
+.Pp
+To kill one specific state by its key
+(protocol, host1, port1, direction, host2 and port2 in the same format
+of pfctl -s state),
+use the
+.Ar key
+modifier and as a second argument the state key.
+To kill a state whose protocol is TCP and originating from
+10.0.0.101:32123 to 10.0.0.1:80 use:
+.Pp
+.Dl # pfctl -k key -k 'tcp 10.0.0.1:80 <- 10.0.0.101:32123'
 .Pp
 To kill one specific state by its unique state ID
 (as shown by pfctl -s state -vv),
Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.339
diff -u -p -r1.339 pfctl.c
--- sbin/pfctl/pfctl.c 27 Mar 2017 17:38:09 -0000 1.339
+++ sbin/pfctl/pfctl.c 21 Apr 2017 04:40:00 -0000
@@ -72,6 +72,8 @@ int pfctl_kill_src_nodes(int, const cha
 int pfctl_net_kill_states(int, const char *, int, int);
 int pfctl_label_kill_states(int, const char *, int, int);
 int pfctl_id_kill_states(int, int);
+int pfctl_key_kill_states(int, const char *, int, int);
+int pfctl_parse_host(char *, struct pf_rule_addr *);
 void pfctl_init_options(struct pfctl *);
 int pfctl_load_options(struct pfctl *);
 int pfctl_load_limit(struct pfctl *, unsigned int, unsigned int);
@@ -683,6 +685,122 @@ pfctl_id_kill_states(int dev, int opts)
  return (0);
 }
 
+int
+pfctl_key_kill_states(int dev, const char *iface, int opts, int rdomain)
+{
+ struct pfioc_state_kill psk;
+ char *s, *token, *tokens[4];
+ struct protoent *p;
+ u_int i, sidx, didx;
+
+ if (state_killers != 2 || (strlen(state_kill[1]) == 0)) {
+ warnx("no key specified");
+ usage();
+ }
+ memset(&psk, 0, sizeof(psk));
+
+ if (iface != NULL && strlcpy(psk.psk_ifname, iface,
+    sizeof(psk.psk_ifname)) >= sizeof(psk.psk_ifname))
+ errx(1, "invalid interface: %s", iface);
+
+ psk.psk_rdomain = rdomain;
+
+ s = strdup(state_kill[1]);
+ if (!s)
+ errx(1, "pfctl_key_kill_states: strdup");
+ i = 0;
+ while ((token = strsep(&s, " \t")) != NULL)
+ if (*token != '\0') {
+ if (i < 4)
+ tokens[i] = token;
+ i++;
+ }
+ if (i != 4)
+ 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]);
+ psk.psk_proto = p->p_proto;
+
+ if (strcmp(tokens[2], "->") == 0) {
+ sidx = 1;
+ didx = 3;
+ } else if (strcmp(tokens[2], "<-") == 0) {
+ sidx = 3;
+ didx = 1;
+ } else
+ errx(1, "invalid direction: %s", tokens[2]);
+
+ if (pfctl_parse_host(tokens[sidx], &psk.psk_src) == -1)
+ errx(1, "invalid host: %s", tokens[sidx]);
+ if (pfctl_parse_host(tokens[didx], &psk.psk_dst) == -1)
+ errx(1, "invalid host: %s", tokens[didx]);
+
+ if (ioctl(dev, DIOCKILLSTATES, &psk))
+ err(1, "DIOCKILLSTATES");
+
+ if ((opts & PF_OPT_QUIET) == 0)
+ fprintf(stderr, "killed %d states\n", psk.psk_killed);
+
+ return (0);
+}
+
+int
+pfctl_parse_host(char *str, struct pf_rule_addr *addr)
+{
+ char *s = NULL, *sbs, *sbe;
+ struct addrinfo hints, *ai;
+ struct sockaddr_in *sin4;
+ struct sockaddr_in6 *sin6;
+
+ s = strdup(str);
+ if (!s)
+ errx(1, "pfctl_parse_host: strdup");
+
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_socktype = SOCK_DGRAM; /* dummy */
+ hints.ai_flags = AI_NUMERICHOST;
+
+ if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
+ hints.ai_family = AF_INET6;
+ *(sbs++) = *sbe = '\0';
+ } else if ((sbs = strchr(s, ':')) != NULL) {
+ hints.ai_family = AF_INET;
+ *(sbs++) = '\0';
+ } else
+ goto error;
+
+ if (getaddrinfo(s, sbs, &hints, &ai) != 0)
+ goto error;
+
+ switch (ai->ai_family) {
+ case AF_INET:
+ sin4 = (struct sockaddr_in *)ai->ai_addr;
+ addr->addr.v.a.addr.v4 = sin4->sin_addr;
+ addr->port[0] = sin4->sin_port;
+ break;
+
+ case AF_INET6:
+ sin6 = (struct sockaddr_in6 *)ai->ai_addr;
+ addr->addr.v.a.addr.v6 = sin6->sin6_addr;
+ addr->port[0] = sin6->sin6_port;
+ break;
+ }
+ freeaddrinfo(ai);
+ free(s);
+
+ memset(&addr->addr.v.a.mask, 0xff, sizeof(struct pf_addr));
+ addr->port_op = PF_OP_EQ;
+ addr->addr.type = PF_ADDR_ADDRMASK;
+
+ return (0);
+
+ error:
+ free(s);
+ return (-1);
+}
+
 void
 pfctl_print_rule_counters(struct pf_rule *rule, int opts)
 {
@@ -2427,6 +2545,8 @@ main(int argc, char *argv[])
  pfctl_label_kill_states(dev, ifaceopt, opts, rdomain);
  else if (!strcmp(state_kill[0], "id"))
  pfctl_id_kill_states(dev, opts);
+ else if (!strcmp(state_kill[0], "key"))
+ pfctl_key_kill_states(dev, ifaceopt, opts, rdomain);
  else
  pfctl_net_kill_states(dev, ifaceopt, opts, rdomain);
  }

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

YASUOKA Masahiko-4
On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
YASUOKA Masahiko <[hidden email]> wrote:
> +int
> +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
> +{
(snip)
> + if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
> + hints.ai_family = AF_INET6;
> + *(sbs++) = *sbe = '\0';

The condition must be

        if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {

I will fix this before commit.

--yasuoka

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

Alexandr Nedvedicky
On Sat, Apr 22, 2017 at 08:14:06AM +0900, YASUOKA Masahiko wrote:

> On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
> YASUOKA Masahiko <[hidden email]> wrote:
> > +int
> > +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
> > +{
> (snip)
> > + if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
> > + hints.ai_family = AF_INET6;
> > + *(sbs++) = *sbe = '\0';
>
> The condition must be
>
> if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
>
> I will fix this before commit.
>

    good you could catch it.

thanks and
regards
sasha

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

Hiltjo Posthuma
In reply to this post by YASUOKA Masahiko-4
On Sat, Apr 22, 2017 at 08:14:06AM +0900, YASUOKA Masahiko wrote:

> On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
> YASUOKA Masahiko <[hidden email]> wrote:
> > +int
> > +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
> > +{
> (snip)
> > + if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
> > + hints.ai_family = AF_INET6;
> > + *(sbs++) = *sbe = '\0';
>
> The condition must be
>
> if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
>
> I will fix this before commit.
>
> --yasuoka
>

Hey,

Shouldn't it be:

        if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(sbs, ']')) != NULL) {
                                                              ^

Since the ] should come after [ ?

--
Kind regards,
Hiltjo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

YASUOKA Masahiko-3
On Sun, 23 Apr 2017 00:18:07 +0200
Hiltjo Posthuma <[hidden email]> wrote:

> On Sat, Apr 22, 2017 at 08:14:06AM +0900, YASUOKA Masahiko wrote:
>> On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
>> YASUOKA Masahiko <[hidden email]> wrote:
>> > +int
>> > +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
>> > +{
>> (snip)
>> > + if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
>> > + hints.ai_family = AF_INET6;
>> > + *(sbs++) = *sbe = '\0';
>>
>> The condition must be
>>
>> if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
>>
>> I will fix this before commit.
>
> Hey,
>
> Shouldn't it be:
>
> if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(sbs, ']')) != NULL) {
>                                                      ^
>
> Since the ] should come after [ ?

Yes, it's better.  Thanks.

ok?

Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.340
diff -u -p -r1.340 pfctl.c
--- sbin/pfctl/pfctl.c 21 Apr 2017 23:22:49 -0000 1.340
+++ sbin/pfctl/pfctl.c 23 Apr 2017 09:02:20 -0000
@@ -762,7 +762,8 @@ pfctl_parse_host(char *str, struct pf_ru
  hints.ai_socktype = SOCK_DGRAM; /* dummy */
  hints.ai_flags = AI_NUMERICHOST;
 
- if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
+ if ((sbs = strchr(s, '[')) != NULL &&
+    (sbe = strrchr(sbs, ']')) != NULL) {
  hints.ai_family = AF_INET6;
  *(sbs++) = *sbe = '\0';
  } else if ((sbs = strchr(s, ':')) != NULL) {


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: delete pf states by exact flow info and speed it up

Alexandr Nedvedicky
In reply to this post by Hiltjo Posthuma
Hello,

On Sun, Apr 23, 2017 at 12:18:07AM +0200, Hiltjo Posthuma wrote:

> On Sat, Apr 22, 2017 at 08:14:06AM +0900, YASUOKA Masahiko wrote:
> > On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
> > YASUOKA Masahiko <[hidden email]> wrote:
> > > +int
> > > +pfctl_parse_host(char *str, struct pf_rule_addr *addr)
> > > +{
> > (snip)
> > > + if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
> > > + hints.ai_family = AF_INET6;
> > > + *(sbs++) = *sbe = '\0';
> >
> > The condition must be
> >
> > if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(s, ']')) != NULL) {
> >
> > I will fix this before commit.
> >
> > --yasuoka
> >
>
> Hey,
>
> Shouldn't it be:
>
> if ((sbs = strchr(s, '[')) != NULL && (sbe = strrchr(sbs, ']')) != NULL) {
>                                                      ^
>
> Since the ] should come after [ ?
>

    I think it does not matter. consider situation as follows:

        s = "[ whatever ]"
        sbs = strchr(s, '[');
        /* sbs contains '[ whatever ]', s and sbs are same */

    I would leave it on Yasuoka's personal choice...

thanks and
regards
sasha
       

Loading...