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

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

[Joel Granados]

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(-)

-- 

Joel Granados

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

[Kees Cook]

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. :)

-- 
Kees Cook

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

[Joel Granados]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 1086 bytes --]

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.

Best

-- 

Joel Granados

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

[Thomas Weißschuh]

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

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

[Kees Cook]

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.

-- 
Kees Cook

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

[pr-tracker-bot]

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!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Solar Designer]

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

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

[Linus Torvalds]

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

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

[Thomas Weißschuh]

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.

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

[Joel Granados]

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

-- 

Joel Granados