Wrong include order for ruleset-prepend

[somerandoname]

According to the documentation, custom includes with the ruleset-prepend option should be placed "before the fw4 table definition". However, this is currently not the case.

This seems to be caused by the rules being included after the fw4 table was initialized, which effectively makes this option behave nearly the same as ruleset-append:

table inet fw4
flush table inet fw4
{% if (fw4.check_flowtable()): %}
delete flowtable inet fw4 ft
{% endif %}
{% fw4.includes('ruleset-prepend') %}

Changing the order fixes this on my system:

{% fw4.includes('ruleset-prepend') %}
table inet fw4
flush table inet fw4
{% if (fw4.check_flowtable()): %}
delete flowtable inet fw4 ft
{% endif %}

https://github.com/openwrt/firewall4/blob/18fc0ead19faf06b8ce7ec5be84957278e942dfa/root/usr/share/firewall4/templates/ruleset.uc#L8C1-L13C38

[brada4]

Worth tagging autohor @jow-
11256ff

[brada4]

I am out of ideas how actual implementation could look, because unless you make include file flush table it would be repeatedly adding same chains+rules over and over again.

[somerandoname]

@brada4 This is presently already the case. Rulesets outside the inet fw4 context have to be "manually" flushed. As far as I am aware, the reordering shouldn't change anything in that regard.

[brada4]

Mind to make a PR? Actual documentation misses out that table-pre equals /etc/nftables.d/

[brada4]

Mail me if you want proper credit.

[jow-]

Under which circumstances is the current inclusion order a problem? Can we rule out that moving it will alter the behaviour of existing configs?

[brada4]

Description clearly says it is before table .. fw4, while at present it is next to table-prepend separated by some initial setup routines.
See ruleset-append at the very end of same file - it is outside table inet fw4.

[jow-]

Well ruleset-prepend is before and outside table inet fw4 { ... } while table-prepend is inside table inet fw4 { ... } so a different scope.

So asking again, under which circumstances is a preceding forward declaration and flushing of table inet fw4 a problem for an include that is solved by moving the include before the forward declaration?

To me it seems as if the proposed change will change the behavior to be less deterministic.

• before: table inet fw4 is guaranteed to exist but empty
• after: table inet fw4 might or might not exist and might or might not be empty

To be clear, I agree that the wiki-documented position of "At the very beginning, before the fw4 table definition" is misleading or even wrong, but I am not convinced that changing the code to match the wiki is the way to go, rather than fixing the wiki to match the established implementation.

[brada4]

flush table keeps blank table definition....
Yes, it is outside but right after....

[jow-]

Yeah sure, but the question remains - why is this a problem?

You cannot really alter the fw4 table without risking that rules generated later will be incompatible with it. And for working on unrelated tables or other entities, the forward declaration does not matter.

If the objective is to somehow get rid of the generated fw4 table or replace it with a table of the same name but a different family or whatever, then this can only be done from ruleset-post

[brada4]

I dunno, i know qosmate picks crumbles explicitly after fw4. Maybe somebody wants to wash traffic before.
ar present it is
table inet fw4{}
table netdev prepend {}
table egress append {}

[somerandoname]

Shouldn't someone who wants to edit the fw4 table be using the ruleset-post or table/chain options? The way I see it is if the wiki description is wrong, then it begs the question of what the purpose of the ruleset-prepend option is in the first place.

In my use case for example, the change would allow bridge packets to be indiscriminately blocked before they are needlessly evaluated by later definitions. It could I guess potentially break some installations, but only ones which already improperly use ruleset-prepend.

[jow-]

What's the actual things you try to inject? ruleset-prepend and ruleset-append are for doing things like additional tables, declaring variables. To inject new chains inside table inet fw4 the table-pre and table-post positions should be used.

I guess I see where the confusion stems from, "ruleset" here refers to the entire ruleset, not "the rules and chains within table fw4" and "table-pre"/"table-post" refers to "inside table fw4 at the top" and "inside table fw4 at the end" rather than "before table fw4" and "after table fw4".

Maybe it's worth adding new aliases:

• nftables-top aka. ruleset-pre
• table-top aka table-pre
• ...
• table-bottom aka table-post
• nftables-bottom aka ruleset-post

Alternatively maybe start/end or head/tail - I'm not a native english speaker so not sure what works best.

[somerandoname]

So this is the .nft file I'm trying to include:

add table bridge guest_isolation
delete table bridge guest_isolation

table bridge guest_isolation {
  chain forward {
    type filter hook forward priority filter; policy accept;
    ibrname "br-lan" vlan id 99 drop
  }
}

From my expectation ruleset-prepend should result in the table being created before the fw4 one, and therefore immediately drop all relevant packets before they are evaluated by later definitions.

[jow-]

According to my understanding (which might be wrong) - the declaration order of tables does not matter, the hook priority does influence which chain is entered first. So if you want to precede fw4's forward chain you would need to adjust the priority filter (equivalent to priority 0) to priority -1.

When multiple hooks use the same priority then the order of declaration of the hooks themselves should be followed, the (forward) declaration order of the tables themselves should not influence evaluation order.

But instead of guessing; is the proposed change by @brada4 (moving the {% fw4.includes('ruleset-prepend') %} line to the top) fixing your use case or do the rules still fail to match?

[brada4]

It is @somerandoname 's proposed change, i just wrote it down and drilled though tests

[somerandoname]

Hm, I see, so technically the order of the tables in the nftables ruleset doesn't matter, and ruleset-prepend correctly declares them before other definitions. Since the rules work in both cases on my system, I'm not so sure now if this cosmetic fix is worth it to potentially break some installations even if it more accurately reflects how the rules will be interpreted. From my side, this issue could be closed for now.

[brada4]

Only thing it can break if something inet (ip4 or ip6) drops some packet before fw4 in place of after. eg ct state new masquerade and ct state new drop are swapped.

[somerandoname]

Sorry, but what exactly determines the order of declaration? Is it literally the order of the rule declaration, or how they are ordered in the nft list ruleset print? Because intuitively, I would expect it to be the latter, as otherwise nftables would need to keep track of the declaration order in another place like in RAM. In that case however the proposed change would be more than only cosmetic.

Edit: It does keep track of it, and can be viewed with nft -a list ruleset, or maybe it doesn't as the handle count starts at 1 for each table.

[brada4]

It renumbers on interfaces created/deleted/down/up.
As the file is loaded first go the enclosing containers, then rules, not even in particularily sequential manner.

[somerandoname]

So to ensure order of operation, one shouldn't rely on the ruleset ordering and just use the priority setting. That clears things up for me, thanks.
As far as I'm concerned, the issue can be closed now.

[brada4]

The ordering should probably be adjusted to have one of each instead of 2 -append table places.
You can keep this open so you see how it resolves.

[jow-]

Closing this in favor to consistency and potential compatibility with existing setups. As discussed above, the change would not actually change the match order, just introduce a bit of uncertainty (since table fw4 might or might not exist while in the current implementation it is guaranteed to exist and be empty)