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;