[RFC] pf ioctl changes

[Cy Schubert]

In message <75FA4097-ED2A-4B96-9C90-E82F49F7764B at FreeBSD.org>, "Kristof 
Provost
" writes:
> On 29 Mar 2021, at 17:16, Cy Schubert wrote:
> > In message <18DC1EA9-ABFC-4A06-8710-A3068370EC52 at FreeBSD.org>, 
> > "Kristof
> > Provost
> > " writes:
> >> On 29 Mar 2021, at 16:03, Cy Schubert wrote:
> >>> In message <24E09373-EBCD-4ED1-8B59-A44E687F287E at 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 at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  https://FreeBSD.org
NTP:           <cy at nwtime.org>    Web:  https://nwtime.org

	The need of the many outweighs the greed of the few.