Current state of the sysctl constification effort

Current state of the sysctl constification effort

[Thomas Weißschuh]

Hi Joel, Hi Luis,

most of the sysctl handler preparation patches have been picked up by
the subsystem maintainers and are available in -next.

Only two are missing:

* utsname: constify ctl_table arguments of utility function [0]
* sysctl: constify ctl_table arguments of utility function [1]

Both of them are going through the sysctl tree anyways.

With this done it should be possible to also queue up 
sysctl: treewide: constify the ctl_table argument of handlers [2]
for the bots to chew on in -next.

My local builds are still succeeding on the last submitted version of
the patch.


Thomas

[0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-utsname-v1-1-27a6c8813620@weissschuh.net/
[1] https://lore.kernel.org/lkml/20240513-jag-constfy_sysctl_proc_args-v1-1-bba870a480d5@samsung.com/
[2] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-11-e0beccb836e2@weissschuh.net/

Re: Current state of the sysctl constification effort

[Kees Cook]

On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> Hi Joel, Hi Luis,
> 
> most of the sysctl handler preparation patches have been picked up by
> the subsystem maintainers and are available in -next.
> 
> Only two are missing:
> 
> * utsname: constify ctl_table arguments of utility function [0]
> * sysctl: constify ctl_table arguments of utility function [1]
> 
> Both of them are going through the sysctl tree anyways.

This is great! Is the target v6.11 or v6.10 for these?

-Kees

> With this done it should be possible to also queue up 
> sysctl: treewide: constify the ctl_table argument of handlers [2]
> for the bots to chew on in -next.
> 
> My local builds are still succeeding on the last submitted version of
> the patch.
> 
> 
> Thomas
> 
> [0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-utsname-v1-1-27a6c8813620@weissschuh.net/
> [1] https://lore.kernel.org/lkml/20240513-jag-constfy_sysctl_proc_args-v1-1-bba870a480d5@samsung.com/
> [2] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-11-e0beccb836e2@weissschuh.net/

-- 
Kees Cook

Re: Current state of the sysctl constification effort

[Joel Granados]

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

On Fri, May 31, 2024 at 09:31:24AM -0700, Kees Cook wrote:
> On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> > Hi Joel, Hi Luis,
> > 
> > most of the sysctl handler preparation patches have been picked up by
> > the subsystem maintainers and are available in -next.
> > 
> > Only two are missing:
> > 
> > * utsname: constify ctl_table arguments of utility function [0]
> > * sysctl: constify ctl_table arguments of utility function [1]
> > 
> > Both of them are going through the sysctl tree anyways.
> 
> This is great! Is the target v6.11 or v6.10 for these?
I was pulling all this into 6.11.

> 
> -Kees
> 
> > With this done it should be possible to also queue up 
> > sysctl: treewide: constify the ctl_table argument of handlers [2]
> > for the bots to chew on in -next.
> > 
> > My local builds are still succeeding on the last submitted version of
> > the patch.
> > 
> > 
> > Thomas
> > 
> > [0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-utsname-v1-1-27a6c8813620@weissschuh.net/
> > [1] https://lore.kernel.org/lkml/20240513-jag-constfy_sysctl_proc_args-v1-1-bba870a480d5@samsung.com/
> > [2] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-11-e0beccb836e2@weissschuh.net/
> 
> -- 
> Kees Cook

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Joel Granados]

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

On Fri, May 31, 2024 at 09:31:24AM -0700, Kees Cook wrote:
> On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> > Hi Joel, Hi Luis,
> > 
> > most of the sysctl handler preparation patches have been picked up by
> > the subsystem maintainers and are available in -next.
> > 
> > Only two are missing:
> > 
> > * utsname: constify ctl_table arguments of utility function [0]
> > * sysctl: constify ctl_table arguments of utility function [1]
> > 
> > Both of them are going through the sysctl tree anyways.
> 
> This is great! Is the target v6.11 or v6.10 for these?
> 
> -Kees
> 
> > With this done it should be possible to also queue up 
> > sysctl: treewide: constify the ctl_table argument of handlers [2]
> > for the bots to chew on in -next.
@kees: Since you have probably done these before, I'll ask you the
questions:

1. The idea is to send Linus the treewide-constify patch on its own at
   the end of the merge window for 6.11. Right?

2. Is there a special way to send these treewide patches? Or is it just
   a regular PR with an explanation on why it is being done?

3. Can you please send (if there are any) me any examples where this has
   been done in the past. Maybe some lore.kernel.org links?

Best

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Kees Cook]

On Wed, Jun 05, 2024 at 10:26:25AM +0200, Joel Granados wrote:
> On Fri, May 31, 2024 at 09:31:24AM -0700, Kees Cook wrote:
> > On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Wei?schuh wrote:
> > > Hi Joel, Hi Luis,
> > > 
> > > most of the sysctl handler preparation patches have been picked up by
> > > the subsystem maintainers and are available in -next.
> > > 
> > > Only two are missing:
> > > 
> > > * utsname: constify ctl_table arguments of utility function [0]
> > > * sysctl: constify ctl_table arguments of utility function [1]
> > > 
> > > Both of them are going through the sysctl tree anyways.
> > 
> > This is great! Is the target v6.11 or v6.10 for these?
> > 
> > -Kees
> > 
> > > With this done it should be possible to also queue up 
> > > sysctl: treewide: constify the ctl_table argument of handlers [2]
> > > for the bots to chew on in -next.
> @kees: Since you have probably done these before, I'll ask you the
> questions:
> 
> 1. The idea is to send Linus the treewide-constify patch on its own at
>    the end of the merge window for 6.11. Right?

Right. The best time is likely around Wed on the second week of the merge
window, assuming all dependencies have landed. And it could be sent
earlier if all the dependencies land sooner than that.

> 2. Is there a special way to send these treewide patches? Or is it just
>    a regular PR with an explanation on why it is being done?

I would do a regular PR with all the details for Linus to do the change
himself, but many times people send these as an explicit patch. For
example, include the full Coccinelle script, or the "sed" command
line, etc, and then detail any "by hand" changes that were needed on
top of that.

> 3. Can you please send (if there are any) me any examples where this has
>    been done in the past. Maybe some lore.kernel.org links?

I found this one that is a good example, though it's a PATCH not a GIT PULL:

https://lore.kernel.org/lkml/20221220134519.3dd1318b@gandalf.local.home/
became
https://git.kernel.org/linus/292a089d78d3e2f7944e60bb897c977785a321e3

-Kees

-- 
Kees Cook

Re: Current state of the sysctl constification effort

[Joel Granados]

On Thu, Jun 06, 2024 at 11:52:25AM -0700, Kees Cook wrote:
> On Wed, Jun 05, 2024 at 10:26:25AM +0200, Joel Granados wrote:
> > On Fri, May 31, 2024 at 09:31:24AM -0700, Kees Cook wrote:
...
> > @kees: Since you have probably done these before, I'll ask you the
> > questions:
> > 
> > 1. The idea is to send Linus the treewide-constify patch on its own at
> >    the end of the merge window for 6.11. Right?
> 
> Right. The best time is likely around Wed on the second week of the merge
> window, assuming all dependencies have landed. And it could be sent
> earlier if all the dependencies land sooner than that.
That makes sense. I have added a reminder to myself to do this when the
time comes. Feel free (@kees and @thomas) to scream at me if you see
that I might be forgetting :)

