On Thu, May 28, 2015 at 01:17 +0200, Alexandr Nedvedicky wrote:
> Hello,
>
>
> On Wed, May 27, 2015 at 07:44:15PM +0200, Mike Belopuhov wrote:
> > On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
> > > Hello,
> > >
> > > > > - if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
> > > > > - pf_state_key_detach(s, PF_SK_STACK);
> > > > > - pf_state_key_detach(s, PF_SK_WIRE);
> > > >
> > > > This bug is not yours, but doing two pf_state_key_detach is wrong
> > > > and results in all kinds of protection fault fireworks. The right
> > > > thing is to do pf_detach_state that would handle the case where
> > > > PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
> > > good catch.
> > >
> > > >
> > > > In csfailed case we do pf_remove_src_node and then call
> > > > pf_src_tree_remove_state. With your diff this means that
> > > > pf_src_tree_remove_state will be dereferencing sni->sn that
> > > > might be pool_put by the pf_remove_src_node. Therefore their
> > > > order needs to be reversed.
> > >
> > > I see. Another option to fix it would be to do:
> > >
> > > sni->sn = sns[i];
> > > sns[i] = NULL;
> > >
> >
> > I'm not sure I follow. Where do you want to do it?
> > If it's when we pool_get sni's, then this would mean
> > we won't run pf_remove_src_node and cleanup source
> > nodes that might have been created.
> >
>
> 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).
