From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Joel Granados <joel.granados@kernel.org>
Cc: Kees Cook <kees@kernel.org>, Luis Chamberlain <mcgrof@kernel.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] treewide: const qualify ctl_tables where applicable
Date: Thu, 9 Jan 2025 16:25:06 +0100 [thread overview]
Message-ID: <73f3c35b-35f1-433e-bd67-b428c3f2dd6e@t-8ch.de> (raw)
In-Reply-To: <20250109-jag-ctl_table_const-v1-1-622aea7230cf@kernel.org>
(Drop most recipients as it doesn't affect them)
On 2025-01-09 14:16:39+0100, Joel Granados wrote:
> Add the const qualifier to all the ctl_tables in the tree except the
> ones in ./net dir. The "net" sysctl code is special as it modifies the
> arrays before passing it 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.
>
> Created this by running an spatch followed by a sed command:
> Spatch:
> virtual patch
>
> @
> depends on !(file in "net")
> disable optional_qualifier
> @
> identifier table_name;
> @@
>
> + const
> struct ctl_table table_name [] = { ... };
>
> sed:
> sed --in-place \
> -e "s/struct ctl_table .table = &uts_kern/const struct ctl_table *table = \&uts_kern/" \
> kernel/utsname_sysctl.c
>
> Signed-off-by: Joel Granados <joel.granados@kernel.org>
> ---
> This treewide commit builds upon the work Thomas began a few releases
> ago [1], where he laid the groundwork for constifying ctl_tables. We
> implement constification throughout the tree, with the exception of the
> ctl_tables in the "net" directory. Those are special in that they treat
> the ctl_table as non-const but we can take them at a later point.
The modifications in net/ are from register_net_sysctl_sz(), correct?
Given that before modifying the table the code will already have called
WARN() and thereby tainted and most likely panicked the system.
If it is fine to crash the system I would argue it is also fine to just
fail to register the sysctl. Then the modification would not be
necessary anymore.
Something like (untested):
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 5a2a0df8ad91..5b0fff5fcaf9 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -496,12 +496,12 @@ struct ctl_table;
#ifdef CONFIG_SYSCTL
int net_sysctl_init(void);
struct ctl_table_header *register_net_sysctl_sz(struct net *net, const char *path,
- struct ctl_table *table, size_t table_size);
+ const struct ctl_table *table, size_t table_size);
void unregister_net_sysctl_table(struct ctl_table_header *header);
#else
static inline int net_sysctl_init(void) { return 0; }
static inline struct ctl_table_header *register_net_sysctl_sz(struct net *net,
- const char *path, struct ctl_table *table, size_t table_size)
+ const char *path, const struct ctl_table *table, size_t table_size)
{
return NULL;
}
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 19e8048241ba..754a3713eadb 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -120,10 +120,10 @@ __init int net_sysctl_init(void)
* data segment, and rather into the heap where a per-net object was
* allocated.
*/
-static void ensure_safe_net_sysctl(struct net *net, const char *path,
- struct ctl_table *table, size_t table_size)
+static bool ensure_safe_net_sysctl(struct net *net, const char *path,
+ const struct ctl_table *table, size_t table_size)
{
- struct ctl_table *ent;
+ const struct ctl_table *ent;
pr_debug("Registering net sysctl (net %p): %s\n", net, path);
ent = table;
@@ -155,18 +155,20 @@ static void ensure_safe_net_sysctl(struct net *net, const char *path,
WARN(1, "sysctl %s/%s: data points to %s global data: %ps\n",
path, ent->procname, where, ent->data);
- /* Make it "safe" by dropping writable perms */
- ent->mode &= ~0222;
+ return false;
}
+
+ return true;
}
struct ctl_table_header *register_net_sysctl_sz(struct net *net,
const char *path,
- struct ctl_table *table,
+ const struct ctl_table *table,
size_t table_size)
{
- if (!net_eq(net, &init_net))
- ensure_safe_net_sysctl(net, path, table, table_size);
+ if (!net_eq(net, &init_net) &&
+ !ensure_safe_net_sysctl(net, path, table, table_size))
+ return NULL;
return __register_sysctl_table(&net->sysctls, path, table, table_size);
}
> Upstreaming:
> ===========
> It is late in the release cycle, but I'm hopeful that we can get this
> in for the upcoming merge window and this is why:
> 1. We don't use linux-next: As with previous treewide changes similar to
> this one [1], we avoid using linux-next in order to avoid unwanted
> merge conflicts
> 2. This is a non-functional change: which lowers the probability of
> unforeseen errors or regressions.
> 3. It will have at least 2 weeks to be tested/reviewed: The PULL should
> be sent at the end of the merge window, giving it at least 2 weeks.
> And if there are more release candidates after rc6, there will be
> more time.
>
> Testing:
> ========
> 1. Currently being tested in 0-day
> 2. sysctl self-tests/kunit-tests
>
> Reduced To/Cc:
> ==============
> b4 originally gave me 200 ppl that this should go out to (which seems a
> bit overkill from my point of view). So I left the mailing lists and
> reduced the To: the ppl previously involved in the effort and sysctl
> maintainers. Please tell me if I missed someone important to the
> constification effort.
>
> Comments are greatly appreciated.
> Specially interested in any specifics from @Thomas and @kees
Looks good to me, not much (surprising) going on.
> Best
>
> [1] https://lore.kernel.org/20240724210014.mc6nima6cekgiukx@joelS2.panther.com
>
> --
> ---
> arch/arm/kernel/isa.c | 2 +-
> arch/arm64/kernel/fpsimd.c | 4 ++--
> arch/arm64/kernel/process.c | 2 +-
> arch/powerpc/kernel/idle.c | 2 +-
> arch/powerpc/platforms/pseries/mobility.c | 2 +-
> arch/riscv/kernel/process.c | 2 +-
> arch/riscv/kernel/vector.c | 2 +-
> arch/s390/appldata/appldata_base.c | 2 +-
> arch/s390/kernel/debug.c | 2 +-
> arch/s390/kernel/hiperdispatch.c | 2 +-
> arch/s390/kernel/topology.c | 2 +-
> arch/s390/mm/cmm.c | 2 +-
> arch/s390/mm/pgalloc.c | 2 +-
> arch/x86/entry/vdso/vdso32-setup.c | 2 +-
> arch/x86/kernel/cpu/bus_lock.c | 2 +-
> arch/x86/kernel/itmt.c | 2 +-
> crypto/fips.c | 2 +-
> drivers/base/firmware_loader/fallback_table.c | 2 +-
> drivers/cdrom/cdrom.c | 2 +-
> drivers/char/hpet.c | 2 +-
> drivers/char/ipmi/ipmi_poweroff.c | 2 +-
> drivers/char/random.c | 2 +-
> drivers/gpu/drm/i915/i915_perf.c | 2 +-
> drivers/gpu/drm/xe/xe_observation.c | 2 +-
> drivers/hv/hv_common.c | 2 +-
> drivers/infiniband/core/iwcm.c | 2 +-
> drivers/infiniband/core/ucma.c | 2 +-
> drivers/macintosh/mac_hid.c | 2 +-
> drivers/md/md.c | 2 +-
> drivers/misc/sgi-xp/xpc_main.c | 4 ++--
> drivers/perf/arm_pmuv3.c | 2 +-
> drivers/perf/riscv_pmu_sbi.c | 2 +-
> drivers/scsi/scsi_sysctl.c | 2 +-
> drivers/scsi/sg.c | 2 +-
> drivers/tty/tty_io.c | 2 +-
> drivers/xen/balloon.c | 2 +-
> fs/aio.c | 2 +-
> fs/cachefiles/error_inject.c | 2 +-
> fs/coda/sysctl.c | 2 +-
> fs/coredump.c | 2 +-
> fs/dcache.c | 2 +-
> fs/devpts/inode.c | 2 +-
> fs/eventpoll.c | 2 +-
> fs/exec.c | 2 +-
> fs/file_table.c | 2 +-
> fs/fuse/sysctl.c | 2 +-
> fs/inode.c | 2 +-
> fs/lockd/svc.c | 2 +-
> fs/locks.c | 2 +-
> fs/namei.c | 2 +-
> fs/namespace.c | 2 +-
> fs/nfs/nfs4sysctl.c | 2 +-
> fs/nfs/sysctl.c | 2 +-
> fs/notify/dnotify/dnotify.c | 2 +-
> fs/notify/fanotify/fanotify_user.c | 2 +-
> fs/notify/inotify/inotify_user.c | 2 +-
> fs/ocfs2/stackglue.c | 2 +-
> fs/pipe.c | 2 +-
> fs/quota/dquot.c | 2 +-
> fs/sysctls.c | 2 +-
> fs/userfaultfd.c | 2 +-
> fs/verity/init.c | 2 +-
> fs/xfs/xfs_sysctl.c | 2 +-
> init/do_mounts_initrd.c | 2 +-
> io_uring/io_uring.c | 2 +-
> ipc/ipc_sysctl.c | 2 +-
> ipc/mq_sysctl.c | 2 +-
> kernel/acct.c | 2 +-
> kernel/bpf/syscall.c | 2 +-
> kernel/delayacct.c | 2 +-
> kernel/exit.c | 2 +-
> kernel/hung_task.c | 2 +-
> kernel/kexec_core.c | 2 +-
> kernel/kprobes.c | 2 +-
> kernel/latencytop.c | 2 +-
> kernel/locking/lockdep.c | 2 +-
> kernel/panic.c | 2 +-
> kernel/pid_namespace.c | 2 +-
> kernel/pid_sysctl.h | 2 +-
> kernel/printk/sysctl.c | 2 +-
> kernel/reboot.c | 2 +-
> kernel/sched/autogroup.c | 2 +-
> kernel/sched/core.c | 2 +-
> kernel/sched/deadline.c | 2 +-
> kernel/sched/fair.c | 2 +-
> kernel/sched/rt.c | 2 +-
> kernel/sched/topology.c | 2 +-
> kernel/seccomp.c | 2 +-
> kernel/signal.c | 2 +-
> kernel/stackleak.c | 2 +-
> kernel/sysctl-test.c | 6 +++---
> kernel/sysctl.c | 4 ++--
> kernel/time/timer.c | 2 +-
> kernel/trace/ftrace.c | 2 +-
> kernel/trace/trace_events_user.c | 2 +-
> kernel/umh.c | 2 +-
> kernel/utsname_sysctl.c | 4 ++--
> kernel/watchdog.c | 4 ++--
> lib/alloc_tag.c | 2 +-
> lib/test_sysctl.c | 6 +++---
> mm/compaction.c | 2 +-
> mm/hugetlb.c | 2 +-
> mm/hugetlb_vmemmap.c | 2 +-
> mm/memory-failure.c | 2 +-
> mm/oom_kill.c | 2 +-
> mm/page-writeback.c | 2 +-
> mm/page_alloc.c | 2 +-
> security/apparmor/lsm.c | 2 +-
> security/keys/sysctl.c | 2 +-
> security/loadpin/loadpin.c | 2 +-
> security/yama/yama_lsm.c | 2 +-
> 111 files changed, 120 insertions(+), 120 deletions(-)
next prev parent reply other threads:[~2025-01-09 15:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-09 13:16 Joel Granados
2025-01-09 14:50 ` Steven Rostedt
2025-01-09 14:58 ` Jani Nikula
2025-01-09 15:00 ` Martin K. Petersen
2025-01-09 15:25 ` Thomas Weißschuh [this message]
2025-01-09 16:55 ` Kees Cook
2025-01-10 14:12 ` Joel Granados
2025-01-09 15:45 ` Corey Minyard
2025-01-09 15:51 ` Darrick J. Wong
2025-01-09 18:05 ` Song Liu
[not found] ` <501b718d1bd046bf3eb1932b11627e4cec6194f992124722840145bed109ac0a@mail.kernel.org>
[not found] ` <fnrup4bdfgoeiy3nfsl56juhqjrnstntf4fg5jrcdxv226ehqp@alrqzkyfw6sl>
2025-01-10 17:05 ` Daniel Xu
2025-01-13 8:44 ` Joel Granados
[not found] ` <Z4TnZXCjgCIGEfsR@pathway.suse.cz>
[not found] ` <42gh2e7ckwjcccorbrapeos42n4yr22koqotsu7qithb7cxl6x@ynn6yr5h7dtb>
2025-02-18 10:35 ` Joel Granados
2025-01-10 17:13 ` Dixit, Ashutosh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=73f3c35b-35f1-433e-bd67-b428c3f2dd6e@t-8ch.de \
--to=linux@weissschuh.net \
--cc=joel.granados@kernel.org \
--cc=kees@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®