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

From: Joel Granados
Date: Thu Dec 07 2023 - 05:44:13 EST

Next message: Stanley Chang[昌育德]: "RE: [PATCH v3 RESEND 4/4] usb: core: add phy notify connect and disconnect"
Previous message: Yuanhan Zhang: "Re: [PATCH v3] sched/cputime: let ktimers align with ksoftirqd in accounting CPUTIME_SOFTIRQ"
In reply to: Joel Granados: "Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables"
Next in thread: Thomas Weißschuh: "Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hey Thomas

You have a couple of test bot issues for your 12/18 patch. Can you
please address those for your next version.

On Mon, Dec 04, 2023 at 08:52:13AM +0100, Thomas Weißschuh wrote:
> Problem description:
>
> The kernel contains a lot of struct ctl_table throught the tree.
> These are very often 'static' definitions.
> It would be good to make the tables unmodifiable by marking them "const"
Here I would remove "It would be good to". Just state it: "Make the
tables unmodifiable...."

> to avoid accidental or malicious modifications.
> This is in line with a general effort to move as much data as possible
> into .rodata. (See for example[0] and [1])
If you could find more examples, it would make a better case.

>
> Unfortunately the tables can not be made const right now because the
> core registration functions expect mutable tables.
>
> This is for two main reasons:
>
> 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify
> the table.
> 2) The table is passed to the handler function as a non-const pointer.
>
> This series migrates the core and all handlers.
awesome!

