On 9 May 2017 at 14:13, Patrick Wildt <[email protected]> wrote:
> 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
>
>
Please show me the code you're using to test this.
