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

[Alexandr Nedvedicky]

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

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

[YASUOKA Masahiko]

On Sun, 23 Apr 2017 00:18:07 +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  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 -  1.340
+++ sbin/pfctl/pfctl.c  23 Apr 2017 09:02:20 -
@@ -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) {

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

[Hiltjo Posthuma]

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

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

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

[YASUOKA Masahiko]

On Fri, 21 Apr 2017 13:52:51 +0900 (JST)
YASUOKA Masahiko  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

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

[YASUOKA Masahiko]

Hi,

On Fri, 21 Apr 2017 00:27:04 +0200
Alexandr Nedvedicky  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.8Thu Apr 20 19:53:47 2017 +0200
> +++ b/src/sbin/pfctl/pfctl.8Thu 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.cThu Apr 20 19:53:47 2017 +0200
> +++ b/src/sbin/pfctl/pfctl.cThu 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.cThu Apr 20 19:53:47 2017 +0200
> +++ b/src/sys/net/pf_ioctl.cThu 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, _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 -  1.308
+++ sys/net/pf_ioctl.c  21 Apr 2017 04:38:59 -
@@ -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 

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.8Thu Apr 20 19:53:47 2017 +0200
+++ b/src/sbin/pfctl/pfctl.8Thu 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.cThu Apr 20 19:53:47 2017 +0200
+++ b/src/sbin/pfctl/pfctl.cThu 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.cThu Apr 20 19:53:47 2017 +0200
+++ b/src/sys/net/pf_ioctl.cThu 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, _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);
-