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

From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Joel Granados <j.granados@samsung.com>
Cc: Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables
Date: Thu, 7 Dec 2023 20:19:43 +0100	[thread overview]
Message-ID: <fa911908-a14d-4746-a58e-caa7e1d4b8d4@t-8ch.de> (raw)
In-Reply-To: <20231207104357.kndqvzkhxqkwkkjo@localhost>

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