> > > > sorry I was too brief. The snippet below comes from
> > > > pf_create_state() with your patch applied:
> > > >
> > > > 3560 for (i = 0; i < PF_SN_MAX; i++)
> > > > 3561 if (sns[i] != NULL) {
> > > > 3562 struct pf_sn_item *sni;
> > > > 3563
> > > > 3564 sni = pool_get(&pf_sn_item_pl,
> > > > PR_NOWAIT);
> > > > 3565 if (sni == NULL) {
> > > > 3566 REASON_SET(&reason,
> > > > PFRES_MEMORY);
> > > > 3567 goto csfailed;
> > > > 3568 }
> > > > 3569 sni->sn = sns[i];
> > > > 3570 sns[i] = NULL;
> > > > 3571 SLIST_INSERT_HEAD(&s->src_nodes, sni,
> > > > next);
> > > > 3572 sni->sn->states++;
> > > > 3573 }
> > > > 3574
> > > >
> > > > the point of my suggestion is to transfer ownership of source node from
> > > > local variable sns[] to state. so the check performed later in csfailed
> > > > at line 3617, will find NULL pointer in sns[i] field and won't attempt
> > > > to call pf_remove_src_node().
> > > >
> > > > 3610 csfailed:
> > > > 3611 if (s) {
> > > > 3612 pf_normalize_tcp_cleanup(s); /* safe even
> > > > w/o init */
> > > > 3613 pf_src_tree_remove_state(s);
> > > > 3614 }
> > > > 3615
> > > > 3616 for (i = 0; i < PF_SN_MAX; i++)
> > > > 3617 if (sns[i] != NULL)
> > > > 3618 pf_remove_src_node(sns[i]);
> > > > 3619
> > > >
> > > >
> > > > your patch updated by my suggestion is further below.
> > > >
> > >
> > > But that introduces a potential leak, as I said in my mail:
> > > we won't run pf_remove_src_node and cleanup source nodes that
> > > might have been created.
> > >
> > > sns[] is an array of pointers to independently tracked objects
> > > (inserted into the pf_src_tree).
> > >
> >
> > yes you are right, I was completely wrong. Still my only concern is what
> > happens in case we fail to allocate source node item (sni) for let's say
> > PF_SN_NAT type, while there is PF_SN_ROUTE type in sns[] to be processed.
> >
> > I'm referring to line numbers above. So we fail to allocate PF_SN_NAT
> > source node and taking goto at 3567, arriving to 3610.
> >
> > We do the right thing for state clean up (lines 3612, 3613). And now
> > arriving to for loop 3616, which will remove all source nodes from
> > sns[], including PF_SN_ROUTE. I completely agree we should do
> > pf_remove_src_node(sns[PF_SN_NAT]) as we took reference at 3572 for it.
>
> But we'll drop this "reference" in pf_src_tree_remove_state,
> then how will sns[PF_SN_NAT] and sns[PF_SN_ROUTE] be different?
I think I should take PF class again ;-) I've just realized there
is a test in pf_remove_src_node():
572 if (sn->states > 0 || sn->expire > time_uptime)
573 return;
so it will do the right thing. This is the piece I was missing.
>
> sns[] array was prepared for this state so if we can't insert
> the state, sns entries must be cleaned up. pf_remove_src_node
> checks the number of associated states and if source node should
> expire some time later.
yes, it seems more clear to me now.
>
> Speaking of PF_SN_ROUTE, pf_set_rt_ifp should be probably called
> before we insert the state for the very same reason, plus it
> should check the pf_map_addr return value and do the cleanup.
I don't feel entirely qualified now, to discuss the matter ;-), however
pf_set_rt_ifp() should indeed test return value of pf_map_addr(), In case of
failure the error should be thrown further up, so pf_create_state() can handle
it. Probably jumping to csfailed: should be sufficient.
regards
sasha
