Date: Mon, 29 Mar 2021 09:16:03 -0700 From: Cy Schubert <Cy.Schubert@cschubert.com> To: "Kristof Provost" <kp@FreeBSD.org> Cc: "Cy Schubert" <Cy.Schubert@cschubert.com>, "FreeBSD pf" <freebsd-pf@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: [RFC] pf ioctl changes Message-ID: <202103291616.12TGG3IL005274@slippy.cwsent.com> In-Reply-To: <75FA4097-ED2A-4B96-9C90-E82F49F7764B@FreeBSD.org> References: <24E09373-EBCD-4ED1-8B59-A44E687F287E@FreeBSD.org> <202103291403.12TE3Y2H094131@slippy.cwsent.com> <18DC1EA9-ABFC-4A06-8710-A3068370EC52@FreeBSD.org> <202103291516.12TFGjTi004433@slippy.cwsent.com> <75FA4097-ED2A-4B96-9C90-E82F49F7764B@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <75FA4097-ED2A-4B96-9C90-E82F49F7764B@FreeBSD.org>, "Kristof Provost " writes: > On 29 Mar 2021, at 17:16, Cy Schubert wrote: > > In message <18DC1EA9-ABFC-4A06-8710-A3068370EC52@FreeBSD.org>, > > "Kristof > > Provost > > " writes: > >> On 29 Mar 2021, at 16:03, Cy Schubert wrote: > >>> In message <24E09373-EBCD-4ED1-8B59-A44E687F287E@FreeBSD.org>, > >>> "Kristof > >>> Provost > >>> " writes: > >>>> Hi, > >>>> > >>>> There are several patches in the pipeline that require changes in > >>>> pfâÂÂs > >>>> interface between kernel and userspace. > >>>> In the past these have been handled in multiple ways. Either by > >>>> simply > >>>> making the change, breaking binary compatibility, or by introducing > >>>> a > >>>> v2 > >>>> ioctl (e.g. DIOCADDALTQV1). > >>>> > >>>> While one is better than the other neither is wholly satisfying. > >>>> New > >>>> versions of calls constitute a maintenance burden after all. > >>>> > >>>> IâÂÂd like to change the ioctl interface to use nvlists, > >>>> which > >>>> would > >>>> make such extensions much easier, because fields can be optional. > >>>> That is, if userspace doesnâÂÂt supply the > >>>> âÂÂshinynewfeatureâ field > >>>> the kernel can assume the default value and things just work. > >>>> Similarly, > >>>> if the kernel supplies a âÂÂshinynewfeatureâ > >>>> which userspace > >>>> doesnâÂÂt > >>>> know about itâÂÂs simply ignored. > >>>> > >>>> The rough plan is to introduce nvlist versions of the get/add rules > >>>> calls for now. Others will follow as the need presents itself. > >>>> As these are new ioctls it is safe to MFC them to stable/12 and > >>>> stable/13. > >>>> The old interface will remain supported in those branches, but > >>>> IâÂÂd > >>>> like to remove it from main (and thus FreeBSD 14). > >>>> > >>>> As part of this effort I may end up splitting off the ioctl > >>>> interface > >>>> code from pfctl into libpfctl, which should make reuse of that code > >>>> easier. > >>>> > >>>> I hope to post preliminary patches in the coming week. > >>>> > >>>> Thoughts? Objections? > >>> > >>> Kernel and userland should be, I'd say must be, kept in sync. We > >>> have > >>> many > >>> examples of userland and kernel not being in sync over the years. > >>> For > >>> ipfilter, I've made incompatible changes to data structures > >>> requiring > >>> userand and kernel be in sync. These are few and far between. > >>> > >>> I've gotten away with this because there is no third party software > >>> that > >>> relies on the ipfilter kernel interfaces. I could be wrong but I > >>> doubt > >>> there may be third party software requiring pf ABI compatibility. > >>> But > >>> if > >>> there is then verstioned library interfaces are required. > >>> > >>> Given that the advice is to keep kernel and userland in sync there > >>> probably > >>> is no requirement for an UPDATING entry but that would be your call. > >>> > >> There are out-of-tree users of the pf ioctl interface. > >> security/expiretable[1] for example. > >> security/snort2pfcd appears to as well. > >> sysutils/pfstat and sysutils/pftop use the ioctl interface as well, > >> although not the three specific calls of immediate interest. > > > > This complicates things. IMO you'll probably need versioned function > > calls > > for at least 13-STABLE EOL. Or, versioning the data structures passed > > into > > the kernel such that the new fields are at the tail of the existing > > structures. > > > That’s essentially the plan. I plan to keep the existing definitions > (of both structure and ioctl numbers) in stable/12 and stable/13. > They’ll disappear in main (i.e. 14). > > Alongside we’ll introduce new nvlist variants for those calls, which > will have the new features. > > >> Iâm trying to work out how many examples there are, because one > >> way or > >> the other theyâre going to have to cope with changes. > >> > >> So, Iâd prefer to not just change the definitions of structs, > >> even if > >> weâve done that in the past. struct pf_rule contains a few > >> peculiarities from historical mistakes that I hope to correct now. > > > > Technical debt is difficult to eliminate. We either fix it, paying it > > off > > in one lump sum or we pay it off through aggravation and design > > limitations, with interest, over time. > > > Indeed. > > To take struct pf_rule as an example: it contains counter_u64’s, which > don’t really work for userspace, so we’ve added uint64_t versions of > those variables. Now the struct has two version of the same field. > That can be cleaned up once the ioctls which use the struct have been > removed (so on main only). My plan is to remove the struct definition > from the kernel’s headers (again, once there are alternative ioctls > and only in main), moving it into libpfctl. > Then there will be nothing to stop us from removing the counter_u64 > versions of those fields, cleaning up the struct. > > > Given that pf uses ioctl, versioned function calls won't help. A new > > ioctl > > may be the only answer. If you do choose this, add an identifier and > > version number to the head of each new struct to future proof pf. > > > The nvlist versions will be much more flexible, so embedding a version > number seem redundant. This is probably the best plan. It of course adds some MFC pain or the requirement to commit directly to -STABLE when fixing serious bugs but it's manageable. -- Cheers, Cy Schubert <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org NTP: <cy@nwtime.org> Web: https://nwtime.org The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202103291616.12TGG3IL005274>