Re: [GIT PULL] sysctl changes for v6.11-rc1

[Solar Designer <solar@openwall.com>]

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