Re: Re: [PATCH v2] treewide: const qualify ctl_tables where applicable
From: Joel Granados <joel.granados@kernel.org>
Date: 2025-01-22 12:25:51
Also in:
bpf, dri-devel, intel-gfx, intel-xe, io-uring, kexec, keyrings, linux-arm-kernel, linux-crypto, linux-fsdevel, linux-hardening, linux-hyperv, linux-mm, linux-nfs, linux-raid, linux-rdma, linux-riscv, linux-s390, linux-scsi, linux-security-module, linux-serial, linux-trace-kernel, linux-xfs, lkml, netfs, ocfs2-devel, xen-devel
On Tue, Jan 21, 2025 at 02:40:16PM +0100, Alexander Gordeev wrote:
On Fri, Jan 10, 2025 at 03:16:08PM +0100, Joel Granados wrote: Hi Joel,quoted
Add the const qualifier to all the ctl_tables in the tree except for watchdog_hardlockup_sysctl, memory_allocation_profiling_sysctls, loadpin_sysctl_table and the ones calling register_net_sysctl (./net, drivers/inifiniband dirs). These are special cases as they use a registration function with a non-const qualified ctl_table argument or modify the arrays before passing them on to the registration function. Constifying ctl_table structs will prevent the modification of proc_handler function pointers as the arrays would reside in .rodata. This is made possible after commit 78eb4ea25cd5 ("sysctl: treewide: constify the ctl_table argument of proc_handlers") constified all the proc_handlers.I could identify at least these occurences in s390 code as well:
Hey Alexander
Thx for bringing these to my attention. I had completely missed them as
the spatch only deals with ctl_tables outside functions.
Short answer:
These should not be included in the current patch because they are a
different pattern from how sysctl tables are usually used. So I will not
include them.
With that said, I think it might be interesting to look closer at them
as they seem to be complicating the proc_handler (I have to look at them
closer).
I see that they are defining a ctl_table struct within the functions and
just using the data (from the incoming ctl_table) to forward things down
to proc_do{u,}intvec_* functions. This is very odd and I have only seen
it done in order to change the incoming ctl_table (which is not what is
being done here).
I will take a closer look after the merge window and circle back with
more info. Might take me a while as I'm not very familiar with s390
code; any additional information on why those are being used inside the
functions would be helpfull.
Best
quoted hunk ↗ jump to hunk
diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c index dd7ba7587dd5..9b83c318f919 100644 --- a/arch/s390/appldata/appldata_base.c +++ b/arch/s390/appldata/appldata_base.c@@ -204,7 +204,7 @@ appldata_timer_handler(const struct ctl_table *ctl, int write, { int timer_active = appldata_timer_active; int rc; - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &timer_active, .maxlen = sizeof(int),@@ -237,7 +237,7 @@ appldata_interval_handler(const struct ctl_table *ctl, int write, { int interval = appldata_interval; int rc; - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &interval, .maxlen = sizeof(int),@@ -269,7 +269,7 @@ appldata_generic_handler(const struct ctl_table *ctl, int write, struct list_head *lh; int rc, found; int active; - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .data = &active, .maxlen = sizeof(int), .extra1 = SYSCTL_ZERO,diff --git a/arch/s390/kernel/hiperdispatch.c b/arch/s390/kernel/hiperdispatch.c index 7857a7e8e56c..7d0ba16085c1 100644 --- a/arch/s390/kernel/hiperdispatch.c +++ b/arch/s390/kernel/hiperdispatch.c@@ -273,7 +273,7 @@ static int hiperdispatch_ctl_handler(const struct ctl_table *ctl, int write, { int hiperdispatch; int rc; - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &hiperdispatch, .maxlen = sizeof(int),diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c index 6691808bf50a..26e50de83d80 100644 --- a/arch/s390/kernel/topology.c +++ b/arch/s390/kernel/topology.c@@ -629,7 +629,7 @@ static int topology_ctl_handler(const struct ctl_table *ctl, int write, int enabled = topology_is_enabled(); int new_mode; int rc; - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &enabled, .maxlen = sizeof(int),@@ -658,7 +658,7 @@ static int polarization_ctl_handler(const struct ctl_table *ctl, int write, { int polarization; int rc; - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &polarization, .maxlen = sizeof(int),diff --git a/arch/s390/mm/cmm.c b/arch/s390/mm/cmm.c index 939e3bec2db7..8e354c90a3dd 100644 --- a/arch/s390/mm/cmm.c +++ b/arch/s390/mm/cmm.c@@ -263,7 +263,7 @@ static int cmm_pages_handler(const struct ctl_table *ctl, int write, void *buffer, size_t *lenp, loff_t *ppos) { long nr = cmm_get_pages(); - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &nr, .maxlen = sizeof(long),@@ -283,7 +283,7 @@ static int cmm_timed_pages_handler(const struct ctl_table *ctl, int write, loff_t *ppos) { long nr = cmm_get_timed_pages(); - struct ctl_table ctl_entry = { + const struct ctl_table ctl_entry = { .procname = ctl->procname, .data = &nr, .maxlen = sizeof(long),quoted
Best regards, -- Joel Granados [off-list ref]Thanks!
-- Joel Granados