Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables

[Joel Granados]

On Thu, Dec 07, 2023 at 08:19:43PM +0100, Thomas Weißschuh wrote:
> On 2023-12-07 11:43:57+0100, Joel Granados wrote:
> > Hey Thomas
> > 
> > You have a couple of test bot issues for your 12/18 patch. Can you
> > please address those for your next version.
> 
> I have these fixed locally, I assumed Luis would also pick them up
> directly until I have a proper v2, properly should have communicated
> that.
> 
> > On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote:
> > > Problem description:
> > > 
> > > The kernel contains a lot of struct ctl_table throught the tree.
> > > These are very often 'static' definitions.
> > > It would be good to make the tables unmodifiable by marking them "const"
> > Here I would remove "It would be good to". Just state it: "Make the
> > tables unmodifiable...."
> 
> Ack.
> 
> > 
> > > to avoid accidental or malicious modifications.
> > > This is in line with a general effort to move as much data as possible
> > > into .rodata. (See for example[0] and [1])
> 
> > If you could find more examples, it would make a better case.
> 
> I'll look for some. So far my constifications went in without them :-)
> 
> > > 
> > > Unfortunately the tables can not be made const right now because the
> > > core registration functions expect mutable tables.
> > > 
> > > This is for two main reasons:
> > > 
> > > 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify
> > >    the table.
> > > 2) The table is passed to the handler function as a non-const pointer.
> > > 
> > > This series migrates the core and all handlers.
> 
> > awesome!
> > 
> > > 
> > > Structure of the series:
> > > 
> > > Patch 1-3:   Cleanup patches
> > > Patch 4-7:   Non-logic preparation patches
> > > Patch 8:     Preparation patch changing a bit of logic
> > > Patch 9-12:  Treewide changes to handler function signature
> > > Patch 13-14: Adaption of the sysctl core implementation
> > > Patch 15:    Adaption of the sysctl core interface
> > > Patch 16:    New entry for checkpatch
> > > Patch 17-18: Constification of existing "struct ctl_table"s
> > > 
> > > Tested by booting and with the sysctl selftests on x86.
> > > 
> > > Note:
> > > 
> > > This is intentionally sent only to a small number of people as I'd like
> > > to get some more sysctl core-maintainer feedback before sending this to
> > > essentially everybody.
> 
> > When you do send it to the broader audience, you should chunk up your big
> > patches (12/18 and 11/18) and this is why:
> > 1. To avoid mail rejections from lists:
> >    You have to tell a lot of people about the changes in one mail. That
> >    will make mail header too big for some lists and it will be rejected.
> >    This happened to me with [3]
> > 2. Avoid being rejected for the wrong reasons :)
> >    Maintainers are busy ppl and sending them a set with so many files
> >    may elicit a rejection on the grounds that it involves too many
> >    subsystems at the same time.
> > I suggest you chunk it up with directories in mind. Something similar to
> > what I did for [4] where I divided stuff that when for fs/*, kernel/*,
> > net/*, arch/* and drivers/*. That will complicate your patch a tad
> > because you have to ensure that the tree can be compiled/run for every
> > commit. But it will pay off once you push it to the broader public.
> 
> This will break bisections. All function signatures need to be switched
I was suggesting a solution without breaking bisections of course. I can
think of a couple of ways to do this in chunks but it might be
premature. You can send it and if you get push back because of this then
we can deal with chunking it down.

I'm still concerned about the header size for those mails. How does the
mail look like when you run the get maintainers script on it?

> in one step. I would strongly like to avoid introducing broken commits.
> 
> The fact that these big commits have no functional changes at all makes
> me hope it can work.

-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature