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

From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Luis Chamberlain <mcgrof@kernel.org>,
	 Julia Lawall <julia.lawall@inria.fr>
Cc: Joel Granados <j.granados@samsung.com>,
	 Dan Carpenter <dan.carpenter@linaro.org>,
	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: Tue, 19 Dec 2023 20:29:50 +0100	[thread overview]
Message-ID: <a0d96e7b-544f-42d5-b8da-85bc4ca087a9@t-8ch.de> (raw)
In-Reply-To: <ZYC37Vco1p4vD8ji@bombadil.infradead.org>

Hi Luis and Julia,

(Julia, there is a question and context for you inline, marked with your name)

On 2023-12-18 13:21:49-0800, Luis Chamberlain wrote:
> So we can split this up concentually in two:
> 
>  * constificaiton of the table handlers
>  * constification of the table struct itself
> 
> On Sun, Dec 17, 2023 at 11:10:15PM +0100, Thomas Weißschuh wrote:
> > The handlers can already be made const as shown in this series,
> 
> The series did already produce issues with some builds, and so
> Julia's point is confirmed that the series only proves hanlders
> which you did build and for which 0-day has coverage for.
> 
> The challenge here was to see if we could draw up a test case
> that would prove this without build tests, and what occurred to
> me was coccinelle or smatch.

I used the following coccinelle script to find more handlers that I
missed before:

virtual patch
virtual context
virtual report

@@
identifier func;
identifier ctl;
identifier write;
identifier buffer;
identifier lenp;
identifier ppos;
type loff_t;
@@

int func(
- struct ctl_table *ctl,
+ const struct ctl_table *ctl,
  int write, void *buffer, size_t *lenp, loff_t *ppos) { ... }

It did not find any additional occurrences while it was able to match
the existing changes.

After that I manually reviewed all handlers that they are not modifying
their table argument, which they don't.

Should we do more?

For Julia:

Maybe you could advise on how to use coccinelle to find where a const
function argument or one of its members are modified directly or passed
to some other function as non-const arguments.
See the coccinelle patch above.

Is this possible?

> > > 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.
> 
> Instead of "croping up" at build time again, I wonder if we can do
> better with coccinelle / smatch.

As for smatch:

Doesn't smatch itself run as part of a normal build [0]?
So it would have the same visibility issues as the compiler itself.

> Joel, and yes, what you described is what I was suggesting, that is to
> avoid having to add a non-const handler a first step, instead we modify
> those callers which do require to modify the table by first a
> deregistration and later a registration. In fact to make this even
> easier a new call would be nice so to aslo be able to git grep when
> this is done in the kernel.
> 
> But if what you suggest is true that there are no registrations which
> later modify the table, we don't need that. It is the uncertainty that
> we might have that this is a true statment that I wanted to challenge
> to see if we could do better. Can we avoid this being a stupid
> regression later by doing code analysis with coccinelle / smatch?
> 
> The template of the above endeavor seems useful not only to this use
> case but to any place in the kernel where this previously has been done
> before, and hence my suggestion that this seems like a sensible thing
> to think over to see if we could generalize.

I'd like to split the series and submit the part up until and including
the constification of arguments first and on its own.
It keeps the subsystem maintainers out of the discussion of the core
sysctl changes.

I'll submit the core sysctl changes after I figure out proper responses
to all review comments and we can do this in parallel to the tree-wide
preparation.

What do you think Luis and Joel?

[0] https://repo.or.cz/smatch.git/blob/HEAD:/smatch_scripts/test_kernel.sh