Hello,
looks good to me, though I still need better explanation for PF_FRAG_STALE.
The current comment seems bit misleading to me.
</snip>
> #define PFTM_TS_DIFF_VAL 30 /* Allowed TS diff */
>
> +#define PF_FRAG_STALE 200 /* Limit fragments per second per
> connection */
> +
> enum { PF_NOPFROUTE, PF_ROUTETO, PF_DUPTO, PF_REPLYTO };
I did not get how we arrived to 'Limit fragments per second per connection.'
I think it should read as follows:
Limit fragments per minute per connection
But still to make it technically correct it should really read as:
Limit fragments per PFTM_FRAG timeout per connection
Or am I missing something? my arguments above are based on code found
here in your patch:
> @@ -282,6 +286,24 @@ pf_find_fragment(struct pf_frnode *key,
> frag = RB_FIND(pf_frag_tree, &frnode->fn_tree, &idkey);
> if (frag == NULL)
> return (NULL);
> + /*
> + * Limit the number of fragments we accept for each (proto,src,dst,af)
> + * combination (aka pf_frnode), so we can deal better with a high rate
> + * of fragments.
> + * Store the current generation for each pf_frnode in fn_gen and on
> + * lookup discard 'stale' fragments (pf_fragment, based on the fr_gen
> + * member). Instead of adding another button interpret the pf fragment
> + * timeout in multiples of 200 fragments. This way the default of 60s
> + * means: pf_fragment objects older than 60*200 = 18,000 generations
> + * are considered stale.
> + */
> + stale = pf_default_rule.timeout[PFTM_FRAG] * PF_FRAG_STALE;
> + if ((frnode->fn_gen - frag->fr_gen) >= stale) {
> + DPFPRINTF(LOG_NOTICE, "stale fragment %d(%p), gen %u, num %u",
> + frag->fr_id, frag, frag->fr_gen, frnode->fn_fragments);
> + pf_free_fragment(frag);
> + return (NULL);
> + }
> TAILQ_REMOVE(&pf_fragqueue, frag, frag_next);
> TAILQ_INSERT_HEAD(&pf_fragqueue, frag, frag_next);
thanks for clarification...
regards
sasha
