Make contents of struct pfsync_state configurable

[vegeta_tuxpowered.net]

Summary

This patch makes struct pfsync_state contents configurable by sending out new versions of the structure in separate subheader actions. Both old and new version of struct pfsync_state can be understood, so replication of states from a system running an older kernel is possible. The version being sent out is configured using ifconfig pfsync0 … version XXXX. The version is an user-friendly string - 1301 stands for FreeBSD 13.1 (I have checked synchronization against a host running 13.1), 1400 stands for 14.0. A host running an older kernel will just ignore the messages and count them as "packets discarded for bad action".

Sponsored by: InnoGames GmbH

[vegeta_tuxpowered.net]

Remove debug printfs

[vegeta_tuxpowered.net]

sys/net/if_pfsync.h

59

Please note that I've decided to not change the protocol version. Making the code work with messages from older peer would not be as easy as it is with subheaders.

sys/net/pfvar.h

1072

Please don't treat this version of structure as final yet, I am not 100% sure how will it play with D38025.

sys/netpfil/pf/if_pfsync.c

154

State deletions are sent out using pfsync_out_del which sends out struct pfsync_del_c containing only creator id and state id. The actions are queued in PFSYNC_Q_DEL. The receiving functions for those is pfsync_in_del_c.

It seems that pfsync_in_del is never used. Should we remove it? And clean up queue and function names to _c?

[vegeta_tuxpowered.net]

sys/net/if_pfsync.h

65

A new version would be introduced only when compatibility is broken. In theory the 1301 version could be renamed to something lower. As far as my own experience with FreeBSD goes it could be probably 10.0. But I have tested it only with a 13.1 peer, so let's keep this as the initial value.

[kp]

I'll get to a more in-depth look at this when I'm back from AsiaBSD.

sys/net/pfvar.h

1072

I'm fine with breaking the protocol compatibility within 14 until we release 14.0 anyway.

I mean, we shouldn't go out of our way to break it, but we're not going to install compatibility hacks for changes within main prior to a release either.

sys/netpfil/pf/if_pfsync.c

154

It seems so. I'll try to do a bit of a dive in the history later, but it seems like this might be for compatibility with some older version, in which case it's probably old enough we can just delete it.

[vegeta_tuxpowered.net]

Added function pfsync_sstate_to_qid to translates pf_kstate->sync_state to queue name. This removes multiple such translations scattered around the code and fixes pfsync_q_del.

Removed PFSYNC_ACT_DEL and ensured that other deletion functions and constants are named with _c suffix as this seems to be the convention for code operating only on state id.

Removed pfsync_state_1301->state_flags_compat and restored the 8-bit state_flags. That leaves pfsync as broken as it was for FreeBSD 13.1 compatibility. Since this patch provides a way to switch to a new pfsync data structure such tricks should not be needed. The 8+16-bit state_flags trick is kept on struct pf_state_export, though as for this structure no versioning is possible but a lot of spare space is available.

[vegeta_tuxpowered.net]

sys/netpfil/pf/if_pfsync.c

154

Do you want me to split the pf_sync_in_del change into a separate review?

[kp]

sys/netpfil/pf/if_pfsync.c

154

Yeah, that seems like w good idea.

Sorry I haven’t gotten around to this one yet. It’s on my list, but you may be familiar with the ever growing nature of todo lists.

[kp]

sys/net/pfvar.h

736

I'd separate this as its own commit as well.

959

Extending the state information passed to userspace probably belongs in a separate commit as well.

sys/netpfil/pf/if_pfsync.c

154

I did a little bit of digging, and I think that sending of PFSYNC_ACT_DEL was retired in OpenBSD in this commit:

commit c62efabc0a1b17d5fbc801b77749f7b4b8b7049b
Author: dlg <[e-mail]>
Date:   Mon Feb 16 00:31:25 2009 +0000

    pfsync v5, mostly written at n2k9, but based on work done at n2k8.

    WARNING: THIS BREAKS COMPATIBILITY WITH THE PREVIOUS VERSION OF PFSYNC

    this is a new variant of the protocol and a large reworking of the
    pfsync code to address some performance issues. the single largest
    benefit comes from having multiple pfsync messages of different
    types handled in a single packet. pfsyncs handling of pf states is
    highly optimised now, along with packet parsing and construction.

    huggz for beck@ for testing.
    huge thanks to mcbride@ for his help during development and for
    finding all the bugs during the initial tests.
    thanks to peter sutton for letting me get credit for this work.

    ok beck@ mcbride@ "good." deraadt@

So I'm inclined to say we can indeed just get rid of support for it entirely.

2233

I'd default: this, because if we add a new version that doesn't affect PFSYNC_S_INS we'd have to add another case here (or change it to default).

[vegeta_tuxpowered.net]

sys/net/pfvar.h

736

Moved into D40013

sys/netpfil/pf/if_pfsync.c

154

Moved into D40004.

[vegeta_tuxpowered.net]

Rebased on D40004 and D40013 as separate commits.

[vegeta_tuxpowered.net]

Rebased again, the previous diff has been wrongly generated.

[vegeta_tuxpowered.net]

Fixed broken rebase on main.

[vegeta_tuxpowered.net]

Split userspace export into a separate commit for DIOCGETSTATESV2.

[vegeta_tuxpowered.net]

Added tcpdump and netstat changes to this patch because of change of struct pfsync_state to struct pfsync_state_1301.

[kp]

I've changed the title line of the commit message, so I expect the hooks won't pick up that this landed. Can you close/abandon this review?