Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

From: Joel Granados <j.granados@samsung.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "Dave Chinner" <david@fromorbit.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	"Eric Dumazet" <edumazet@google.com>,
	linux-hardening@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-s390@vger.kernel.org, rds-devel@oss.oracle.com,
	linux-rdma@vger.kernel.org,
	"Luis Chamberlain" <mcgrof@kernel.org>,
	linux-sctp@vger.kernel.org, lvs-devel@vger.kernel.org,
	coreteam@netfilter.org, linux-trace-kernel@vger.kernel.org,
	"Kees Cook" <keescook@chromium.org>,
	bridge@lists.linux.dev, apparmor@lists.ubuntu.com,
	linux-xfs@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-nfs@vger.kernel.org, netdev@vger.kernel.org,
	kexec@lists.infradead.org,
	"Thomas Weißschuh" <linux@weissschuh.net>,
	linux-perf-users@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	netfilter-devel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	bpf@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
Date: Thu, 25 Apr 2024 13:04:12 +0200	[thread overview]
Message-ID: <20240425110412.2n5d27smecfncsfa@joelS2.panther.com> (raw)
In-Reply-To: <20240424201234.3cc2b509@kernel.org>

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

On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote:
> On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote:
> > The series was split from my larger series sysctl-const series [0].
> > It only focusses on the proc_handlers but is an important step to be
> > able to move all static definitions of ctl_table into .rodata.
> 
> Split this per subsystem, please.
It is tricky to do that because it changes the first argument (ctl*) to
const in the proc_handler function type defined in sysclt.h:
"
-typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer,
+typedef int proc_handler(const struct ctl_table *ctl, int write, void *buffer,
                size_t *lenp, loff_t *ppos);
"
This means that all the proc_handlers need to change at the same time.

However, there is an alternative way to do this that allows chunking. We
first define the proc_handler as a void pointer (casting it where it is
being used) [1]. Then we could do the constification by subsystem (like
Jakub proposes). Finally we can "revert the void pointer change so we
don't have one size fit all pointer as our proc_handler [2].

Here are some comments about the alternative:
1. We would need to make the first argument const in all the derived
   proc_handlers [3] 
2. There would be no undefined behavior for two reasons:
   2.1. There is no case where we change the first argument. We know
        this because there are no compile errors after we make it const.
   2.2. We would always go from non-const to const. This is the case
        because all the stuff that is unchanged in non-const.
3. If the idea sticks, it should go into mainline as one patchset. I
   would not like to have a void* proc_handler in a kernel release.
4. I think this is a "win/win" solution were the constification goes
   through and it is divided in such a way that it is reviewable.

I would really like to hear what ppl think about this "heretic"
alternative. @Thomas, @Luis, @Kees @Jakub?

Best

[1] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=4a383503b1ea650d4e12c1f5838974e879f5aa6f
[2] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=a3be65973d27ec2933b9e81e1bec60be3a9b460d
[3] proc_dostring, proc_dobool, proc_dointvec....

--

Joel Granados

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