Re: git: 2cef62886dc7 - main - pf: convert state retrieval to netlink

On 15 Oct 2023, at 22:11, Shawn Webb wrote:
> On Tue, Oct 10, 2023 at 09:50:34AM +0000, Kristof Provost wrote:
>> The branch main has been updated by kp:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/?id=2cef62886dc7c33ca01f70ca712845da1e55b470
>>
>> commit 2cef62886dc7c33ca01f70ca712845da1e55b470
>> Author:     Alexander V. Chernikov <melifaro@FreeBSD.org>
>> AuthorDate: 2023-09-15 10:06:59 +0000
>> Commit:     Kristof Provost <kp@FreeBSD.org>
>> CommitDate: 2023-10-10 09:48:21 +0000
>>
>>     pf: convert state retrieval to netlink
>>
>>     Use netlink to export pf's state table.
>>
>>     The primary motivation is to improve how we deal with very large state
>>     stables. With the previous implementation we had to build the entire
>>     list (both in the kernel and in userspace) before we could start
>>     processing. With netlink we start to get data in userspace while the
>>     kernel is still generating more. This reduces peak memory consumption
>>     (which can get to the GB range once we hit millions of states).
>>
>>     Netlink also makes future extension easier, in that we can easily add
>>     fields to the state export without breaking userspace. In that regard
>>     it's similar to an nvlist-based approach, except that it also deals
>>     with transport to userspace and that it performs significantly better
>>     than nvlists. Testing has failed to measure a performance difference
>>     between the previous struct-copy based ioctl and the netlink approach.
>>
>>     Differential Revision:  https://reviews.freebsd.org/D38888
>> ---
>>  include/Makefile          |   3 +-
>>  lib/libpfctl/libpfctl.c   | 214 +++++++++++++++++----------------
>>  sys/conf/files            |   1 +
>>  sys/modules/pf/Makefile   |   2 +-
>>  sys/netpfil/pf/pf_ioctl.c |   5 +
>>  sys/netpfil/pf/pf_nl.c    | 292 ++++++++++++++++++++++++++++++++++++++++++++++
>>  sys/netpfil/pf/pf_nl.h    | 105 +++++++++++++++++
>>  7 files changed, 522 insertions(+), 100 deletions(-)
>
>> diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c
>> index db8f481a1567..42c2aa9bfb01 100644
>> --- a/sys/netpfil/pf/pf_ioctl.c
>> +++ b/sys/netpfil/pf/pf_ioctl.c
>> @@ -83,6 +83,7 @@
>>  #include <netinet/ip_var.h>
>>  #include <netinet6/ip6_var.h>
>>  #include <netinet/ip_icmp.h>
>> +#include <netpfil/pf/pf_nl.h>
>>  #include <netpfil/pf/pf_nv.h>
>>
>>  #ifdef INET6
>> @@ -6648,6 +6649,8 @@ pf_unload(void)
>>  	}
>>  	sx_xunlock(&pf_end_lock);
>>
>> +	pf_nl_unregister();
>> +
>>  	if (pf_dev != NULL)
>>  		destroy_dev(pf_dev);
>>
>> @@ -6683,6 +6686,7 @@ pf_modevent(module_t mod, int type, void *data)
>>  	switch(type) {
>>  	case MOD_LOAD:
>>  		error = pf_load();
>> +		pf_nl_register();
>>  		break;
>>  	case MOD_UNLOAD:
>>  		/* Handled in SYSUNINIT(pf_unload) to ensure it's done after
>> @@ -6703,4 +6707,5 @@ static moduledata_t pf_mod = {
>>  };
>>
>>  DECLARE_MODULE(pf, pf_mod, SI_SUB_PROTO_FIREWALL, SI_ORDER_SECOND);
>> +MODULE_DEPEND(pf, netlink, 1, 1, 1);
>>  MODULE_VERSION(pf, PF_MODVER);
>
> Hey Kristof,
>
> This causes a hard dependency on the netlink kernel module, which may
> not be available in some configurations.
>
> For safety reasons, HardenedBSD prevents loading of netlink.ko by
> default. The code is too new and too complex, with already a
> not-so-nice security history, to be trusted.
>
> A lot (all?) of the other netlink integration code respects the
> potential unavailability of netlink (or netlink.ko). Would it be
> possible to do the same in pf?
>
No.

I’m not maintaining multiple configuration paths for pf. There’s currently fallback paths to support old binaries, but I intend to remove that code as soon as possible. Having configuration paths that are untested, practically untestable and unmaintained is not a situation we want to maintain either.

Kristof