In message <75fa4097-ed2a-4b96-9c90-e82f49f77...@freebsd.org>, "Kristof Provost " writes: > On 29 Mar 2021, at 17:16, Cy Schubert wrote: > > In message <18dc1ea9-abfc-4a06-8710-a3068370e...@freebsd.org>, > > "Kristof > > Provost > > " writes: > >> On 29 Mar 2021, at 16:03, Cy Schubert wrote: > >>> In message <24e09373-ebcd-4ed1-8b59-a44e687f2...@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.schub...@cschubert.com> FreeBSD UNIX: <c...@freebsd.org> Web: https://FreeBSD.org NTP: <c...@nwtime.org> Web: https://nwtime.org The need of the many outweighs the greed of the few. _______________________________________________ freebsd-pf@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-pf To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"
