New options for ipfw - record-state, set-limit and skip-immediate-action - for simpler rulesets

[lev]

Summary

This diff adds new options (pseudo-actions) to ipfw:

1. record-state
2. set-limit
3. skip-immediate-action (aliased as skip-action)

These actions allows de-couple three side-effects of matched rule:

1. Action itself when rule is matched in standard top-to-down rule search.
2. Creation of dynamic state or limit state.
3. Checking of existing states and/or limits.

Now these side-effects are tightly coupled and only possible combinations are only:

1. Checking of existing state (before conditions), state creation, immediate action (keep-state and limit options).
2. Checking of existing state (check-state rule).

New options

With new options it is possible:

• Rules, which create state but not check state implicitly.
• Rules, which create state, but don't run action till dynamic rule is matched.
• Rules, which check and create state, but don't run action till dynamic rule is matched.
• Rules, which does nothing (but ipfw(8) will warn about these).

With this fine control of side-effects it is possible to write complex rulesets (like mixing NAT with statefulness and/or connection limits) more clearly.

This patch add one new opcode to kernel ipfw2 engine (O_SKIP_ACTION), change logic of ipfw(8) to allow it skip emission of O_STATE_PROBE opcode and adds dynamic state update to codepath which try to install new dynamic rule.

[julian]

I can see you speak a language similar to russian or polish . :-)

In english every noun has to be preceded by a specifier as to whether it is a specific or general object being described..

If it is a specific one we are already discussing or have in our hand, or can point at, then add 'the' or 'this' or 'that'. If it is any object, not just one we have in our hand or are already discussing, then add 'a' or 'an' (or sometimes "any").

sbin/ipfw/ipfw.8

196 ↗

