On Tue, May 09, 2017 at 10:36 +0200, Patrick Wildt wrote:
> On Mon, May 08, 2017 at 02:56:55PM +0200, Patrick Wildt wrote:
> > @@ -2307,9 +2326,9 @@ pfr_pool_get(struct pf_pool *rpool, struct pf_addr
> > **raddr,
> > rpool->curweight = kt->pfrkt_maxweight;
> > }
> >
> > - pfr_prepare_network(&pfr_mask, af, ke->pfrke_net);
> > + pfr_prepare_network(&mask, af, ke->pfrke_net);
> > *raddr = SUNION2PF(&ke->pfrke_sa, af);
> > - *rmask = SUNION2PF(&pfr_mask, af);
> > + *rmask = SUNION2PF(&mask, af);
> >
> > if (use_counter && !PF_AZERO(counter, af)) {
> > /* is supplied address within block? */
>
> Before committing I realised that this actually returns the pointer
> instead of the contents, which would mean that we would now return
> a pointer to the callee's stack.
>
> I think we should change pfr_pool_get() so that it's the caller's
> responsibility to pass a structure where the rmask can fit into. Then
> we should always work with that structure and copy the elements instead
> of changing rmask by setting it to a different pointer.
>
> Maybe we should do the same for raddr.
>
> Opinions?
>
You have to take extreme care with this fragile code.
raddr is the current address of the pool. It gets modified
by the pfr_pool_get each time you call it, the same with rmask.
Try to round-robin through a table (all 22 entries) like this:
{ 1.0.0.0/30 2.0.0.0/29 3.0.0.0/28} and see how raddr and rmask
change.
Right now I believe this diff is incorrect as you're not putting
back your updated rmask value back into the pool where you took
it so that the next iteration starts up where you left.
Getting rid of the global value is a good thing though.
> diff --git a/sys/net/pf_lb.c b/sys/net/pf_lb.c
> index 74acc538b8f..af57272c4dc 100644
> --- a/sys/net/pf_lb.c
> +++ b/sys/net/pf_lb.c
> @@ -339,7 +339,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> unsigned char hash[16];
> struct pf_addr faddr;
> struct pf_addr *raddr = &rpool->addr.v.a.addr;
> - struct pf_addr *rmask = &rpool->addr.v.a.mask;
> + struct pf_addr rmask = rpool->addr.v.a.mask;
> u_int64_t states;
> u_int16_t weight;
> u_int64_t load;
> @@ -361,7 +361,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> !PF_POOL_DYNTYPE(rpool->opts))
> return (1);
> raddr = &rpool->addr.p.dyn->pfid_addr4;
> - rmask = &rpool->addr.p.dyn->pfid_mask4;
> + rmask = rpool->addr.p.dyn->pfid_mask4;
> break;
> #ifdef INET6
> case AF_INET6:
> @@ -369,7 +369,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> !PF_POOL_DYNTYPE(rpool->opts))
> return (1);
> raddr = &rpool->addr.p.dyn->pfid_addr6;
> - rmask = &rpool->addr.p.dyn->pfid_mask6;
> + rmask = rpool->addr.p.dyn->pfid_mask6;
> break;
> #endif /* INET6 */
> default:
> @@ -380,7 +380,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> return (1); /* unsupported */
> } else {
> raddr = &rpool->addr.v.a.addr;
> - rmask = &rpool->addr.v.a.mask;
> + rmask = rpool->addr.v.a.mask;
> }
>
> switch (rpool->opts & PF_POOL_TYPEMASK) {
> @@ -388,7 +388,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> PF_ACPY(naddr, raddr, af);
> break;
> case PF_POOL_BITMASK:
> - PF_POOLMASK(naddr, raddr, rmask, saddr, af);
> + PF_POOLMASK(naddr, raddr, &rmask, saddr, af);
> break;
> case PF_POOL_RANDOM:
> if (rpool->addr.type == PF_ADDR_TABLE) {
> @@ -418,31 +418,31 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> break;
> #ifdef INET6
> case AF_INET6:
> - if (rmask->addr32[3] != 0xffffffff)
> + if (rmask.addr32[3] != 0xffffffff)
> rpool->counter.addr32[3] = arc4random();
> else
> break;
> - if (rmask->addr32[2] != 0xffffffff)
> + if (rmask.addr32[2] != 0xffffffff)
> rpool->counter.addr32[2] = arc4random();
> else
> break;
> - if (rmask->addr32[1] != 0xffffffff)
> + if (rmask.addr32[1] != 0xffffffff)
> rpool->counter.addr32[1] = arc4random();
> else
> break;
> - if (rmask->addr32[0] != 0xffffffff)
> + if (rmask.addr32[0] != 0xffffffff)
> rpool->counter.addr32[0] = arc4random();
> break;
> #endif /* INET6 */
> default:
> unhandled_af(af);
> }
> - PF_POOLMASK(naddr, raddr, rmask, &rpool->counter, af);
> + PF_POOLMASK(naddr, raddr, &rmask, &rpool->counter, af);
> PF_ACPY(init_addr, naddr, af);
>
> } else {
> PF_AINC(&rpool->counter, af);
> - PF_POOLMASK(naddr, raddr, rmask, &rpool->counter, af);
> + PF_POOLMASK(naddr, raddr, &rmask, &rpool->counter, af);
> }
> break;
> case PF_POOL_SRCHASH:
> @@ -469,7 +469,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> return (1);
> PF_ACPY(naddr, &rpool->counter, af);
> } else {
> - PF_POOLMASK(naddr, raddr, rmask,
> + PF_POOLMASK(naddr, raddr, &rmask,
> (struct pf_addr *)&hash, af);
> }
> break;
> @@ -485,7 +485,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> if (pfr_pool_get(rpool, &raddr, &rmask, af))
> return (1);
> }
> - } else if (pf_match_addr(0, raddr, rmask, &rpool->counter, af))
> + } else if (pf_match_addr(0, raddr, &rmask, &rpool->counter, af))
> return (1);
>
> /* iterate over table if it contains entries which are weighted
> */
> @@ -527,7 +527,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> if (pfr_pool_get(rpool, &raddr, &rmask, af))
> return (1);
> }
> - } else if (pf_match_addr(0, raddr, rmask, &rpool->counter, af))
> + } else if (pf_match_addr(0, raddr, &rmask, &rpool->counter, af))
> return (1);
>
> states = rpool->states;
> @@ -557,7 +557,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> rpool->addr.type == PF_ADDR_DYNIFTL) {
> if (pfr_pool_get(rpool, &raddr, &rmask, af))
> return (1);
> - } else if (pf_match_addr(0, raddr, rmask,
> + } else if (pf_match_addr(0, raddr, &rmask,
> &rpool->counter, af))
> return (1);
>
> @@ -581,7 +581,7 @@ pf_map_addr(sa_family_t af, struct pf_rule *r, struct
> pf_addr *saddr,
> PF_AZERO(init_addr, af))
> PF_ACPY(init_addr, naddr, af);
> }
> - } while (pf_match_addr(1, &faddr, rmask, &rpool->counter, af) &&
> + } while (pf_match_addr(1, &faddr, &rmask, &rpool->counter, af)
> &&
> (states > 0));
>
> if (rpool->addr.type == PF_ADDR_TABLE) {
> diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
> index 7666ec7013c..f23739e8565 100644
> --- a/sys/net/pf_table.c
> +++ b/sys/net/pf_table.c
> @@ -140,7 +140,6 @@ struct pfr_walktree {
> struct pool pfr_ktable_pl;
> struct pool pfr_kentry_pl[PFRKE_MAX];
> struct pool pfr_kcounters_pl;
> -union sockaddr_union pfr_mask;
> struct pf_addr pfr_ffaddr;
>
> int pfr_gcd(int, int);
> @@ -2252,7 +2251,7 @@ pfr_islinklocal(sa_family_t af, struct pf_addr *addr)
>
> int
> pfr_pool_get(struct pf_pool *rpool, struct pf_addr **raddr,
> - struct pf_addr **rmask, sa_family_t af)
> + struct pf_addr *rmask, sa_family_t af)
> {
> struct pfr_ktable *kt;
> struct pfr_kentry *ke, *ke2;
> @@ -2327,13 +2326,13 @@ pfr_pool_get(struct pf_pool *rpool, struct pf_addr
> **raddr,
> rpool->curweight = kt->pfrkt_maxweight;
> }
>
> - pfr_prepare_network(&pfr_mask, af, ke->pfrke_net);
> + pfr_prepare_network(&mask, af, ke->pfrke_net);
> *raddr = SUNION2PF(&ke->pfrke_sa, af);
> - *rmask = SUNION2PF(&pfr_mask, af);
> + *rmask = *SUNION2PF(&mask, af);
>
> if (use_counter && !PF_AZERO(counter, af)) {
> /* is supplied address within block? */
> - if (!PF_MATCHA(0, *raddr, *rmask, counter, af)) {
> + if (!PF_MATCHA(0, *raddr, rmask, counter, af)) {
> /* no, go to next block in table */
> idx++;
> use_counter = 0;
> @@ -2417,7 +2416,7 @@ _next_entry:
> pfr_prepare_network(&mask, AF_INET, ke2->pfrke_net);
> PF_POOLMASK(addr, addr, SUNION2PF(&mask, af), &pfr_ffaddr, af);
> PF_AINC(addr, af);
> - if (!PF_MATCHA(0, *raddr, *rmask, addr, af)) {
> + if (!PF_MATCHA(0, *raddr, rmask, addr, af)) {
> /* ok, we reached the end of our main block */
> /* go to next block in table */
> idx++;
> diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
> index 981d778e37a..982437b64ca 100644
> --- a/sys/net/pfvar.h
> +++ b/sys/net/pfvar.h
> @@ -1725,7 +1725,7 @@ int pfr_match_addr(struct pfr_ktable *, struct
> pf_addr *, sa_family_t);
> void pfr_update_stats(struct pfr_ktable *, struct pf_addr *,
> struct pf_pdesc *, int, int);
> int pfr_pool_get(struct pf_pool *, struct pf_addr **,
> - struct pf_addr **, sa_family_t);
> + struct pf_addr *, sa_family_t);
> int pfr_states_increase(struct pfr_ktable *, struct pf_addr *, int);
> int pfr_states_decrease(struct pfr_ktable *, struct pf_addr *, int);
> struct pfr_kentry *
>
