On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
> Hello,
>
Hi,
> snippet below comes from pf_create_state():
>
> 3559 if (pf_state_insert(BOUND_IFACE(r, pd->kif), skw, sks, s)) {
> 3560 pf_state_key_detach(s, PF_SK_STACK);
> 3561 pf_state_key_detach(s, PF_SK_WIRE);
> 3562 *sks = *skw = NULL;
> 3563 REASON_SET(&reason, PFRES_STATEINS);
> 3564 goto csfailed;
> 3565 } else
> 3566 *sm = s;
> 3567
> 3568 /* attach src nodes late, otherwise cleanup on error
> nontrivial */
> 3569 for (i = 0; i < PF_SN_MAX; i++)
> 3570 if (sns[i] != NULL) {
> 3571 struct pf_sn_item *sni;
> 3572
> 3573 sni = pool_get(&pf_sn_item_pl, PR_NOWAIT);
> 3574 if (sni == NULL) {
> 3575 REASON_SET(&reason, PFRES_MEMORY);
> 3576 pf_src_tree_remove_state(s);
> 3577 STATE_DEC_COUNTERS(s);
> 3578 pool_put(&pf_state_pl, s);
> 3579 return (PF_DROP);
> 3580 }
> 3581 sni->sn = sns[i];
> 3582 SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
> 3583 sni->sn->states++;
> 3584 }
>
> at line 3559 PF inserts state to table. If insert operation succeeds, then
> state can no longer be killed using simple pool_put() as it currently
> happens at line 3578. I think PF should go for pf_unlink_state() instead.
>
> patch below should kill the bug.
>
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the "cleanup on error nontrivial". Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
> also one more off-topic question:
>
> would you be interested in SMP patch for PF?
> it basically introduces fine locking and reference counting
> on PF data objects, so firewall can handle more packets at
> single instance of time.
>
We would definitely be interested in such a diff, but at the moment
there's no use for it as pf_test is called directly from the IP stack
and hence IP stack needs to support parallel execution first. This
doesn't mean that we can't start integrating bits and pieces, esp.
if they're presented as separate patches.
Thanks for your quality diffs btw, help is always much appreciated.
Cheers,
Mike
> regards
> sasha
>
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.913
> diff -u -r1.913 pf.c
> --- pf.c 11 May 2015 12:22:14 -0000 1.913
> +++ pf.c 21 May 2015 15:20:01 -0000
> @@ -3573,9 +3573,7 @@
> sni = pool_get(&pf_sn_item_pl, PR_NOWAIT);
> if (sni == NULL) {
> REASON_SET(&reason, PFRES_MEMORY);
> - pf_src_tree_remove_state(s);
> - STATE_DEC_COUNTERS(s);
> - pool_put(&pf_state_pl, s);
> + pf_unlink_state(s);
> return (PF_DROP);
> }
> sni->sn = sns[i];
>