>
> Structure of the series:
>
> Patch 1-3: Cleanup patches
> Patch 4-7: Non-logic preparation patches
> Patch 8: Preparation patch changing a bit of logic
> Patch 9-12: Treewide changes to handler function signature
> Patch 13-14: Adaption of the sysctl core implementation
> Patch 15: Adaption of the sysctl core interface
> Patch 16: New entry for checkpatch
> Patch 17-18: Constification of existing "struct ctl_table"s
>
> Tested by booting and with the sysctl selftests on x86.
>
> Note:
>
> This is intentionally sent only to a small number of people as I'd like
> to get some more sysctl core-maintainer feedback before sending this to
> essentially everybody.
When you do send it to the broader audience, you should chunk up your big
patches (12/18 and 11/18) and this is why:
1. To avoid mail rejections from lists:
You have to tell a lot of people about the changes in one mail. That
will make mail header too big for some lists and it will be rejected.
This happened to me with [3]
2. Avoid being rejected for the wrong reasons :)
Maintainers are busy ppl and sending them a set with so many files
may elicit a rejection on the grounds that it involves too many
subsystems at the same time.
I suggest you chunk it up with directories in mind. Something similar to
what I did for [4] where I divided stuff that when for fs/*, kernel/*,
net/*, arch/* and drivers/*. That will complicate your patch a tad
because you have to ensure that the tree can be compiled/run for every
commit. But it will pay off once you push it to the broader public.

[3] https://lore.kernel.org/all/20230621091000.424843-1-j.granados@xxxxxxxxxxx
[4] https://lore.kernel.org/all/20230726140635.2059334-1-j.granados@xxxxxxxxxxx
>
> [0] 43a7206b0963 ("driver core: class: make class_register() take a const *")
> [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@xxxxxxxxx/
>
> ---
> Changes in v2:
> - Migrate all handlers.
> - Remove intermediate "proc_handler_new" step (Thanks Joel).
> - Drop RFC status.
> - Prepare other parts of the tree.
> - Link to v1: https://lore.kernel.org/r/20231125-const-sysctl-v1-0-5e881b0e0290@xxxxxxxxxxxxxx
>
> ---
> Thomas Weißschuh (18):
> watchdog/core: remove sysctl handlers from public header
> sysctl: delete unused define SYSCTL_PERM_EMPTY_DIR
> sysctl: drop sysctl_is_perm_empty_ctl_table
> cgroup: bpf: constify ctl_table arguments and fields
> seccomp: constify ctl_table arguments of utility functions
> hugetlb: constify ctl_table arguments of utility functions
> utsname: constify ctl_table arguments of utility function
> stackleak: don't modify ctl_table argument
> sysctl: treewide: constify ctl_table_root::set_ownership
> sysctl: treewide: constify ctl_table_root::permissions
> sysctl: treewide: constify ctl_table_header::ctl_table_arg
> sysctl: treewide: constify the ctl_table argument of handlers
> sysctl: move sysctl type to ctl_table_header
> sysctl: move internal interfaces to const struct ctl_table
> sysctl: allow registration of const struct ctl_table
> const_structs.checkpatch: add ctl_table
> sysctl: make ctl_table sysctl_mount_point const
> sysctl: constify standard sysctl tables
>
> arch/arm64/kernel/armv8_deprecated.c | 2 +-
> arch/arm64/kernel/fpsimd.c | 2 +-
> arch/s390/appldata/appldata_base.c | 8 +--
> arch/s390/kernel/debug.c | 2 +-
> arch/s390/kernel/topology.c | 2 +-
> arch/s390/mm/cmm.c | 6 +-
> arch/x86/kernel/itmt.c | 2 +-
> drivers/cdrom/cdrom.c | 4 +-
> drivers/char/random.c | 4 +-
> drivers/macintosh/mac_hid.c | 2 +-
> drivers/net/vrf.c | 4 +-
> drivers/parport/procfs.c | 12 ++--
> fs/coredump.c | 2 +-
> fs/dcache.c | 4 +-
> fs/drop_caches.c | 2 +-
> fs/exec.c | 4 +-
> fs/file_table.c | 2 +-
> fs/fs-writeback.c | 2 +-
> fs/inode.c | 4 +-
> fs/pipe.c | 2 +-
> fs/proc/internal.h | 2 +-
> fs/proc/proc_sysctl.c | 102 +++++++++++++++---------------
> fs/quota/dquot.c | 2 +-
> fs/xfs/xfs_sysctl.c | 6 +-
> include/linux/bpf-cgroup.h | 2 +-
> include/linux/filter.h | 2 +-
> include/linux/ftrace.h | 4 +-
> include/linux/mm.h | 8 +--
> include/linux/nmi.h | 7 --
> include/linux/perf_event.h | 6 +-
> include/linux/security.h | 2 +-
> include/linux/sysctl.h | 78 +++++++++++------------
> include/linux/vmstat.h | 6 +-
> include/linux/writeback.h | 2 +-
> include/net/ndisc.h | 2 +-
> include/net/neighbour.h | 6 +-
> include/net/netfilter/nf_hooks_lwtunnel.h | 2 +-
> ipc/ipc_sysctl.c | 12 ++--
> ipc/mq_sysctl.c | 2 +-
> kernel/bpf/cgroup.c | 2 +-
> kernel/bpf/syscall.c | 4 +-
> kernel/delayacct.c | 4 +-
> kernel/events/callchain.c | 2 +-
> kernel/events/core.c | 4 +-
> kernel/fork.c | 2 +-
> kernel/hung_task.c | 4 +-
> kernel/kexec_core.c | 2 +-
> kernel/kprobes.c | 2 +-
> kernel/latencytop.c | 4 +-
> kernel/pid_namespace.c | 2 +-
> kernel/pid_sysctl.h | 2 +-
> kernel/printk/internal.h | 2 +-
> kernel/printk/printk.c | 2 +-
> kernel/printk/sysctl.c | 5 +-
> kernel/sched/core.c | 8 +--
> kernel/sched/rt.c | 12 ++--
> kernel/sched/topology.c | 2 +-
> kernel/seccomp.c | 8 +--
> kernel/stackleak.c | 9 +--
> kernel/sysctl.c | 84 ++++++++++++------------
> kernel/time/timer.c | 2 +-
> kernel/trace/ftrace.c | 2 +-
> kernel/trace/trace.c | 2 +-
> kernel/trace/trace_events_user.c | 2 +-
> kernel/trace/trace_stack.c | 2 +-
> kernel/ucount.c | 4 +-
> kernel/umh.c | 2 +-
> kernel/utsname_sysctl.c | 4 +-
> kernel/watchdog.c | 15 +++--
> mm/compaction.c | 8 +--
> mm/hugetlb.c | 10 +--
> mm/page-writeback.c | 18 +++---
> mm/page_alloc.c | 22 +++----
> mm/util.c | 12 ++--
> mm/vmstat.c | 4 +-
> net/ax25/sysctl_net_ax25.c | 2 +-
> net/bridge/br_netfilter_hooks.c | 6 +-
> net/core/neighbour.c | 24 +++----
> net/core/sysctl_net_core.c | 22 +++----
> net/ieee802154/6lowpan/reassembly.c | 2 +-
> net/ipv4/devinet.c | 8 +--
> net/ipv4/ip_fragment.c | 2 +-
> net/ipv4/route.c | 4 +-
> net/ipv4/sysctl_net_ipv4.c | 35 +++++-----
> net/ipv4/xfrm4_policy.c | 2 +-
> net/ipv6/addrconf.c | 29 +++++----
> net/ipv6/ndisc.c | 4 +-
> net/ipv6/netfilter/nf_conntrack_reasm.c | 2 +-
> net/ipv6/reassembly.c | 2 +-
> net/ipv6/route.c | 2 +-
> net/ipv6/sysctl_net_ipv6.c | 10 +--
> net/ipv6/xfrm6_policy.c | 2 +-
> net/mpls/af_mpls.c | 8 +--
> net/mptcp/ctrl.c | 2 +-
> net/netfilter/ipvs/ip_vs_ctl.c | 16 ++---
> net/netfilter/nf_conntrack_standalone.c | 4 +-
> net/netfilter/nf_hooks_lwtunnel.c | 2 +-
> net/netfilter/nf_log.c | 4 +-
> net/phonet/sysctl.c | 2 +-
> net/rds/tcp.c | 4 +-
> net/sctp/sysctl.c | 30 ++++-----
> net/smc/smc_sysctl.c | 2 +-
> net/sunrpc/sysctl.c | 6 +-
> net/sunrpc/xprtrdma/svc_rdma.c | 2 +-
> net/sysctl_net.c | 4 +-
> net/unix/sysctl_net_unix.c | 2 +-
> net/xfrm/xfrm_sysctl.c | 2 +-
> scripts/const_structs.checkpatch | 1 +
> security/apparmor/lsm.c | 2 +-
> security/min_addr.c | 2 +-
> security/yama/yama_lsm.c | 2 +-
> 111 files changed, 427 insertions(+), 428 deletions(-)
> ---
> base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a
> change-id: 20231116-const-sysctl-e14624f1295c
>
> Best regards,
> --
> Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
>

--

Joel Granados

Attachment: signature.asc
Description: PGP signature

Next message: Stanley Chang[昌育德]: "RE: [PATCH v3 RESEND 4/4] usb: core: add phy notify connect and disconnect"
Previous message: Yuanhan Zhang: "Re: [PATCH v3] sched/cputime: let ktimers align with ksoftirqd in accounting CPUTIME_SOFTIRQ"
In reply to: Joel Granados: "Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables"
Next in thread: Thomas Weißschuh: "Re: [PATCH v2 00/18] sysctl: constify sysctl ctl_tables"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]