(On Diff #3634)

not only those matched by the rule.

200 ↗

(On Diff #3634)

s/not/no/

1609 ↗

(On Diff #3634)

However, this option doesn't imply an implicit check-state

1614 ↗

(On Diff #3634)

remove 'exactly'

1616 ↗

(On Diff #3634)

but does not have an implicit check-state.

1620 ↗

(On Diff #3634)

A rule with...

1625 ↗

(On Diff #3634)

as the dynamic rule, created but ignored on match, will..

1627 ↗

(On Diff #3634)

Rules with both .....create a dynamic rule and continue with the next rule, but when the state is checked and the dynamic rule matches, the action will be performed as usual.

sys/netpfil/ipfw/ip_fw2.c

2132 ↗

(On Diff #3634)

... not a real action either ...

right after the O_KEEP_STATE

... so the real action...

...only if the rule is checked... from the state table... ... starts from the true...

[julian]

while the 'skip-immediate' is useful, I was thinking that record-state would imply that.
skip immediate is only useful with state storage, so I don't see a reason to make it generally available.

[lev]

In D1776#6, @julian wrote:

while the 'skip-immediate' is useful, I was thinking that record-state would imply that.

It was first idea. But after some thoughts, I decided, that it is not good enough.

skip immediate is only useful with state storage, so I don't see a reason to make it generally available.

One word: orthogonality of API (ok, three words).

Reason is simple: now we add 3 new options. To cover all possible combinations of side-effects without separate "skip-action" we need much more. Like this:

• keep-state-skip-action
• limit-skip-action
• record-state
• record-state-skip-action
• set-limit
• set-limit-skip-action
  
  Oh, MY!
  
  Or, if we add only set-limit and record-state with implicit (AGAIN THIS UGLY WORD! implicit side-effect!) skip-action here could be situations when rules should be duplicated: one with record-state and second without, to perform action right now.
  
  I think, there is THREE DIFFERENT things:
• Creation/updating of state, with normal condition checks on packet.
• Checking packet against state table, updating state, performing action (if appropriate state is found, of course).
• Performing action WHEN state is created (not checked!)
  
  These side-effects are independent and should be controlled by independent and explicit options.
  
  Only drawback of separate skip-action is ability to create "no-op" rule. But I've added check to ipfw(8) to warn about such rules.
  
  TL;DR: I think, that implicit side-effects is bad, bad design. I want to add orthogonal set of tools with only explicit effects.

[lev]

In D1776#6, @julian wrote:

while the 'skip-immediate' is useful, I was thinking that record-state would imply that.
skip immediate is only useful with state storage, so I don't see a reason to make it generally available.

And same argument in other word: we remove one implicit effect of keep-state and limit (checking of state) and add another one (skipping of action)!

R U SERIUS?

[lev]

New diff with spelling mistakes fixed. Thanks, Julian!

[julian]

missed one... or, rather, I got it wrong the first time.

sbin/ipfw/ipfw.8

1631 ↗

(On Diff #3642)

create a dynamic rule and continue with the next rule without actually performing the action part of this rule. When the rule is later activated via the state table, the action is performed as usual.

[julian]

R U SERIUS?

kinda..
I sort of was thinking the 'record-state' command would do nothing except record state.
in other words it would have an impicit "don't do it now", and it would not have an implicit check-state.

[lev]

I sort of was thinking the 'record-state' command would do nothing except record state.
in other words it would have an impicit "don't do it now", and it would not have an implicit check-state.

It was my first intention because it solves task I had at hands. But after that I've started to explain idea to somebody at ipfw@ list and found that my first approach is not much better than current one, because main problem is this "implicit".

[lev]

Fix grammar in man page.
Migrate to latest revision.

[julian]

this is a superset of what I would have done.. thanks for fixing the grammar.

[melifaro]

First of all, I agree that current dynamic state stuff is not ideal and we might need a better way to specify how we should handle dynamic rules.
However, I can't see an obvious improvement from these opcodes/changes.

Quoting original topic:
With new options it is possible:

Rules, which create state but not check state implicitly.

We actually _need_ to check state prior to creation since we have to be sure we don't create a duplicate state. If we're talking about not performing check-state for ALL traffic than we can shift O_CHECK_STATE opcode to the end of the rule, instead of starting with it. This can be done in ipfw(8).

Rules, which create state, but don't run action till dynamic rule is matched.
Rules, which check and create state, but don't run action till dynamic rule is matched.

Can you provide examples where it can be used?
We'd better collect some real-world examples and use them as a starting point to determine a better syntax/optode stuff.

[julian]

AS I said before, I can see a use of much of this but I think I would still settle for a version of "keep-state" that sets the state without checking it (except to look for duplicates). If you are being blocked by the nefarious melifaro then maybe a subset that just implements "set-state" would surfice. .

set-state would create a rule and would do the action but would not first do an implicit check-state.
so packets that did not match but would later match a state rule would not trigger at that point.

[lev]

Update diff to latest HEAD version.
Add example for new options to man page.

[ae]

After some thoughts, I think your patch can be simplified to only modify ipfw(8).
You can use F_NOT flag to distinguish record-state from keep-state. The rule with such opcode will be not prepended with O_PROBE_STATE.
So, when it will be matched it will call ipfw_install_state(), but after a state creation the rule action will not be applied due to presence of F_NOT flag.
Also 'set-limit' can be implemented in the same way.

[ae]

Something like this (applied to D6674): https://people.freebsd.org/~ae/ipfw.diff

[lev]

In D1776#143557, @ae wrote:

After some thoughts, I think your patch can be simplified to only modify ipfw(8).
You can use F_NOT flag to distinguish record-state from keep-state. The rule with such opcode will be not prepended with O_PROBE_STATE.
So, when it will be matched it will call ipfw_install_state(), but after a state creation the rule action will not be applied due to presence of F_NOT flag.
Also 'set-limit' can be implemented in the same way.

Difference between record-state and keep-state is not about immediate action execution, but about implicit check-state. record-state could be encoded as F_NOT and keep-state, but defer-action needs kernel changes anyway

Your patch omit defer-action and it is essential part of my patch, IMHO. I need to create allow state (maybe, with limiting) and AFTER that pass packet to nat. I don't see how it could be done without defer-action.

[ae]

Difference between record-state and keep-state is not about immediate action execution, but about implicit check-state.

Yes, this is implemented in user level and you can see this in the patch.

record-state could be encoded as F_NOT and keep-state, but defer-action needs kernel changes anyway
Your patch omit defer-action and it is essential part of my patch, IMHO.

This strange action isn't needed. 'defer-action' is implemented via F_NOT flag.

I need to create allow state (maybe, with limiting) and AFTER that pass packet to nat. I don't see how it could be done without defer-action.

Look how it works:
'record-state' is last opcode (O_KEEP_STATE with F_NOT) in the rule and there is no implicit 'check-state'.
So, when packet is matched by this opcode, ipfw_install_state() will create dynamic rule with action that has this rule.
But 'defer-action' effect will be achieved because opcode has F_NOT flag. Look at the end of swich(), the 'match' variable will be set in the O_KEEP_STATE opcode, then 'match' variable will be inversed to zero due to the F_NOT flag. Now we get 'defer-action' effect and the search continues with the next rule. Now you can do NAT and etc.

[lev]

Look how it works:
'record-state' is last opcode (O_KEEP_STATE with F_NOT) in the rule and there is no implicit 'check-state'.
So, when packet is matched by this opcode, ipfw_install_state() will create dynamic rule with action that has this rule.
But 'defer-action' effect will be achieved because opcode has F_NOT flag. Look at the end of swich(), the 'match' variable will be set in the O_KEEP_STATE opcode, then 'match' variable will be inversed to zero due to the F_NOT flag. Now we get 'defer-action' effect and the search continues with the next rule. Now you can do NAT and etc.

Ok, and how implement record-state with action triggering in this case?
Maybe, I don't understand something, but looks like it is linked effects AGAIN. keep-state means create-or-check-state-and-execute-command-all-in-one and record-state means create-or-update-state-and-skip-command?

Again, ipfw is very low-level (~macro assembler), and for low-level language it is better to have orthogonal "opcodes". Now we have some arbitrary-fused options like keep-state and you suggest to add more of these, when I suggest add one-option-one-meaning ones!

Sorry, but I don't like this at all. keep-state is ugly, ugly monster, and your variant of record-state (O_KEEP_STATE with F_NOT) is not better AT ALL, IMHO. It is ugly hack again, not well-defined simple option with one and only one meaning and action, which could be combined with other ones.

[ae]

In D1776#144161, @lev wrote:

Ok, and how implement record-state with action triggering in this case?

record-state with action triggering implemented as two rules:

# ipfw add allow tcp from me to any out record-state
# ipfw add check-state

Maybe, I don't understand something, but looks like it is linked effects AGAIN. keep-state means create-or-check-state-and-execute-command-all-in-one and record-state means create-or-update-state-and-skip-command?

With my patch keep-state semantic is saved, nothing will be break with this patch.
keep-state means check and apply action or create state and apply action.
There is no update state meaning in your patch, you are wrong with this. record-state means create state.
check-state means check and update state.

Again, ipfw is very low-level (~macro assembler), and for low-level language it is better to have orthogonal "opcodes". Now we have some arbitrary-fused options like keep-state and you suggest to add more of these, when I suggest add one-option-one-meaning ones!

Add new opcodes is not always appreciated, because this extends ABI. And we already have several opcodes that uses F_NOT flag.

Sorry, but I don't like this at all. keep-state is ugly, ugly monster, and your variant of record-state (O_KEEP_STATE with F_NOT) is not better AT ALL, IMHO. It is ugly hack again, not well-defined simple option with one and only one meaning and action, which could be combined with other ones.

I don't like defer-action :)

[lev]

In D1776#144162, @ae wrote:

In D1776#144161, @lev wrote:

Ok, and how implement record-state with action triggering in this case?

record-state with action triggering implemented as two rules:

# ipfw add allow tcp from me to any out record-state
# ipfw add check-state

Ok.

With my patch keep-state semantic is saved, nothing will be break with this patch.

Same with my patch. My patch doesn't touch keep-state at all and is fully backward compatible!

keep-state means check and apply action or create state and apply action.
There is no update state meaning in your patch, you are wrong with this. record-state means create state.
check-state means check and update state.

My patch replace ipfw_install_state() with ipfw_install_or_update_state(). It is necessary, otherwise there are situations where dynamic states are dropped too fast, I had problems with long-living but not very active connections without this change.

Add new opcodes is not always appreciated, because this extends ABI. And we already have several opcodes that uses F_NOT flag.

But it is fully backward-compatible and new /sbin/ipfw with old kernel... Ok, it is possible, but, IMHO it is bad idea anyway. And it will work, too, if here is no defer-action in ruleset.

I don't like defer-action :)

Fair enough :)

[julian]

ok so I see the points of both of you.. hmm that doesn't sound very english.. let's try again..

I see the points both of you are making.

let's try address some of them (all mixed up):
1/ backwards compatibility. The capacity to process the opcodes generated by an old ipfw(8) when it sees 'keep-state' need to remain in place, and do the same things. We all agree with that. that compatibility doesn't really need to be in place for more that a major release..
2/ new functionality could be extending the current opcodes, or adding new ones (no values added to these statements, just facts)
3/ We see from the intel architecture "lack of beauty is no bar to success" :-) By this I mean that hiding the added features of the current keep-state CISC instruction with with modifiers is not a bad idea just on its own.
4/ but having a clean architecture does make it easier to understand. F_NOT is not an obvious flag to mean "but don't check state".
5/ if 'record state' simply saves the state without doing it, then a following check-state rule can be made to actually DO the action
6/ doing (5) works but uses up an extra evaluation cycle when we already knew we matched,

So where do I stand on this?
a) my prime requirement is a "record-and-do" operation. i.e keep-state without an implicit check-state. A record-and-defer has not been a big requirement for me yet, possibly because I didn't think it likely to be feasible.
b) having named state tables makes this a bit less important in one way but a bit more important in others.
c) having a 'deferred' action is only useful with keep-state and record-state. otherwise it is always "do nothing" because by definition, only state saving ops have the option for 'later'. Having a deferred action would make sense as a flag on record-state. That would mean using a different opcode for the implicit check-state. is this already a separate implicit command or is it hardcoded into the keep-state code?

[lev]

In D1776#144733, @julian wrote:

ok so I see the points of both of you.. hmm that doesn't sound very english.. let's try again..

I see the points both of you are making.

let's try address some of them (all mixed up):
1/ backwards compatibility. The capacity to process the opcodes generated by an old ipfw(8) when it sees 'keep-state' need to remain in place, and do the same things. We all agree with that. that compatibility doesn't really need to be in place for more that a major release..

It works for both of patches (my with new opcode and ae@'s with F_NOT).

b) having named state tables makes this a bit less important in one way but a bit more important in others.

IMHO, named states is great and useful feature, which doesn't conflict with this my feature. Patches are conflicting, for sure, as they touches same code, but it is not feature-conflict or ideological conflict, it is only merge conflict, which is mere technical difficulty.

c) having a 'deferred' action is only useful with keep-state and record-state. otherwise it is always "do nothing" because by definition, only state saving ops have the option for 'later'. Having a deferred action would make sense as a flag on record-state. That would mean using a different opcode for the implicit check-state. is this already a separate implicit command or is it hardcoded into the keep-state code?

Yes, it is why ipfw(8) warns user when here is deferred action without state creation flag in my patch. check-state is separate opcode now (and it is not touched by my patch), which is inserted into "compiled" rule by ipfw(8) when it sees keep-state. My patch doesn't change this, it only adds new compilation rule for record-state (like keep-state but without check-state opcode) and adds new opcode of its own for defer-action.

To be honest, I don't have time and energy to force-push my solution with new opcode and orthogonal options. I don't like ae@ solution, as it is CISC again, and I don't like record-state / check-state pair one after another, but I could live and solve my problems with it. My objections are more aesthetic ones, as ae@'s variant allows to express same semantic, as my one, really.

I want to repeat: IMHO, it is orthogonal to named states and named states are great feature by itself!

[lev]

Diff against r304005, with all conflicts resolved.

[ae]

sys/netpfil/ipfw/ip_fw_dynamic.c

887 ↗

(On Diff #19228)

I'm remind again. This function doesn't do what you think. It doesn't update any states, only creates new.

[ae]

sys/netpfil/ipfw/ip_fw_dynamic.c

902 ↗

(On Diff #19228)

Ok, it can change protocol's state. But in this case, please, remove now unneeded debug message in this block.

[lev]

Remove debug output, as this branch is "ok" now.

[lev]

Better merge with named states: previous variant was formally correct, but ugly and against any codestyle.

[ae]

sys/netinet/ip_fw.h

282 ↗

(On Diff #19261)

You can not add new opcode at any place. This breaks ABI. New opcode should be inserted at the end of this enum

[lev]

Fix new opcode placement after megre.

[lev]

Update diff to latest CURRENT version

[lev]

Update to be applied to r334006 or later.

[lev]

Address review notes by a @ae : better new-style printing and more context to diff.
Also, update to r334094