On Tue, May 09, 2017 at 14:13 +0200, Patrick Wildt 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?
>
No, looks like I've misremembered. The assignment at
initialization has deceived me further. Can you please
convert both rmask and raddr to on stack variables at
the same time so there's no confusion in the future?
I'd also argue that we need to remove assignments that
are not needed (for pfr_pool_get), but we should do it
separately.
Please test this well. As I said this is very easy to
break.
