Re: [PATCH RFC 0/7] sysctl: constify sysctl ctl_tables

From: Joel Granados <j.granados@samsung.com>
To: "Thomas Weißschuh" <linux@weissschuh.net>
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 RFC 0/7] sysctl: constify sysctl ctl_tables
Date: Mon, 4 Dec 2023 09:43:14 +0100	[thread overview]
Message-ID: <20231204084314.i7tvvejhlppdz5zd@localhost> (raw)
In-Reply-To: <e3932680-d284-4e13-9c0c-f202d588bf60@t-8ch.de>

[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]

Hey

I see that you sent a V2. I'll try to get to it at the end of the week.

On Sun, Dec 03, 2023 at 04:37:01PM +0100, Thomas Weißschuh wrote:
> Hi Joel,
> 
> On 2023-12-01 17:31:20+0100, Joel Granados wrote:
> > Hey Thomas.
> > 
> > Thx for the clarifications. I did more of a deep dive into your set and
> > have additional comments (in line). I think const-ing all this is a good
> > approach. The way forward is to be able to see the entire patch set of
> > changes in a V1 or a shared repo somewhere to have a better picture of
> > what is going on. By the "entire patchset" I mean all the changes that
> > you described in the "full process".
> 
> All the changes will be a lot. I don't think the incremental value to
> migrate all proc_handlers versus the work is useful for the discussion.
> I can however write up my proposed changes for the sysctl core properly
> and submit them as part of the next revision.
Looking forward to seeing them in V2

> 
> > On Tue, Nov 28, 2023 at 09:18:30AM +0100, Thomas Weißschuh wrote:
> > > Hi Joel,
> > > 
> > > On 2023-11-27 11:13:23+0100, Joel Granados wrote:
> > > > In general I would like to see more clarity with the motivation and I
> > > > would also expect some system testing. My comments inline:

<--- snip --->

> > is all sysctl code and cannot be chunked up because of dependencies,
> > then it should be ok to do it in one go.
> > 
> > > > > * Migrate all other sysctl handlers to proc_handler_new.
> > > > > * Drop the old proc_handler_field.
> > > > > * Fix the sysctl core to not modify the tables anymore.
> > > > > * Adapt public sysctl APIs to take "const struct ctl_table *".
> > > > > * Teach checkpatch.pl to warn on non-const "struct ctl_table"
> > > > >   definitions.
> 
> > Have you considered how to ignore the cases where the ctl_tables are
> > supposed to be non-const when they are defined (like in the network
> > code that we were discussing earlier)
> 
> As it would be a checkpatch warning it can be ignore while writing the
> patch and it won't trigger afterwards.
I mention coccinelle it is able to identify const vs non-const uses of
the ctl_table and only warn on the cases where it makes sense. This
would remove false negatives from pushing patches through.

> 
> > > > > * Migrate definitions of "struct ctl_table" to "const" where applicable.
> > These migrations are treewide and are usually reviewed by a wider
> > audience. You might need to chunk it up to make the review more palpable
> > for the other maintainers.
> 
> Ack.
> 
> > > > >  
> > > > > 
> > > > > Notes:
> > > > > 
> > > > > Just casting the function pointers around would trigger
> > > > > CFI (control flow integrity) warnings.
> > > > > 
> > > > > The name of the new handler "proc_handler_new" is a bit too long messing
> > > > > up the alignment of the table definitions.
> > > > > Maybe "proc_handler2" or "proc_handler_c" for (const) would be better.
> > > 
> > > > indeed the name does not say much. "_new" looses its meaning quite fast
> > > > :)
> > > 
> > > Hopefully somebody comes up with a better name!
> 
> > I would like to avoid this all together and just do add the const to the
> > existing "proc_handler"
> 
> Ack.
> 
> > > 
> > > > In my experience these tree wide modifications are quite tricky. Have you
> > > > run any tests to see that everything is as it was? sysctl selftests and
> > > > 0-day come to mind.
> > > 
> > > I managed to miss one change in my initial submission:
> > > With the hunk below selftests and typing emails work.
> > > 
> > > --- a/fs/proc/proc_sysctl.c
> > > +++ b/fs/proc/proc_sysctl.c
> > > @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header)
> > >                         else
> > >                                 err |= sysctl_check_table_array(path, entry);
> > >                 }
> > > -               if (!entry->proc_handler)
> > > +               if (!entry->proc_handler && !entry->proc_handler_new)
> > >                         err |= sysctl_err(path, entry, "No proc_handler");
> > >  
> > >                 if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode)
> > > 
> > > > [..]
> > > 
> > > [0] 43a7206b0963 ("driver core: class: make class_register() take a const *")
> > > [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/
> 
> Thanks for the feedback!
> 
> Thomas

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]