| Message ID | 20240716141656.pvlrrnxziok2jwxt@joelS2.panther.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GIT,PULL] sysctl changes for v6.11-rc1 | expand |
On Tue, Jul 16, 2024 at 04:16:56PM +0200, Joel Granados wrote: > * Preparation patches for sysctl constification > > Constifying ctl_table structs prevents the modification of proc_handler > function pointers as they would reside in .rodata. The ctl_table arguments > in sysctl utility functions are const qualified in preparation for a future > treewide proc_handler argument constification commit. And to add some additional context and expectation setting, the mechanical treewide constification pull request is planned to be sent during this merge window once the sysctl and net trees land. Thomas Weißschuh has it at the ready. :)
The pull request you sent on Tue, 16 Jul 2024 16:16:56 +0200:
> git://git.kernel.org/pub/scm/linux/kernel/git/sysctl/sysctl.git/ tags/sysctl-6.11-rc1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/f8a8b94d0698ccc56c44478169c91ca774540d9f
Thank you!
On 2024-07-17 21:46:20+0000, Joel Granados wrote: > On Tue, Jul 16, 2024 at 11:13:24AM -0700, Kees Cook wrote: > > On Tue, Jul 16, 2024 at 04:16:56PM +0200, Joel Granados wrote: > > > * Preparation patches for sysctl constification > > > > > > Constifying ctl_table structs prevents the modification of proc_handler > > > function pointers as they would reside in .rodata. The ctl_table arguments > > > in sysctl utility functions are const qualified in preparation for a future > > > treewide proc_handler argument constification commit. > > > > And to add some additional context and expectation setting, the mechanical > > treewide constification pull request is planned to be sent during this > > merge window once the sysctl and net trees land. Thomas Weißschuh has > > it at the ready. :) > > Big fan of setting expectations :). thx for the comment. > Do you (@kees/ @thomas) have any preference on how to send the treewide > const patch? I have prepared wording for the pull request for when the > time comes next week, but if any of you prefer to send it through > another path different than sysctl, please let me know. Sysctl sounds good to me. Linus, if you are already interested in the upcoming patch and its background: https://lore.kernel.org/lkml/20240619-sysctl-const-handler-v2-1-e36d00707097@weissschuh.net/ Thomas
On Wed, Jul 17, 2024 at 09:46:20PM +0200, Joel Granados wrote: > On Tue, Jul 16, 2024 at 11:13:24AM -0700, Kees Cook wrote: > > On Tue, Jul 16, 2024 at 04:16:56PM +0200, Joel Granados wrote: > > > * Preparation patches for sysctl constification > > > > > > Constifying ctl_table structs prevents the modification of proc_handler > > > function pointers as they would reside in .rodata. The ctl_table arguments > > > in sysctl utility functions are const qualified in preparation for a future > > > treewide proc_handler argument constification commit. > > > > And to add some additional context and expectation setting, the mechanical > > treewide constification pull request is planned to be sent during this > > merge window once the sysctl and net trees land. Thomas Wei?schuh has > > it at the ready. :) > > Big fan of setting expectations :). thx for the comment. > Do you (@kees/ @thomas) have any preference on how to send the treewide > const patch? I have prepared wording for the pull request for when the > time comes next week, but if any of you prefer to send it through > another path different than sysctl, please let me know. I don't have any preference. I can only speak to historical context for other treewide changes: Linus has taken a PR, a raw patch, or just run a script directly in the past, so any should work. I would guess that a PR created from a script (that is reproduced in the commit log) is the easiest, as Linus can either just take the PR or choose to run the script himself.
On Tue, Jul 16, 2024 at 04:16:56PM +0200, Joel Granados wrote: > sysctl changes for 6.11-rc1 > > Summary > > * Remove "->procname == NULL" check when iterating through sysctl table arrays > > Removing sentinels in ctl_table arrays reduces the build time size and > runtime memory consumed by ~64 bytes per array. With all ctl_table > sentinels gone, the additional check for ->procname == NULL that worked in > tandem with the ARRAY_SIZE to calculate the size of the ctl_table arrays is > no longer needed and has been removed. The sysctl register functions now > returns an error if a sentinel is used. > > * Preparation patches for sysctl constification > > Constifying ctl_table structs prevents the modification of proc_handler > function pointers as they would reside in .rodata. The ctl_table arguments > in sysctl utility functions are const qualified in preparation for a future > treewide proc_handler argument constification commit. As (I assume it was) expected, these changes broke out-of-tree modules. For LKRG, I am repairing this by adding "#if LINUX_VERSION_CODE >= KERNEL_VERSION(6,11,0)" checks around the corresponding module changes. This works. However, I wonder if it would possibly be better for the kernel to introduce a corresponding "feature test macro" (or two, for the two changes above). I worry that these changes (or some of them) could get backported to stable/longterm, which with the 6.11+ checks would unnecessarily break out-of-tree modules again (and again and again for each backport to a different kernel branch). Feature test macro(s) would avoid such further breakage, as they would (be supposed to be) included along with the backports. Joel, Linus, or anyone else - what do you think? And in general, would it be a good practice for Linux to be providing feature test macros to indicate this sort of changes? Is there a naming convention for them? For omitting the ctl_table array sentinel elements, it is now possible to check whether register_sysctl() is a function or a macro. I've tested the below and it works: +++ b/src/modules/comm_channel/p_comm_channel.c @@ -332,7 +332,14 @@ struct ctl_table p_lkrg_sysctl_table[] = { .extra1 = &p_profile_enforce_min, .extra2 = &p_profile_enforce_max, }, +/* + * Empty element at the end of array was required when register_sysctl() was a + * function. It's no longer required when it became a macro in 2023, and it's + * disallowed after further changes in 2024. + */ +#ifndef register_sysctl { } +#endif }; But it's a hack, which I'm unhappy about. So instead of a macro indicating that the "Remove "->procname == NULL" check when iterating through sysctl table arrays" change is in place, we could have one that indicates that the sentinel elements are no longer required (and no need for one indicating that they're no longer allowed, then). Something like LINUX_SYSCTL_NO_SENTINELS. This could even be backported to kernels that do not have the "Remove "->procname == NULL" check" commit, if they do have last year's removal of the requirement. Alternatively, maybe "Remove "->procname == NULL" check when iterating through sysctl table arrays" should be reverted. I can see how it's useful as a policy check for the kernel itself, so no space is inadvertently wasted on a sentinel element anywhere in the kernel tree, but maybe it isn't worth enforcing this for out-of-tree modules. The impact of an extra element (if allowed) is negligible, whereas the impact of not having it on an older kernel is really bad. I worry that some out-of-tree modules would be adapted or written for the new convention without a 6.11+ check, yet someone would also build and use them on pre-6.11. There's no compile-time failure from omitting the sentinel element on a kernel where it was needed, and there isn't a _reliable_ runtime failure either. The other macro could be called LINUX_SYSCTL_TABLE_CONST, although I'm not sure whether it should apply only to the "ctl_table arguments in sysctl utility functions" (the change so far) or also to "Constifying ctl_table structs" (a near future change, right?) Alexander
On Tue, 6 Aug 2024 at 11:57, Solar Designer <solar@openwall.com> wrote: > > As (I assume it was) expected, these changes broke out-of-tree modules. It was presumably not expected - but for the simple reason that no kernel developer should spend one single second worrying about out-of-tree modules. It's simply not a concern - never has been, and never will be. Now, if some out-of-tree module is on the cusp of being integrated, and is out-of-tree just because it's not quite ready yet, that would maybe be then a case of "hey, wait a second". But no. We are not going to start any kind of feature test macros for external modules in general. Linus
On 2024-08-06 20:57:37+0000, Solar Designer wrote: > On Tue, Jul 16, 2024 at 04:16:56PM +0200, Joel Granados wrote: > > sysctl changes for 6.11-rc1 > > > > Summary > > > > * Remove "->procname == NULL" check when iterating through sysctl table arrays > > > > Removing sentinels in ctl_table arrays reduces the build time size and > > runtime memory consumed by ~64 bytes per array. With all ctl_table > > sentinels gone, the additional check for ->procname == NULL that worked in > > tandem with the ARRAY_SIZE to calculate the size of the ctl_table arrays is > > no longer needed and has been removed. The sysctl register functions now > > returns an error if a sentinel is used. > > > > * Preparation patches for sysctl constification > > > > Constifying ctl_table structs prevents the modification of proc_handler > > function pointers as they would reside in .rodata. The ctl_table arguments > > in sysctl utility functions are const qualified in preparation for a future > > treewide proc_handler argument constification commit. > > As (I assume it was) expected, these changes broke out-of-tree modules. > For LKRG, I am repairing this by adding "#if LINUX_VERSION_CODE >= > KERNEL_VERSION(6,11,0)" checks around the corresponding module changes. > This works. However, I wonder if it would possibly be better for the > kernel to introduce a corresponding "feature test macro" (or two, for > the two changes above). I worry that these changes (or some of them) > could get backported to stable/longterm, which with the 6.11+ checks > would unnecessarily break out-of-tree modules again (and again and again > for each backport to a different kernel branch). Feature test macro(s) > would avoid such further breakage, as they would (be supposed to be) > included along with the backports. I don't see any of these changes being backported. The removal of the "->procname == NULL" check depends on all in-kernel tables being registered with an explicit size, which is not the case on old kernels. So a backport would not only silently fail for external modules but also for internal code. The same for the constification patches but with build errors instead of runtime errors. My future sysctl constification patches will be backwards compatible at both compiletime and runtime, for both internal and external code. So the version checks should be enough here.
On Tue, Aug 06, 2024 at 08:57:37PM +0200, Solar Designer wrote: > On Tue, Jul 16, 2024 at 04:16:56PM +0200, Joel Granados wrote: > > sysctl changes for 6.11-rc1 > > > > Summary > > > > * Remove "->procname == NULL" check when iterating through sysctl table arrays > > > > Removing sentinels in ctl_table arrays reduces the build time size and > > runtime memory consumed by ~64 bytes per array. With all ctl_table > > sentinels gone, the additional check for ->procname == NULL that worked in > > tandem with the ARRAY_SIZE to calculate the size of the ctl_table arrays is > > no longer needed and has been removed. The sysctl register functions now > > returns an error if a sentinel is used. > > > > * Preparation patches for sysctl constification > > > > Constifying ctl_table structs prevents the modification of proc_handler > > function pointers as they would reside in .rodata. The ctl_table arguments > > in sysctl utility functions are const qualified in preparation for a future > > treewide proc_handler argument constification commit. > > As (I assume it was) expected, these changes broke out-of-tree modules. > For LKRG, I am repairing this by adding "#if LINUX_VERSION_CODE >= > KERNEL_VERSION(6,11,0)" checks around the corresponding module changes. > This works. However, I wonder if it would possibly be better for the > kernel to introduce a corresponding "feature test macro" (or two, for > the two changes above). I worry that these changes (or some of them) > could get backported to stable/longterm, which with the 6.11+ checks > would unnecessarily break out-of-tree modules again (and again and again > for each backport to a different kernel branch). Feature test macro(s) > would avoid such further breakage, as they would (be supposed to be) > included along with the backports. > > Joel, Linus, or anyone else - what do you think? And in general, would As mentioned by Thomas; These changed must not be backported and therefore there is not concern about backport consequences. > it be a good practice for Linux to be providing feature test macros to > indicate this sort of changes? Is there a naming convention for them? I don't think that would be a good practice. IMO, a good way to handle these things in out-of-tree modules is the LINUX_VERSION_CODE hack. You can see it here for the same reason : https://github.com/cryptodev-linux/cryptodev-linux/commit/99ae2a39ddc3f89c66d9f09783b591c0f2dbf2e9 ... Best
Linus Note: I have update the capabilities in my signing key. I don't think anything changes on your side, but thought I'd mention it for good measure. Pulling from https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git would probably solve any unforeseen issues. The following changes since commit c3f38fa61af77b49866b006939479069cd451173: Linux 6.10-rc2 (2024-06-02 15:44:56 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/sysctl/sysctl.git/ tags/sysctl-6.11-rc1 for you to fetch changes up to acc154691fc75e1a178fc36624bdeee1420585a4: sysctl: Warn on an empty procname element (2024-06-13 10:50:52 +0200) ---------------------------------------------------------------- sysctl changes for 6.11-rc1 Summary * Remove "->procname == NULL" check when iterating through sysctl table arrays Removing sentinels in ctl_table arrays reduces the build time size and runtime memory consumed by ~64 bytes per array. With all ctl_table sentinels gone, the additional check for ->procname == NULL that worked in tandem with the ARRAY_SIZE to calculate the size of the ctl_table arrays is no longer needed and has been removed. The sysctl register functions now returns an error if a sentinel is used. * Preparation patches for sysctl constification Constifying ctl_table structs prevents the modification of proc_handler function pointers as they would reside in .rodata. The ctl_table arguments in sysctl utility functions are const qualified in preparation for a future treewide proc_handler argument constification commit. * Misc fixes Increase robustness of set_ownership by providing sane default ownership values in case the callee doesn't set them. Bound check proc_dou8vec_minmax to avoid loading buggy modules and give sysctl testing module a name to avoid compiler complaints. Testing * This got push to linux-next in v6.10-rc2, so it has had more than a month of testing ---------------------------------------------------------------- Jeff Johnson (1): sysctl: Add module description to sysctl-testing Joel Granados (8): locking: Remove superfluous sentinel element from kern_lockdep_table mm profiling: Remove superfluous sentinel element from ctl_table sysctl: Remove check for sentinel element in ctl_table arrays sysctl: Replace nr_entries with ctl_table_size in new_links sysctl: Remove superfluous empty allocations from sysctl internals sysctl: Remove "child" sysctl code comments sysctl: Remove ctl_table sentinel code comments sysctl: Warn on an empty procname element Thomas Weißschuh (3): sysctl: always initialize i_uid/i_gid utsname: constify ctl_table arguments of utility function sysctl: constify ctl_table arguments of utility function Wen Yang (1): sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array fs/proc/proc_sysctl.c | 70 ++++++++++++++++++++++++++---------------------- include/linux/sysctl.h | 2 +- kernel/locking/lockdep.c | 1 - kernel/sysctl-test.c | 50 ++++++++++++++++++++++++++++++++++ kernel/sysctl.c | 31 +++++++++------------ kernel/utsname_sysctl.c | 2 +- lib/alloc_tag.c | 1 - net/sysctl_net.c | 11 ++------ 8 files changed, 105 insertions(+), 63 deletions(-)