[RFC] pf ioctl changes

[Kristof Provost]

"Kristof
Provost

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.

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.

Best regards,
Kristof

[1] Perhaps not the greatest example, because its use of struct pf_state
was incorrect, so it couldn’t actually have worked correctly before it
stopped building. See PR #253547 for details.

[Kristof Provost]

"Kristof
Provost

Post by Kristof Provost

"Kristof
Provost

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.

Post by Kristof Provost
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.

Best regards,
Kristof

[Cy Schubert]

In message <75FA4097-ED2A-4B96-9C90-***@FreeBSD.org>, "Kristof
Provost

Post by Kristof Provost

"Kristof
Provost

Post by Kristof Provost

"Kristof
Provost

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.

Post by Kristof Provost
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.

[Kristof Provost]

I hope to post preliminary patches in the coming week.

- https://reviews.freebsd.org/D29556
- https://reviews.freebsd.org/D29557
- https://reviews.freebsd.org/D29558
- https://reviews.freebsd.org/D29559
- https://reviews.freebsd.org/D29560
- https://reviews.freebsd.org/D29561
- https://reviews.freebsd.org/D29562

Best regards,
Kristof