> 
> > 2. Is there a special way to send these treewide patches? Or is it just
> >    a regular PR with an explanation on why it is being done?
> 
> I would do a regular PR with all the details for Linus to do the change
> himself, but many times people send these as an explicit patch. For
> example, include the full Coccinelle script, or the "sed" command
> line, etc, and then detail any "by hand" changes that were needed on
> top of that.
@Thomas: have you sent the 11/11 patch on its own to the lists? I cant
find it in my history. Please send it as a stand-alone patch, so It can
go into sysctl just like the others.

> 
> > 3. Can you please send (if there are any) me any examples where this has
> >    been done in the past. Maybe some lore.kernel.org links?
> 
> I found this one that is a good example, though it's a PATCH not a GIT PULL:
> 
> https://lore.kernel.org/lkml/20221220134519.3dd1318b@gandalf.local.home/
> became
> https://git.kernel.org/linus/292a089d78d3e2f7944e60bb897c977785a321e3
> 

Thx for the help

Best

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Thomas Weißschuh]

On 2024-06-07 11:30:53+0000, Joel Granados wrote:
> On Thu, Jun 06, 2024 at 11:52:25AM -0700, Kees Cook wrote:
> > On Wed, Jun 05, 2024 at 10:26:25AM +0200, Joel Granados wrote:
> > > On Fri, May 31, 2024 at 09:31:24AM -0700, Kees Cook wrote:
> ...
> > > @kees: Since you have probably done these before, I'll ask you the
> > > questions:
> > > 
> > > 1. The idea is to send Linus the treewide-constify patch on its own at
> > >    the end of the merge window for 6.11. Right?
> > 
> > Right. The best time is likely around Wed on the second week of the merge
> > window, assuming all dependencies have landed. And it could be sent
> > earlier if all the dependencies land sooner than that.
> That makes sense. I have added a reminder to myself to do this when the
> time comes. Feel free (@kees and @thomas) to scream at me if you see
> that I might be forgetting :)
> 
> > 
> > > 2. Is there a special way to send these treewide patches? Or is it just
> > >    a regular PR with an explanation on why it is being done?
> > 
> > I would do a regular PR with all the details for Linus to do the change
> > himself, but many times people send these as an explicit patch. For
> > example, include the full Coccinelle script, or the "sed" command
> > line, etc, and then detail any "by hand" changes that were needed on
> > top of that.
> @Thomas: have you sent the 11/11 patch on its own to the lists? I cant
> find it in my history. Please send it as a stand-alone patch, so It can
> go into sysctl just like the others.

