Re: pf "route-to least-state" in an anchor doesn't work

2020-06-03 Thread YASUOKA Masahiko
Hello,

On Wed, 3 Jun 2020 23:30:56 +0200
Alexandr Nedvedicky  wrote:
> I'm OK with your change.

Thank you for your review and comment.

> However I would like to ask you to do yet another test.  I wonder if things
> will eventually work on unfixed PF if rules will be constructed as follows:
> 
> pfctl -a test -t LB -T add 10.0.0.11@pair102
> 
> echo 'pass in on rdomain 102 quick proto tcp to 10.0.0.101 port 8080 \
> keep state ( sloppy ) route-to  \
> least-states sticky-address' |pfctl -a test -f -
> 
> echo 'anchor test' | pfctl -f -
> 
> pfctl -e
> 
> I suspect the bug you've found and fixed happens when pfctl loads rules
> from pf.conf. I think the steps above will take a different route
> through the code, which avoids pfr_ina_define() (a.k.a. transactions).

I've tested it before the diff and after.  Both tests were OK.

  # pfctl -sr -a test   
 
  pass in quick on rdomain 102 inet proto tcp from any to 10.0.0.101 port = 
8080 flags S/SA keep state (sloppy) route-to  least-states sticky-address
  # pfctl -a test -tLB -Tshow
 10.0.0.11@pair102
  # 

  $ doas route -T 101 exec telnet 10.0.0.101 8080
  Trying 10.0.0.101...
  Connected to 10.0.0.101.
  Escape character is '^]'.
  ^]
  
  telnet> close
  Connection closed.
  $ 

> I don't have a test system readily available and I'm just curious
> if anything changes or not. Thanks for finding that for me.
> 
> As I've said I think your change should go in.
> 
> OK sashan

Thanks,



Re: pf "route-to least-state" in an anchor doesn't work

2020-06-03 Thread Alexandr Nedvedicky
Hello Yasuoka,

I'm OK with your change.

However I would like to ask you to do yet another test.  I wonder if things
will eventually work on unfixed PF if rules will be constructed as follows:

pfctl -a test -t LB -T add 10.0.0.11@pair102

echo 'pass in on rdomain 102 quick proto tcp to 10.0.0.101 port 8080 \
keep state ( sloppy ) route-to  \
least-states sticky-address' |pfctl -a test -f -

echo 'anchor test' | pfctl -f -

pfctl -e

I suspect the bug you've found and fixed happens when pfctl loads rules
from pf.conf. I think the steps above will take a different route
through the code, which avoids pfr_ina_define() (a.k.a. transactions).

I don't have a test system readily available and I'm just curious
if anything changes or not. Thanks for finding that for me.

As I've said I think your change should go in.

OK sashan



pf "route-to least-state" in an anchor doesn't work

2020-06-03 Thread YASUOKA Masahiko
Hi,

pf.conf:

  anchor {
pass in on rdomain 102 quick proto tcp to 10.0.0.101 port 8080 \
  keep state ( sloppy ) route-to  \
  least-states sticky-address
  }
  table  {
10.0.0.11@pair102
  }

this doesn't work.  All packets going to 10.0.0.101 are dropped with
'no-route'.  The problem doesn't happen if the pass rule is moved to
outside of the anchor or uses "round-robin" instead of "least-states".

