Re: [RFC] pf ioctl changes
On 27 Mar 2021, at 12:54, Kristof Provost wrote: > 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 ___ 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"
Re: [RFC] pf ioctl changes
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
Re: [RFC] pf ioctl changes
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. -- Cheers, Cy Schubert FreeBSD UNIX: Web: https://FreeBSD.org NTP: 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"
Re: [RFC] pf ioctl changes
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. Best regards, Kristof ___ 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"
Re: [RFC] pf ioctl changes
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. > > 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. 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. > > 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. -- Cheers, Cy Schubert FreeBSD UNIX: Web: https://FreeBSD.org NTP: 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"
Re: [RFC] pf ioctl changes
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. 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. ___ 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"
[RFC] pf ioctl changes
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? Best regards, Kristof ___ 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"
