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: Luis Chamberlain <mcgrof@kernel.org>,
	 Dan Carpenter <dan.carpenter@linaro.org>,
	Julia Lawall <julia.lawall@inria.fr>,
	 Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <gustavoars@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: Sun, 17 Dec 2023 23:10:15 +0100	[thread overview]
Message-ID: <908dc370-7cf6-4b2b-b7c9-066779bc48eb@t-8ch.de> (raw)
In-Reply-To: <20231217120201.z4gr3ksjd4ai2nlk@localhost>

On 2023-12-17 13:02:01+0100, Joel Granados wrote:
> Catching up with mail....
> 
> On Tue, Dec 12, 2023 at 11:51:30PM -0800, Luis Chamberlain wrote:
> > On Tue, Dec 12, 2023 at 10:09:30AM +0100, Joel Granados wrote:
> > > My idea was to do something similar to your originl RFC, where you have
> > > an temporary proc_handler something like proc_hdlr_const (we would need
> > > to work on the name) and move each subsystem to the new handler while
> > > the others stay with the non-const one. At the end, the old proc_handler
> > > function name would disapear and would be completely replaced by the new
> > > proc_hdlr_const.
> > >
> > > This is of course extra work and might not be worth it if you don't get
> > > negative feedback related to tree-wide changes. Therefore I stick to my
> > > previous suggestion. Send the big tree-wide patches and only explore
> > > this option if someone screams.
> >
> > I think we can do better, can't we just increase confidence in that we
> > don't *need* muttable ctl_cables with something like smatch or
> > coccinelle so that we can just make them const?
> >
> > Seems like a noble endeavor for us to generalize.
> >
> > Then we just breeze through by first fixing those that *are* using
> > mutable tables by having it just de-register and then re-register
> So let me see if I understand your {de,re}-register idea:
> When we have a situation (like in the networking code) where a ctl_table
> is being used in an unmuttable way, we do your {de,re}-register trick.

unmuttable?

> The trick consists in unregistering an old ctl_table and reregistering
> with a whole new const changed table. In this way, whatever we register
> is always const.
> 
> Once we address all the places where this happens, then we just change
> the handler to const and we are done.
> 
> Is that correct?

I'm confused.

The handlers can already be made const as shown in this series, which
does convert the whole kernel tree.
There is only one handler (the stackleak one) which modifies the table
and this one is fixed as part of the series.

(Plus the changes needed to the sysctl core to avoid mutation there)

> If that is indeed what you are proposing, you might not even need the
> un-register step as all the mutability that I have seen occurs before
> the register. So maybe instead of re-registering it, you can so a copy
> (of the changed ctl_table) to a const pointer and then pass that along
> to the register function.

Tables that are modified, but *not* through the handler, would crop
during the constification of the table structs.
Which should be a second step.

But Luis' message was not completely clear to me.
I guess I'm missing something.

> Can't think of anything else off the top of my head. Would have to
> actually see the code to evaluate further I think.
> 
> > new tables if they need to be changed, and then a new series is sent
> > once we fix all those muttable tables.

Thomas