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

[Joel Granados <j.granados@samsung.com>]

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

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@samsung.com
[4] https://lore.kernel.org/all/20230726140635.2059334-1-j.granados@samsung.com
> 
> [0] 43a7206b0963 ("driver core: class: make class_register() take a const *")
> [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/
> 
> ---
> 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@weissschuh.net
> 
> ---
> 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@weissschuh.net>
> 

-- 

Joel Granados

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