No, I didn't send it to the list on its own yet.
Do you want some changes or can I send it as-is?
(Plus the new motivational blurb)

> 
> > 
> > > 3. Can you please send (if there are any) me any examples where this has
> > >    been done in the past. Maybe some lore.kernel.org links?
> > 
> > I found this one that is a good example, though it's a PATCH not a GIT PULL:
> > 
> > https://lore.kernel.org/lkml/20221220134519.3dd1318b@gandalf.local.home/
> > became
> > https://git.kernel.org/linus/292a089d78d3e2f7944e60bb897c977785a321e3

Re: Current state of the sysctl constification effort

[Joel Granados]

On Fri, Jun 07, 2024 at 03:48:20PM +0200, Thomas Weißschuh wrote:
> On 2024-06-07 11:30:53+0000, Joel Granados wrote:
> > On Thu, Jun 06, 2024 at 11:52:25AM -0700, Kees Cook wrote:
> > > On Wed, Jun 05, 2024 at 10:26:25AM +0200, Joel Granados wrote:
> > > > On Fri, May 31, 2024 at 09:31:24AM -0700, Kees Cook wrote:
...
> > > 
> > > > 2. Is there a special way to send these treewide patches? Or is it just
> > > >    a regular PR with an explanation on why it is being done?
> > > 
> > > I would do a regular PR with all the details for Linus to do the change
> > > himself, but many times people send these as an explicit patch. For
> > > example, include the full Coccinelle script, or the "sed" command
> > > line, etc, and then detail any "by hand" changes that were needed on
> > > top of that.
> > @Thomas: have you sent the 11/11 patch on its own to the lists? I cant
> > find it in my history. Please send it as a stand-alone patch, so It can
> > go into sysctl just like the others.
> 
> No, I didn't send it to the list on its own yet.
> Do you want some changes or can I send it as-is?
> (Plus the new motivational blurb)

Please work on the commit message; no need to change the diff.

Here is more specific feedback on how to change the message in [1]

1. Say what was done in the first sentence. Something similar to this:
  "Add the const qualifier to the proc_handler function signatures to
  make clear...."

2. Include the general constification motivation. Something similar to
   this:
   "This patch is a prerequisite to moving all static ctl_talbe structs
   into .rodata which will reduce the attack surface in sysctl by
   ensuring that proc_handler function pointers cannot be changed."

3. No need to mention that this is to avoid lengthy transition. Please
   remove it from the commit. You can add it to the cover letter or just
   leave it out altogether. Up to you.

4. Please leave the cocci script. But I would be more specific on the
   rest of the changes. Something like this:

   "
   The patch was mostly generated by coccinelle with the following script:

       @@
       identifier func, ctl, write, buffer, lenp, ppos;
       @@

       int func(
       - struct ctl_table *ctl,
       + const struct ctl_table *ctl,
         int write, void *buffer, size_t *lenp, loff_t *ppos)
       { ... }

   In addition to the cocci changes:
   * Added a const qualifier to the ctl_table argument of the
     proc_handler typedef.

   * ... Change the others accordingly ...

   "

Best

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Joel Granados]

On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> Hi Joel, Hi Luis,
> 
> most of the sysctl handler preparation patches have been picked up by
> the subsystem maintainers and are available in -next.
> 
> Only two are missing:
> 
> * utsname: constify ctl_table arguments of utility function [0]
> * sysctl: constify ctl_table arguments of utility function [1]
> 
> Both of them are going through the sysctl tree anyways.
Indeed. These are going to go into sysctl-next as soon as I get around
to creating the branch (this week, if all goes well :)

> 
> With this done it should be possible to also queue up 
> sysctl: treewide: constify the ctl_table argument of handlers [2]
> for the bots to chew on in -next.
> 
> My local builds are still succeeding on the last submitted version of
> the patch.
Indeed, and I have also tested it in the kernel test bots through my
kernel.org repos. So this is no surprise. 

