On Tue, May 09, 2017 at 01:55:18PM +0200, Mike Belopuhov wrote:
> 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.
I don't see where rmask is put back into the pool? pfr_pool_get
only replaced the pointer pointing to the rmask value, so the
value in the pool never changed. Instead the rmask pointer in
pf_map_addr() simply pointed to the global variable instead of
the pool value.
pf_lb.c:342:
struct pf_addr *rmask = rpool->addr.v.a.mask;
pf_lb.c:401:
pfr_pool_get(rpool, &raddr, &rmask, af)
pf_table.c:2331:
*rmask = SUNION2PF(&mask, af);
Am I missing something?
Patrick
>
> > 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 *
> >
