Hello,
On Thu, May 21, 2015 at 07:43:51PM +0200, Mike Belopuhov wrote:
> 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.
perhaps swapping the for loop block with pf_state_insert() will work.
We can then bail out using goto csfailed then (see patch below...)
> we should do a pf_state_insert afterwards? This might simplify
> locking later on.
speaking of locking... everything is more complicated, however
in this particular case it makes things easier for sure. basically in
our current SMP model, once we insert state to table, the only way
to remove it is to leave the job on garbage collector thread...
>
> > 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.
we use a compile time switch to enable SMP code (-D_PF_SMP_). this is
something I'd like to keep around anyway. I'll start on syncing up our changes
with CURRENT. It will take couple of weeks (ENOCYCLES as usually). Once
I'll have something to show I'll share it and you'll see what can be done
with it.
>
> Thanks for your quality diffs btw, help is always much appreciated.
>
I'm just trying to be useful. It's pleasure to work with PF sources.
thanks and
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 19:18:04 -0000
@@ -3556,15 +3556,6 @@
goto csfailed;
}
- 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);
- *sks = *skw = NULL;
- REASON_SET(&reason, PFRES_STATEINS);
- goto csfailed;
- } else
- *sm = s;
-
/* attach src nodes late, otherwise cleanup on error nontrivial */
for (i = 0; i < PF_SN_MAX; i++)
if (sns[i] != NULL) {
@@ -3572,16 +3563,21 @@
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);
- return (PF_DROP);
+ goto csfailed;
}
sni->sn = sns[i];
SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
sni->sn->states++;
}
+
+ 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);
+ *sks = *skw = NULL;
+ REASON_SET(&reason, PFRES_STATEINS);
+ goto csfailed;
+ } else
+ *sm = s;
pf_set_rt_ifp(s, pd->src); /* needs s->state_key set */
if (tag > 0) {
