On 28 May 2015 at 21:22, Alexandr Nedvedicky
<alexandr.nedvedi...@oracle.com> wrote:
>> > > > 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.
>
Good! At least I wasn't blind this time! (:
>>
>> 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 ;-)
Hey, don't worry about it. Half of the people reading this have
zero clue as to what the hell are we talking about.
> 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.
>
You can't just jump to csfailed unless you do a pf_set_rt_ifp
before the pf_state_insert, but then it needs an attached key
to only get it's address family.
> regards
> sasha