> 
> 
> Thomas
> 
> [0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-utsname-v1-1-27a6c8813620@weissschuh.net/
> [1] https://lore.kernel.org/lkml/20240513-jag-constfy_sysctl_proc_args-v1-1-bba870a480d5@samsung.com/
> [2] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-11-e0beccb836e2@weissschuh.net/

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Joel Granados]

On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> Hi Joel, Hi Luis,
> 
> most of the sysctl handler preparation patches have been picked up by
> the subsystem maintainers and are available in -next.
> 
> Only two are missing:
> 
> * utsname: constify ctl_table arguments of utility function [0]
> * sysctl: constify ctl_table arguments of utility function [1]
> 
> Both of them are going through the sysctl tree anyways.
> 
> With this done it should be possible to also queue up 
> sysctl: treewide: constify the ctl_table argument of handlers [2]
> for the bots to chew on in -next.
> 
> My local builds are still succeeding on the last submitted version of
> the patch.
> 
> 
> Thomas
> 
> [0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-utsname-v1-1-27a6c8813620@weissschuh.net/
> [1] https://lore.kernel.org/lkml/20240513-jag-constfy_sysctl_proc_args-v1-1-bba870a480d5@samsung.com/
> [2] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-11-e0beccb836e2@weissschuh.net/

All this is coming together nicely. Is there anything left to do besides
what is being discussed in this mail, to start changing the ctl_tables
to `static const`?

Best

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Thomas Weißschuh]

On 2024-06-07 11:40:53+0000, Joel Granados wrote:
> On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> > Hi Joel, Hi Luis,
> > 
> > most of the sysctl handler preparation patches have been picked up by
> > the subsystem maintainers and are available in -next.
> > 
> > Only two are missing:
> > 
> > * utsname: constify ctl_table arguments of utility function [0]
> > * sysctl: constify ctl_table arguments of utility function [1]
> > 
> > Both of them are going through the sysctl tree anyways.
> > 
> > With this done it should be possible to also queue up 
> > sysctl: treewide: constify the ctl_table argument of handlers [2]
> > for the bots to chew on in -next.
> > 
> > My local builds are still succeeding on the last submitted version of
> > the patch.
> > 
> > 
> > Thomas
> > 
> > [0] https://lore.kernel.org/lkml/20240518-sysctl-const-handler-utsname-v1-1-27a6c8813620@weissschuh.net/
> > [1] https://lore.kernel.org/lkml/20240513-jag-constfy_sysctl_proc_args-v1-1-bba870a480d5@samsung.com/
> > [2] https://lore.kernel.org/lkml/20240423-sysctl-const-handler-v3-11-e0beccb836e2@weissschuh.net/
> 
> All this is coming together nicely.

Indeed.

> Is there anything left to do besides
> what is being discussed in this mail, to start changing the ctl_tables
> to `static const`?

The changes to the tables also need (as per [0] and [1]):

* sysctl: move internal interfaces to const struct ctl_table
* sysctl: allow registration of const struct ctl_table

I think we do the handlers for v6.11, the rest of [0] and [1] for v6.12
and then we can go through the rest of the trees ctl_tables.


[0] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net/
[1] https://git.sr.ht/~t-8ch/linux/log/sysctl-constfy

Re: Current state of the sysctl constification effort

[Joel Granados]

On Fri, Jun 07, 2024 at 03:54:01PM +0200, Thomas Weißschuh wrote:
> On 2024-06-07 11:40:53+0000, Joel Granados wrote:
> > On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
...
> > Is there anything left to do besides
> > what is being discussed in this mail, to start changing the ctl_tables
> > to `static const`?
> 
> The changes to the tables also need (as per [0] and [1]):
> 
> * sysctl: move internal interfaces to const struct ctl_table
> * sysctl: allow registration of const struct ctl_table
> 
> I think we do the handlers for v6.11, the rest of [0] and [1] for v6.12
> and then we can go through the rest of the trees ctl_tables.

LGTM. Once you send "sysctl: move internal interfaces to const struct ctl_table" and
"sysctl: allow registration of const struct ctl_table", I'll put them
into sysctl-testing and have them there until they can go into sysctl-next
(after the end of the next merge window). Please send both of them in one
series and remember to work on the "what" and the "why" for the commit
messages and cover letter.