In sys/net/pf_lb.c:
594 if (rpool->addr.type == PF_ADDR_TABLE) {
595 if (pfr_states_increase(rpool->addr.p.tbl,
596 naddr, af) == -1) {
597 if (pf_status.debug >= LOG_DEBUG) {
598 log(LOG_DEBUG,"pf: pf_map_addr: 
"
599 "selected address ");
600 pf_print_host(naddr, 0, af);
601 addlog(". Failed to increase 
count!\n");
602 }
603 return (1);
604 }

This chunk is to increase the counter for "least-state".  The packets
drops here because pfr_states_increase() returns -1.
pfr_states_increase() uses pfr_kentry_byaddr(), and
pfr_kentry_byaddr() uses pfr_lookup_addr() to lookup a kentry in the
table.

pfr_lookup_addr() never succeeded for above case, because it doesn't
care about using global (root) tables from rules in an anchor.  All
other functions which lookup a kentry from the table than
pfr_lookup_addr() seem to take care about that.

I thought that pfr_lookup_addr() is a local function used for ioctl to
create tables and manage its members.  So the keep it
untouched. Instead, the diff replaces the body of pfr_kentry_byaddr()
by the logic of pfr_match_addr().

* * *
Test

1. prepare network

  ifconfig pair101 rdomain 101 10.0.0.1/24
  ifconfig pair102 rdomain 102 10.0.0.10/24
  ifconfig pair102 alias 10.0.0.101/24
  ifconfig pair103 rdomain 103 10.0.0.11/24
  ifconfig pair104 rdomain 100 patch pair101 up
  ifconfig pair105 rdomain 100 patch pair102 up
  ifconfig pair106 rdomain 100 patch pair103 up
  ifconfig lo103 127.0.0.1/8
  ifconfig lo103 alias 10.0.0.101/24

  ifconfig bridge100 add pair104
  ifconfig bridge100 add pair105
  ifconfig bridge100 add pair106 up

2. setup pf.conf

  anchor {
pass in on rdomain 102 quick proto tcp to 10.0.0.101 port 8080 \
  keep state ( sloppy ) route-to  \
  least-states sticky-address
  }
  table  {
10.0.0.11@pair102
  }

3. start a daemon on 8080/tcp on #103

   doas route -T 103 exec nc -l 8080

4. try to connect to it from #101

   doas route -T 101 exec telnet 10.0.0.101 8080

   - test OK if the connection is established

5. teardown

  ifconfig pair106 destroy
  ifconfig pair105 destroy
  ifconfig pair104 destroy
  ifconfig pair103 destroy
  ifconfig pair102 destroy
  ifconfig pair101 destroy
  ifconfig bridge100 destroy

* * *

ok?

Fix pfr_kentry_byaddr() to be used for a rule in an anchor.  It
couldn't find an entry if its table is attached a table on the root.
This fixes the problem "route-to  least-states" doesn't work.
The problem is found by IIJ.

Index: sys/net/pf_table.c
===
RCS file: /cvs/src/sys/net/pf_table.c,v
retrieving revision 1.131
diff -u -p -r1.131 pf_table.c
--- sys/net/pf_table.c  8 Jul 2019 17:49:57 -   1.131
+++ sys/net/pf_table.c  3 Jun 2020 07:21:27 -
@@ -2085,11 +2085,28 @@ int
 pfr_match_addr(struct pfr_ktable *kt, struct pf_addr *a, sa_family_t af)
 {
struct pfr_kentry   *ke = NULL;
+   int  match;
+
+   ke = pfr_kentry_byaddr(kt, a, af, 0);
+
+   match = (ke && !(ke->pfrke_flags & PFRKE_FLAG_NOT));
+   if (match)
+   kt->pfrkt_match++;
+   else
+   kt->pfrkt_nomatch++;
+
+   return (match);
+}
+
+struct pfr_kentry *
+pfr_kentry_byaddr(struct pfr_ktable *kt, struct pf_addr *a, sa_family_t af,
+int exact)
+{
+   struct pfr_kentry   *ke = NULL;
struct sockaddr_in   tmp4;
 #ifdef INET6
struct sockaddr_in6  tmp6;
 #endif /* INET6 */
-   int  match;
 
if (!(kt->pfrkt_flags & PFR_TFLAG_ACTIVE) && kt->pfrkt_root != NULL)
kt = kt->pfrkt_root;
@@ -2116,12 +2133,10 @@ pfr_match_addr(struct pfr_ktable *kt, st
default:
unhandled_af(af);
}
-   match = (ke && !(ke->pfrke_flags & PFRKE_FLAG_NOT));
-   if (match)
-   kt->pfrkt_match++;
-   else
-   kt->pfrkt_nomatch++;
-   return (match);
+   if (exact && ke && KENTRY_NETWORK(ke))
+   ke = NULL;
+
+   return (ke);
 }
 
 void
@@ -2497,39 +2512,6 @@ pfr_states_decrease(struct pfr_ktable *k
"pfr_states_decrease: states-- when states <= 0");