On Fri, Feb 22, 2019 at 01:52:24AM +0100, Alexandr Nedvedicky wrote:
 
> so far so good. Now let's flush the rules from kernel:
> 
>     lumpy# ./pfctl -Fr
>     rules cleared
>     lumpy# ./pfctl -sr
>     lumpy#
> 
> However the underscore anchors are still there:
Any unreferenced anchor will remain, not just internal ones.  We have no
code/logic that clears them automatically.

>     lumpy# ./pfctl -vsA
>       _1
>       _1/_2
>     lumpy# 
That's a known issue, easily visible if you run the pfctl regress tests.
I've been pondering this with different attempts but haven't come up
with a good/safe one yet.

Perhaps we can teach pfctl something like `-F Anchors`, such that it
looks at the ruleset as well as all anchors and removes those which are
not referenced.

The benefits are that we do not require any further
starts-with-underscore quirks and it remains a wilful action done by
root.

> I could not figure out any existing way to remove them, hence I'm proposing
> small patch, which allows me to remove those 'underscore' anchors by doing
> this:
> 
>     lumpy# ./pfctl -a _1/_2 -Fr
>     rules cleared
>     lumpy# ./pfctl -a _1 -Fr
>     rules cleared
Did you look at happens with _1/_2 when you flush _1 directly?

> Does patch below make sense? Or are there some pitfalls I'm not aware of?
Given your example, using named anchors seems like the right approach.

> +             /*
> +              * we want enable administrator to flush anonymous anchors,
> +              * thus '_' should be allowed for '-Fr' only.  Also make sure
> +              * we fail in case of option combination as follows:
> +              *      pfctl -a _1 -Fr -f /some/rules.conf
> +              */
>               if (mode == O_RDWR && tblcmdopt == NULL &&
> +                 (clearopt == NULL || *clearopt != 'r' ||
> +                 rulesopt != NULL) &&
I'm not fond of adding an exception to what already is an exception.
The anchor handling code is tricky already and has big potential for
pitfalls and subtle bugs (as seen in the past).

>                   (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL))
>                       errx(1, "anchor names beginning with '_' cannot "
>                           "be modified from the command line");

That said, I think the internal semantics should stay purely internal to
avoid another can of worms.

Reply via email to