Date: Mon, 16 Oct 2023 09:37:51 +0200 From: Kristof Provost <kp@FreeBSD.org> To: Shawn Webb <shawn.webb@hardenedbsd.org> Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 2cef62886dc7 - main - pf: convert state retrieval to netlink Message-ID: <92D6FE8C-50FF-4DC2-B0BF-29CBC7A4075C@FreeBSD.org> In-Reply-To: <20231015201139.zt7mfyss4ua2bkn3@mutt-hbsd> References: <202310100950.39A9oYuc029996@gitrepo.freebsd.org> <20231015201139.zt7mfyss4ua2bkn3@mutt-hbsd>
next in thread | previous in thread | raw e-mail | index | archive | help
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=3D2cef62886dc7c33ca01f70c= a712845da1e55b470 >> >> 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 s= tate >> stables. With the previous implementation we had to build the enti= re >> list (both in the kernel and in userspace) before we could start >> processing. With netlink we start to get data in userspace while t= he >> kernel is still generating more. This reduces peak memory consumpt= ion >> (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 reg= ard >> it's similar to an nvlist-based approach, except that it also deal= s >> with transport to userspace and that it performs significantly bet= ter >> than nvlists. Testing has failed to measure a performance differen= ce >> between the previous struct-copy based ioctl and the netlink appro= ach. >> >> 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 !=3D NULL) >> destroy_dev(pf_dev); >> >> @@ -6683,6 +6686,7 @@ pf_modevent(module_t mod, int type, void *data) >> switch(type) { >> case MOD_LOAD: >> error =3D 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 =3D { >> }; >> >> 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=E2=80=99m not maintaining multiple configuration paths for pf. There=E2= =80=99s currently fallback paths to support old binaries, but I intend to= remove that code as soon as possible. Having configuration paths that ar= e untested, practically untestable and unmaintained is not a situation we= want to maintain either. Kristof
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?92D6FE8C-50FF-4DC2-B0BF-29CBC7A4075C>