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

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?

Thanks,

-- 
Shawn Webb
Cofounder / Security Engineer
HardenedBSD

https://git.hardenedbsd.org/hardenedbsd/pubkeys/-/raw/master/Shawn_Webb/03A4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc