pf: Add the action structure size (in words) to the sub-header.

[marcel]

Summary

Adding the per-action structure size to the sub-header means that:

1. Unknown actions can be skipped. The size they occupy in the packet is count * size * 2. This is useful when the consumer of the packet isn't necessarily an identical kernel and only a subset of actions is relevant or known.
2. The size of the action structure is variable. Variability is possible if and when pfsync(4) can be configured to include optional fields. Even if there's no such configuration, the ability to add a field to an action structure without it requiring a new action code and thus a version bump is very valuable.

Test Plan

If pfsync(4) is used in user space (whether source tree or ports), a recompile is needed if the code
expects the version to be identical to what was compiled-in.

[jhb]

I'm actually not very familiar with pf(4) so probably not well-suited to thinking about the implications of this change.

sys/net/if_pfsync.h

113

Hmm, I find "word" a bit ambiguous as I don't always associate it with uint16_t. Instead, I think of it as the "natural" integer size of a processor (basically the size of a GP register). Even though Intel refers to 32-bits as DWORDS for legacy 16-bit compat reasons, I think of the word size on i386 to be 32-bits and on amd64 as 64-bits, etc. I would just suggest making this comment more explicit, e.g. "Size of structure in 16-bit units."

[glebius]

Is this part of a bigger change? What's the final goal.

The problem with bumping pfsync version is that two routers of different version will no longer be able to communicate. Meaning old FreeBSD won't communicate with newer. The problem is even worse, since OpenBSD has already bumped version to 6 and for completely different change. If we bump it to 6, for this change, this is going to be a mess. FreeBSD 10.0 was able to sync with OpenBSD from that times, and IMHO ruining protocol compatibility is evil. And although we decided not to do bulk imports of pf from OpenBSD, but still we respect them as owners of the protocol.

[marcel]

In D10863#225094, @glebius wrote:

Is this part of a bigger change? What's the final goal.

The goal is to make the definition less rigid. We're (= Bracket) use pfsync(4) for NetFlow v9 export and we really need the byte and packet counters in the compressed update and delete structures.

The problem with bumping pfsync version is that two routers of different version will no longer be able to communicate. Meaning old FreeBSD won't communicate with newer. The problem is even worse, since OpenBSD has already bumped version to 6 and for completely different change. If we bump it to 6, for this change, this is going to be a mess. FreeBSD 10.0 was able to sync with OpenBSD from that times, and IMHO ruining protocol compatibility is evil. And although we decided not to do bulk imports of pf from OpenBSD, but still we respect them as owners of the protocol.

Protocol compatibility has never been part of the protocol definition. As you point out, compatibility is already broken between FreeBSD and OpenBSD and any change we need to make will break compatibility will previous versions. We broke compatibility between FreeBSD 10 and older. How can we ever keep in sync with OpenBSD if their constraints are far more reasonable that ours (yours)?

I concur that If OpenBSD has bumped their version already, then we should not do it unless it's for the exact same change. I'll have an updated patch shortly that should address the concerns.

[marcel]

Don't change the version in the header (i.e. keep at 5), and
instead use the subsequent _pad field for a revision. The
revision allows minor changes to the definitions while not
breaking compatibility with previous revisions.

[marcel]

If there are no objections, I'll commit this in a few days.

[glebius]

Actually my comment was an objection. And you read me wrong on compatibility history. All FreeBSD versions that support pfsync, starting with FreeBSD 5.4 and ending in FreeBSD 12.0 are compatible. They are also compatible with OpenBSD versions that used protocol version 5.

I think that we must stay compatible with FreeBSD, so that a CARP pair of machines can be upgraded one by one, and pair would be in sync when running different versions. I think that we must at least try to be compatible with OpenBSD. We should contact them and discuss that. The feature you suggest is useful, and both OSes can benefit from it.

Please sign up more people for this review. I really don't like to block someone commits in a 1 to 1 opposition. Let's see what more people think on how is it important to keep compatibility in this particular case.

[marcel]

The updated diff addresses your concern and eliminates incompatibilities. Your comment tells me that you haven't seen the updated diff...

[glebius]

I apologize, Marcel. Didn't notice the update. So, what do you plan to add for compatibility with older FreeBSD versions? My suggestion is that if we ever receive a bulk update request with version 5.0, we must downgrade to version 5.0. If this is implemented, I am all for this change.

One more note: I've checked all pfsync subheaders, and they all are 32-bit multiple at their current version. Why do you shift size by 1, not by 2? We can get more compression here. Also, can you please add CTASSERTs that would assert that current sizes of structures are 16-bit multiple (or 32-bit multiple, if you agree that we can compress more).

[marcel]

First the quick answer: The pf_state structure as sent by the INS, DEL and UPD actions is not a multiple of 4-bytes on amd64. All the pfsync structure are indeed.

As for the backward compatibility: I don't have any good answers, other than that we need to find a way to create a packet exchange that is compatible with version 5.0 and for the purpose of letting each other know that version 5.1 or higher is supported. Eventually the exchanges need to lead to an agreed upon version.

Ideally we can introduce a packet that is designed for exchanging capabilities, but a peer cannot immediately send it without causing compatibility problems.

I'm thinking along these lines:
When both peers support 5.1, one side can piggy back an extra field in an existing structure (which structure and what field is part of 5.2). If the other side understands 5.2, it will do the same. A 5.1 peer will simply ignore the extra data. The extra data understood by 5.2 peers is the signal that a negotiation protocol can be used to negotiate the actual characteristics.

Once the pfsync protocol has negotiation capabilities, changes are a lot easier to make to the packets. Provided of course is part of the negotiation...

In a nutshell: I think it starts with having the ability to send extra data with a 5.0 packet. Once both sides consume the extra data (instead of ignoring it), peers can start sending non-5.0 packets

[glebius]

Oh, they use struct pfsync_state. :( So we are limited to 512 byte subheaders for the time being of this 5.x protocol.

Here is my compatibility plan. It is very simple and robust. When we start pfsync, we always request bulk update. This is done in pfsyncioctl() case SIOCSETPFSYNC. This is our first packet out. And it is very basic packet. And if there are any other pfsync hosts on the network, they will reply. So, we basically put our 5.x version into this bulk update request, but we do not violate the 5.0 protocol, since packet is very simple. So it has non-zero "revision" octet, but it is 5.0 compatible. If the remote peer is a 5.1+ host, it reads our header, and knows our version. If it is lower than peer, the peer downgrades. If the remote peer is old 5.0 host, e.g. FreeBSD 11.0, then it ignores our "revision" and continues as 5.0 host. Anyways, peer processes the bulk update, and we start to receive packets from them. If we see that their version is lower than ours, we downgrade.