You can be inspired by this
"""
# Motivation
The reason we are constifying is:
1. It provides increased safety: Having things in .rodata section reduces the
   attack surface. This is especially relevant for structures that have function
   pointers (like ctl_table); having these in .rodata means that these pointers
   always point to the "intended" function and cannot be changed.
2. Readability: because it is easier to know up-front that data is not supposed
   to change or its obvious that a function is re-entrant. Actually a lot of the
   readability reasons is about knowing things "up-front".
As we move forward with the constification in sysctl, please include a more
detailed motivation in all your cover letters. This helps maintainers (that
don't have the context) understand what you are trying to do.
"""

Best

-- 

Joel Granados

Re: Current state of the sysctl constification effort

[Thomas Weißschuh]

On 2024-06-10 10:10:36+0000, Joel Granados wrote:
> On Fri, Jun 07, 2024 at 03:54:01PM +0200, Thomas Weißschuh wrote:
> > On 2024-06-07 11:40:53+0000, Joel Granados wrote:
> > > On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> ...
> > > Is there anything left to do besides
> > > what is being discussed in this mail, to start changing the ctl_tables
> > > to `static const`?
> > 
> > The changes to the tables also need (as per [0] and [1]):
> > 
> > * sysctl: move internal interfaces to const struct ctl_table
> > * sysctl: allow registration of const struct ctl_table
> > 
> > I think we do the handlers for v6.11, the rest of [0] and [1] for v6.12
> > and then we can go through the rest of the trees ctl_tables.
> 
> LGTM. Once you send "sysctl: move internal interfaces to const struct ctl_table" and
> "sysctl: allow registration of const struct ctl_table", I'll put them
> into sysctl-testing and have them there until they can go into sysctl-next
> (after the end of the next merge window). Please send both of them in one
> series and remember to work on the "what" and the "why" for the commit
> messages and cover letter.

IMO all of the remaining commits can go in as one series, as they all
belong to the sysctl core
(I can leave out "sysctl: constify standard sysctl tables")

FYI recent changes to the sysctl core introduced another prerequisite
for "sysctl: move internal interfaces to const struct ctl_table", which
is "bpf: Constify ctl_table argument of filter function".

See https://git.kernel.org/pub/scm/linux/kernel/git/thomas.weissschuh/linux.git/ sysctl-constfy


Thomas

Re: Current state of the sysctl constification effort

[Joel Granados]

On Sat, Jun 15, 2024 at 10:18:48PM +0200, Thomas Weißschuh wrote:
> On 2024-06-10 10:10:36+0000, Joel Granados wrote:
> > On Fri, Jun 07, 2024 at 03:54:01PM +0200, Thomas Weißschuh wrote:
> > > On 2024-06-07 11:40:53+0000, Joel Granados wrote:
> > > > On Fri, May 31, 2024 at 12:50:32PM +0200, Thomas Weißschuh wrote:
> > ...
> > > > Is there anything left to do besides
> > > > what is being discussed in this mail, to start changing the ctl_tables
> > > > to `static const`?
> > > 
> > > The changes to the tables also need (as per [0] and [1]):
> > > 
> > > * sysctl: move internal interfaces to const struct ctl_table
> > > * sysctl: allow registration of const struct ctl_table
> > > 
> > > I think we do the handlers for v6.11, the rest of [0] and [1] for v6.12
> > > and then we can go through the rest of the trees ctl_tables.
> > 
> > LGTM. Once you send "sysctl: move internal interfaces to const struct ctl_table" and
> > "sysctl: allow registration of const struct ctl_table", I'll put them
> > into sysctl-testing and have them there until they can go into sysctl-next
> > (after the end of the next merge window). Please send both of them in one
> > series and remember to work on the "what" and the "why" for the commit
> > messages and cover letter.
> 
> IMO all of the remaining commits can go in as one series, as they all
> belong to the sysctl core
This sounds good to me. If everything is related to sysctl core, then it
belongs in one series.

> (I can leave out "sysctl: constify standard sysctl tables")
This also sounds good. You can push this through when you are actually
changing all the static struct ctl_table occurances

> 
> FYI recent changes to the sysctl core introduced another prerequisite
> for "sysctl: move internal interfaces to const struct ctl_table", which
> is "bpf: Constify ctl_table argument of filter function".
> 
> See https://git.kernel.org/pub/scm/linux/kernel/git/thomas.weissschuh/linux.git/ sysctl-constfy
I see it. This looks like something related to BPF [STORAGE & CGROUPS]
(cc: martin.lau@linux.dev). We can push it ("bpf: Constify ctl_table
argument of filter function") through sysctl-next, if Martin is OK with
it. Otherwise you need to send a separate patch to bpf@vger.kernel.org.

Best

-- 

Joel Granados