Re: pf: route-to least-states
Hello Yasuoka, On Wed, Jul 29, 2020 at 01:55:20AM +0900, YASUOKA Masahiko wrote: > Hi, > > Let me add another fix of previous. > > ok? OK. thanks for taking care of that. I've entirely missed 1 vs. -1 return value, when reviewing your change. regards sashan
Re: pf: route-to least-states
Hi,
On Tue, 28 Jul 2020 18:54:48 +0200
Alexandr Nedvedicky wrote:
> On Wed, Jul 29, 2020 at 01:22:48AM +0900, YASUOKA Masahiko wrote:
>> Previous commit has a wrong part..
>>
>> ok?
>>
>> Fix previous commit which referred wrong address.
>
> would it make sense to move the block, you've introduced earler
> under the !PF_AZERO() branch just couple lines below. something
> like this:
>
> 8<---8<---8<--8<
> diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c
> index 510795a4d0b..f77d96a99ec 100644
> --- a/sys/net/pf_lb.c
> +++ b/sys/net/pf_lb.c
> @@ -322,13 +322,13 @@ pf_map_addr_sticky(sa_family_t af, struct pf_rule *r,
> struct pf_addr *saddr,
> return (-1);
> }
>
> - if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
> - if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + if (!PF_AZERO(cached, af)) {
> + pf_addrcpy(naddr, cached, af);
> + if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES)
> &&
> + ((pf_map_addr_states_increase(af, rpool, cached) == -1))
> return (-1);
> }
>
> - if (!PF_AZERO(cached, af))
> - pf_addrcpy(naddr, cached, af);
> if (pf_status.debug >= LOG_DEBUG) {
> log(LOG_DEBUG, "pf: pf_map_addr: "
> "src tracking (%u) maps ", type);
>
> 8<---8<---8<--8<
>
> It seems to me it would be better to bump number of states if and only if we
> actually find some address in pool.
Yes, I agree.
ok?
Fix previous commit which referred wrong address and returned wrong
value.
Index: sys/net/pf_lb.c
===
RCS file: /cvs/src/sys/net/pf_lb.c,v
retrieving revision 1.66
diff -u -p -r1.66 pf_lb.c
--- sys/net/pf_lb.c 28 Jul 2020 16:47:41 - 1.66
+++ sys/net/pf_lb.c 28 Jul 2020 17:01:34 -
@@ -322,13 +322,13 @@ pf_map_addr_sticky(sa_family_t af, struc
return (-1);
}
- if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
- if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
- return (-1);
- }
- if (!PF_AZERO(cached, af))
+ if (!PF_AZERO(cached, af)) {
pf_addrcpy(naddr, cached, af);
+ if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES &&
+ pf_map_addr_states_increase(af, rpool, cached) == -1)
+ return (-1);
+ }
if (pf_status.debug >= LOG_DEBUG) {
log(LOG_DEBUG, "pf: pf_map_addr: "
"src tracking (%u) maps ", type);
@@ -651,7 +651,7 @@ pf_map_addr_states_increase(sa_family_t
pf_print_host(naddr, 0, af);
addlog(". Failed to increase count!\n");
}
- return (1);
+ return (-1);
}
} else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
if (pfr_states_increase(rpool->addr.p.dyn->pfid_kt,
@@ -663,7 +663,7 @@ pf_map_addr_states_increase(sa_family_t
pf_print_host(naddr, 0, af);
addlog(". Failed to increase count!\n");
}
- return (1);
+ return (-1);
}
}
return (0);
Re: pf: route-to least-states
Hello Yasuoka,
On Wed, Jul 29, 2020 at 01:22:48AM +0900, YASUOKA Masahiko wrote:
> Hi,
>
> Previous commit has a wrong part..
>
> ok?
>
> Fix previous commit which referred wrong address.
would it make sense to move the block, you've introduced earler
under the !PF_AZERO() branch just couple lines below. something
like this:
8<---8<---8<--8<
diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c
index 510795a4d0b..f77d96a99ec 100644
--- a/sys/net/pf_lb.c
+++ b/sys/net/pf_lb.c
@@ -322,13 +322,13 @@ pf_map_addr_sticky(sa_family_t af, struct pf_rule *r,
struct pf_addr *saddr,
return (-1);
}
- if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
- if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ if (!PF_AZERO(cached, af)) {
+ pf_addrcpy(naddr, cached, af);
+ if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) &&
+ ((pf_map_addr_states_increase(af, rpool, cached) == -1))
return (-1);
}
- if (!PF_AZERO(cached, af))
- pf_addrcpy(naddr, cached, af);
if (pf_status.debug >= LOG_DEBUG) {
log(LOG_DEBUG, "pf: pf_map_addr: "
"src tracking (%u) maps ", type);
8<---8<---8<--8<
It seems to me it would be better to bump number of states if and only if we
actually find some address in pool.
thanks and
regards
sashan
Re: pf: route-to least-states
Hi,
Let me add another fix of previous.
ok?
Fix previous commit which referred wrong address and returned wrong
value.
Index: sys/net/pf_lb.c
===
RCS file: /cvs/src/sys/net/pf_lb.c,v
retrieving revision 1.66
diff -u -p -r1.66 pf_lb.c
--- sys/net/pf_lb.c 28 Jul 2020 16:47:41 - 1.66
+++ sys/net/pf_lb.c 28 Jul 2020 16:52:24 -
@@ -323,7 +323,7 @@ pf_map_addr_sticky(sa_family_t af, struc
}
if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
- if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ if (pf_map_addr_states_increase(af, rpool, cached) == -1)
return (-1);
}
@@ -651,7 +651,7 @@ pf_map_addr_states_increase(sa_family_t
pf_print_host(naddr, 0, af);
addlog(". Failed to increase count!\n");
}
- return (1);
+ return (-1);
}
} else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
if (pfr_states_increase(rpool->addr.p.dyn->pfid_kt,
@@ -663,7 +663,7 @@ pf_map_addr_states_increase(sa_family_t
pf_print_host(naddr, 0, af);
addlog(". Failed to increase count!\n");
}
- return (1);
+ return (-1);
}
}
return (0);
Re: pf: route-to least-states
Hi,
Previous commit has a wrong part..
ok?
Fix previous commit which referred wrong address.
Index: sys/net/pf_lb.c
===
RCS file: /cvs/src/sys/net/pf_lb.c,v
retrieving revision 1.65
diff -u -p -r1.65 pf_lb.c
--- sys/net/pf_lb.c 24 Jul 2020 14:06:33 - 1.65
+++ sys/net/pf_lb.c 28 Jul 2020 16:15:50 -
@@ -323,7 +323,7 @@ pf_map_addr_sticky(sa_family_t af, struc
}
if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
- if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ if (pf_map_addr_states_increase(af, rpool, cached) == -1)
return (-1);
}
Re: pf: route-to least-states
Hello Yasuoka, > > Yes. > > Let me simplify the block for "least-states". > thanks for your explanation. It helped me to understand the code. I'm OK with your fix. thanks and regards sashan
Re: pf: route-to least-states
Hi,
Thank you for your review.
On Fri, 24 Jul 2020 01:25:42 +0200
Alexandr Nedvedicky wrote:
>> - interface is not selected properly if selected table entry specifies
>> an interface.
>
> to be honest I don't quite understand what's going on here.
> can you share some details of configuration/scenario, which
> triggers the bug your diff is fixing?
You seem to have understood the scenario correctly.
> the part of your change, which I'm not able to figure out is
> this single line:
>
>> +if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
>> +return (1);
>> +/* revert the kif which was set by pfr_pool_get() */
>> +rpool->kif = kif;
>> break;
>> }
>
> your fix changes behavior, which is there since least-state
> option has been introduced. I believe it does not matter
> for case when route-to specifies single interface such as:
>
> route-to 192.168.1.10@em0 least-states
>
> I'm not sure what will happen in situation, when there are more interfaces
> specified in combination with sticky-address:
>
> route-to {192.168.1.10@em0, 192.168.1.20@em1} last-states sticky-address
Yes. This is a senario.
> the resulting code does not look quite right with your diff applied:
>
> 602 } while (pf_match_addr(1, , rmask, >counter,
> af) &&
> 603 (states > 0));
> 604
> 605 if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> 606 return (1);
> 607 /* revert the kif which was set by pfr_pool_get() */
> 608 rpool->kif = kif;
> 609 break;
> 610 }
> 611
> 612 if (rpool->opts & PF_POOL_STICKYADDR) {
> 613 if (sns[type] != NULL) {
> 614 pf_remove_src_node(sns[type]);
> 615 sns[type] = NULL;
> 616 }
> 617 if (pf_insert_src_node([type], r, type, af, saddr,
> naddr,
> 618 rpool->kif))
> 619 return (1);
> 620 }
>
>
> at line 608 new code reverts kif set by pfr_pool_get(). At line 617
> (executed when sticky-address is set) the original code passes kif chosen
> be
> pfr_pool_get(). You diff changes that behavior. So my question is simple:
> is that intentional change?
Yes.
Let me simplify the block for "least-states".
535 case PF_POOL_LEASTSTATES:
539 pfr_pool_get(rpool); // fist entry
:
561 faddr = rpool->counter; //record as final
:
557 load = rpool->states / rpool->weight;
563 naddr = rpool->counter;
:
571 do {
572 rpool->counter++;
575 pfr_pool_get(rpool); /* next entry */
:
585 cload = rpool->states / rpool->weight;
:
: /* find lc minimum */
591 if (cload < load) {
595 load = cload;
597 naddr = rpool->counter;
601 }
603 } while (raddr->counter != faddr); // loop until final
the loop #571:606 is to find the minimum (least-states) entry. If the
last entry is not the minimum, after the loop,
rpool <= the last entry
naddr <= the minimum entry
Also see the pfr_pool_get():
2272 int
2273 pfr_pool_get(struct pf_pool *rpool, struct pf_addr **raddr,
2274 struct pf_addr **rmask, sa_family_t af)
2275 {
(snip)
2417 rpool->states = 0;
2418 if (ke->pfrke_counters != NULL)
2419 rpool->states = ke->pfrke_counters->states;
2420 switch (ke->pfrke_type) {
2421 case PFRKE_COST:
2422 rpool->weight =
2423 ((struct pfr_kentry_cost *)ke)->weight;
2424 /* FALLTHROUGH */
2425 case PFRKE_ROUTE:
2426 rpool->kif = ((struct pfr_kentry_route
*)ke)->kif;
2427 break;
2428 default:
2429 rpool->weight = 1;
2430 break;
2431 }
some fields of rpool (states, weight, kif) are set by the values of
the selected table entry.
So,
| rpool <= the last entry
| naddr <= the minimum entry
rpool->kif is the interface of the last entery. It might be different
than the inteface of the minimum entry.
The diff is to keep kif of the minimum entry,
+ kif = rpool->kif;
revert rpool->kif by it after the loop.
+ /* revert the kif which was set by pfr_pool_get() */
+ rpool->kif = kif;
Re: pf: route-to least-states
Hello Yasuoka,
> - interface is not selected properly if selected table entry specifies
> an interface.
to be honest I don't quite understand what's going on here.
can you share some details of configuration/scenario, which
triggers the bug your diff is fixing?
the part of your change, which I'm not able to figure out is
this single line:
> + if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + return (1);
> + /* revert the kif which was set by pfr_pool_get() */
> + rpool->kif = kif;
> break;
> }
your fix changes behavior, which is there since least-state
option has been introduced. I believe it does not matter
for case when route-to specifies single interface such as:
route-to 192.168.1.10@em0 least-states
I'm not sure what will happen in situation, when there are more interfaces
specified in combination with sticky-address:
route-to {192.168.1.10@em0, 192.168.1.20@em1} last-states sticky-address
the resulting code does not look quite right with your diff applied:
602 } while (pf_match_addr(1, , rmask, >counter,
af) &&
603 (states > 0));
604
605 if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
606 return (1);
607 /* revert the kif which was set by pfr_pool_get() */
608 rpool->kif = kif;
609 break;
610 }
611
612 if (rpool->opts & PF_POOL_STICKYADDR) {
613 if (sns[type] != NULL) {
614 pf_remove_src_node(sns[type]);
615 sns[type] = NULL;
616 }
617 if (pf_insert_src_node([type], r, type, af, saddr,
naddr,
618 rpool->kif))
619 return (1);
620 }
at line 608 new code reverts kif set by pfr_pool_get(). At line 617
(executed when sticky-address is set) the original code passes kif chosen be
pfr_pool_get(). You diff changes that behavior. So my question is simple:
is that intentional change?
thanks and
regards
sashan
Re: pf: route-to least-states
> On 23. Jul 2020, at 13:23, YASUOKA Masahiko wrote:
>
> The diff fixes 2 problems of "least-states":
>
> - states whose address is selected by sticky-address is not counted
> for the number of states.
> - interface is not selected properly if selected table entry specifies
> an interface.
>
> ok?
Good catch, diff looks good to me.
ok jung@
> Increase state counter for least-states when the address is selected
> by sticky-address. Also fix the problem that the interface which is
> specified by the selected table entry is not used properly.
>
> Index: sys/net/pf_lb.c
> ===
> RCS file: /disk/cvs/openbsd/src/sys/net/pf_lb.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 pf_lb.c
> --- sys/net/pf_lb.c 2 Jul 2019 09:04:53 - 1.64
> +++ sys/net/pf_lb.c 23 Jul 2020 11:06:05 -
> @@ -97,6 +97,8 @@ u_int64_tpf_hash(struct pf_addr *, st
> intpf_get_sport(struct pf_pdesc *, struct pf_rule *,
> struct pf_addr *, u_int16_t *, u_int16_t,
> u_int16_t, struct pf_src_node **);
> +int pf_map_addr_states_increase(sa_family_t,
> + struct pf_pool *, struct pf_addr *);
> intpf_get_transaddr_af(struct pf_rule *,
> struct pf_pdesc *, struct pf_src_node **);
> intpf_map_addr_sticky(sa_family_t, struct pf_rule *,
> @@ -319,6 +321,12 @@ pf_map_addr_sticky(sa_family_t af, struc
> sns[type] = NULL;
> return (-1);
> }
> +
> + if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
> + if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + return (-1);
> + }
> +
> if (!PF_AZERO(cached, af))
> pf_addrcpy(naddr, cached, af);
> if (pf_status.debug >= LOG_DEBUG) {
> @@ -345,6 +353,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
> struct pf_addr faddr;
> struct pf_addr *raddr = >addr.v.a.addr;
> struct pf_addr *rmask = >addr.v.a.mask;
> + struct pfi_kif *kif;
> u_int64_tstates;
> u_int16_tweight;
> u_int64_tload;
> @@ -539,6 +548,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
>
> states = rpool->states;
> weight = rpool->weight;
> + kif = rpool->kif;
>
> if ((rpool->addr.type == PF_ADDR_TABLE &&
> rpool->addr.p.tbl->pfrkt_refcntcost > 0) ||
> @@ -581,6 +591,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
> if (cload < load) {
> states = rpool->states;
> weight = rpool->weight;
> + kif = rpool->kif;
> load = cload;
>
> pf_addrcpy(naddr, >counter, af);
> @@ -591,29 +602,10 @@ pf_map_addr(sa_family_t af, struct pf_ru
> } while (pf_match_addr(1, , rmask, >counter, af) &&
> (states > 0));
>
> - if (rpool->addr.type == PF_ADDR_TABLE) {
> - if (pfr_states_increase(rpool->addr.p.tbl,
> - naddr, af) == -1) {
> - if (pf_status.debug >= LOG_DEBUG) {
> - log(LOG_DEBUG,"pf: pf_map_addr: "
> - "selected address ");
> - pf_print_host(naddr, 0, af);
> - addlog(". Failed to increase count!\n");
> - }
> - return (1);
> - }
> - } else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
> - if (pfr_states_increase(rpool->addr.p.dyn->pfid_kt,
> - naddr, af) == -1) {
> - if (pf_status.debug >= LOG_DEBUG) {
> - log(LOG_DEBUG, "pf: pf_map_addr: "
> - "selected address ");
> - pf_print_host(naddr, 0, af);
> - addlog(". Failed to increase count!\n");
> - }
> - return (1);
> - }
> - }
> + if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
> + return (1);
> + /* revert the kif which was set by pfr_pool_get() */
> + rpool->kif = kif;
> break;
> }
>
> @@ -642,6 +634,38 @@ pf_map_addr(sa_family_t af, struct pf_ru
> addlog("\n");
> }
>
> + return (0);
> +}
> +
> +int
> +pf_map_addr_states_increase(sa_family_t af, struct pf_pool *rpool,
> +struct pf_addr *naddr)
> +{
>
pf: route-to least-states
Hi,
The diff fixes 2 problems of "least-states":
- states whose address is selected by sticky-address is not counted
for the number of states.
- interface is not selected properly if selected table entry specifies
an interface.
ok?
Increase state counter for least-states when the address is selected
by sticky-address. Also fix the problem that the interface which is
specified by the selected table entry is not used properly.
Index: sys/net/pf_lb.c
===
RCS file: /disk/cvs/openbsd/src/sys/net/pf_lb.c,v
retrieving revision 1.64
diff -u -p -r1.64 pf_lb.c
--- sys/net/pf_lb.c 2 Jul 2019 09:04:53 - 1.64
+++ sys/net/pf_lb.c 23 Jul 2020 11:06:05 -
@@ -97,6 +97,8 @@ u_int64_t pf_hash(struct pf_addr *, st
int pf_get_sport(struct pf_pdesc *, struct pf_rule *,
struct pf_addr *, u_int16_t *, u_int16_t,
u_int16_t, struct pf_src_node **);
+int pf_map_addr_states_increase(sa_family_t,
+ struct pf_pool *, struct pf_addr *);
int pf_get_transaddr_af(struct pf_rule *,
struct pf_pdesc *, struct pf_src_node **);
int pf_map_addr_sticky(sa_family_t, struct pf_rule *,
@@ -319,6 +321,12 @@ pf_map_addr_sticky(sa_family_t af, struc
sns[type] = NULL;
return (-1);
}
+
+ if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_LEASTSTATES) {
+ if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ return (-1);
+ }
+
if (!PF_AZERO(cached, af))
pf_addrcpy(naddr, cached, af);
if (pf_status.debug >= LOG_DEBUG) {
@@ -345,6 +353,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
struct pf_addr faddr;
struct pf_addr *raddr = >addr.v.a.addr;
struct pf_addr *rmask = >addr.v.a.mask;
+ struct pfi_kif *kif;
u_int64_tstates;
u_int16_tweight;
u_int64_tload;
@@ -539,6 +548,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
states = rpool->states;
weight = rpool->weight;
+ kif = rpool->kif;
if ((rpool->addr.type == PF_ADDR_TABLE &&
rpool->addr.p.tbl->pfrkt_refcntcost > 0) ||
@@ -581,6 +591,7 @@ pf_map_addr(sa_family_t af, struct pf_ru
if (cload < load) {
states = rpool->states;
weight = rpool->weight;
+ kif = rpool->kif;
load = cload;
pf_addrcpy(naddr, >counter, af);
@@ -591,29 +602,10 @@ pf_map_addr(sa_family_t af, struct pf_ru
} while (pf_match_addr(1, , rmask, >counter, af) &&
(states > 0));
- if (rpool->addr.type == PF_ADDR_TABLE) {
- if (pfr_states_increase(rpool->addr.p.tbl,
- naddr, af) == -1) {
- if (pf_status.debug >= LOG_DEBUG) {
- log(LOG_DEBUG,"pf: pf_map_addr: "
- "selected address ");
- pf_print_host(naddr, 0, af);
- addlog(". Failed to increase count!\n");
- }
- return (1);
- }
- } else if (rpool->addr.type == PF_ADDR_DYNIFTL) {
- if (pfr_states_increase(rpool->addr.p.dyn->pfid_kt,
- naddr, af) == -1) {
- if (pf_status.debug >= LOG_DEBUG) {
- log(LOG_DEBUG, "pf: pf_map_addr: "
- "selected address ");
- pf_print_host(naddr, 0, af);
- addlog(". Failed to increase count!\n");
- }
- return (1);
- }
- }
+ if (pf_map_addr_states_increase(af, rpool, naddr) == -1)
+ return (1);
+ /* revert the kif which was set by pfr_pool_get() */
+ rpool->kif = kif;
break;
}
@@ -642,6 +634,38 @@ pf_map_addr(sa_family_t af, struct pf_ru
addlog("\n");
}
+ return (0);
+}
+
+int
+pf_map_addr_states_increase(sa_family_t af, struct pf_pool *rpool,
+struct pf_addr *naddr)
+{
+ if (rpool->addr.type == PF_ADDR_TABLE) {
+ if (pfr_states_increase(rpool->addr.p.tbl,
+ naddr, af) == -1) {
+
