Hello,
On Tue, Jul 19, 2022 at 10:58:08PM +0200, Alexander Bluhm wrote:
> On Tue, Jul 19, 2022 at 07:18:54PM +0200, Moritz Buhl wrote:
> > A solution would be to move the allocation of the pf_anchor struct
> > into a pool. One final question would be regarding the size of the
> > hiwat or hardlimit. Any suggestions?
>
> 10 seems way to low. We want to limit resource abuse. But regular
> users should never hit this. Especially existing configurations
> should still work after upgrade.
>
> Perhaps 512 anchors. Noone shoul ever need more than 512 anchors :-)
512 seems reasonable. All other options which come to my mind
require far more complex change. for example we might consider
to tight up those uncommitted transactions to process. Then we
can release all uncommitted data in pfcloe() for such process.
>
> > Any other comments?
</snip>
>
> Please do not use PR_WAITOK in pool_init() unless you know what you
> are doing. I think it should work, but who knows. The other pf
> pools have 0 flags, just use that.
>
> Comment says rs_malloc is needed to compile pfctl. Have you tested
> that?
>
> Replacing M_WAITOK|M_CANFAIL|M_ZERO with PR_NOWAIT|PR_ZERO looks
> wrong. PR_WAITOK|PR_LIMITFAIL|PR_ZERO should do something equivalent.
>
> I am not sure if pf_find_anchor() should fail if the limit is hit.
> It uses only a temporary buffer. All calls to pf_find_ruleset()
> should be checked whether permanent failure, after the limit has
> been reached, is what we want. Or we keep the malloc there?
>
this seems to be a good point. I think we should keep malloc() there.
thanks and
regards
sashan
