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/[email protected]/
--
Kees Cook
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/[email protected]/
>
> --
> Kees Cook
--
Joel Granados
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
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/[email protected]/
became
https://git.kernel.org/linus/292a089d78d3e2f7944e60bb897c977785a321e3
-Kees
--
Kees Cook
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/[email protected]/
> became
> https://git.kernel.org/linus/292a089d78d3e2f7944e60bb897c977785a321e3
>
Thx for the help
Best
--
Joel Granados
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/[email protected]/
> > became
> > https://git.kernel.org/linus/292a089d78d3e2f7944e60bb897c977785a321e3
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