Hi,
Thank you for your review.
On Fri, 24 Jul 2020 01:25:42 +0200
Alexandr Nedvedicky <alexandr.nedvedi...@oracle.com> 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, &faddr, rmask, &rpool->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(&sns[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;
