From: "Thomas Weißschuh" <linux@weissschuh.net>
To: Joel Granados <j.granados@samsung.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
Kees Cook <keescook@chromium.org>,
Eric Dumazet <edumazet@google.com>,
Dave Chinner <david@fromorbit.com>,
linux-fsdevel@vger.kernel.org, netdev@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-riscv@lists.infradead.org, linux-mm@kvack.org,
linux-security-module@vger.kernel.org, bpf@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-xfs@vger.kernel.org,
linux-trace-kernel@vger.kernel.org,
linux-perf-users@vger.kernel.org,
netfilter-devel@vger.kernel.org, coreteam@netfilter.org,
kexec@lists.infradead.org, linux-hardening@vger.kernel.org,
bridge@lists.linux.dev, lvs-devel@vger.kernel.org,
linux-rdma@vger.kernel.org, rds-devel@oss.oracle.com,
linux-sctp@vger.kernel.org, linux-nfs@vger.kernel.org,
apparmor@lists.ubuntu.com
Subject: Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
Date: Fri, 3 May 2024 16:09:40 +0200 [thread overview]
Message-ID: <4cda5d2d-dd92-44ef-9e7b-7b780ec795ab@t-8ch.de> (raw)
In-Reply-To: <20240503090332.irkiwn73dgznjflz@joelS2.panther.com>
Hey Joel,
On 2024-05-03 11:03:32+0000, Joel Granados wrote:
> Here is my feedback for your outstanding constification patches [1] and [2].
Thanks!
> # You need to split the patch
> The answer that you got from Jakub in the network subsystem is very clear and
> baring a change of heart from the network folks, this will go in as but as a
> split patchset. Please split it considering the following:
> 1. Create a different patchset for drivers/, fs/, kernel/, net, and a
> miscellaneous that includes whatever does not fit into the others.
> 2. Consider that this might take several releases.
> 3. Consider the following sufix for the interim function name "_const". Like in
> kfree_const. Please not "_new".
Ack. "_new" was an intentionally unacceptable placeholder.
> 4. Please publish the final result somewhere. This is important so someone can
> take over in case you need to stop.
Will do. Both for each single series and a combination of all of them.
> 5. Consistently mention the motivation in your cover letters. I specify more
> further down in "#Motivation".
> 6. Also mention that this is part of a bigger effort (like you did in your
> original cover letters). I would include [3,4,5,6]
> 7. Include a way to show what made it into .rodata. I specify more further down
> in "#Show the move".
>
> # Motivation
> As I read it, the motivation for these constification efforts are:
> 1. It provides increased safety: Having things in .rodata section reduces the
> attack surface. This is especially relevant for structures that have function
> pointers (like ctl_table); having these in .rodata means that these pointers
> always point to the "intended" function and cannot be changed.
> 2. Compiler optimizations: This was just a comment in the patchsets that I have
> mentioned ([3,4,5]). Do you know what optimizations specifically? Does it
> have to do with enhancing locality for the data in .rodata? Do you have other
> specific optimizations in mind?
I don't know about anything that would make it faster.
It's more about safety and transmission of intent to API users,
especially callback implementers.
> 3. Readability: because it is easier to know up-front that data is not supposed
> to change or its obvious that a function is re-entrant. Actually a lot of the
> readability reasons is about knowing things "up-front".
> As we move forward with the constification in sysctl, please include a more
> detailed motivation in all your cover letters. This helps maintainers (that
> don't have the context) understand what you are trying to do. It does not need
> to be my three points, but it should be more than just "put things into
> .rodata". Please tell me if I have missed anything in the motivation.
Will do.
> # Show the move
> I created [8] because there is no easy way to validate which objects made it
> into .rodata. I ran [8] for your Dec 2nd patcheset [7] and there are less in
> .rodata than I expected (the results are in [9]) Why is that? Is it something
> that has not been posted to the lists yet?
Constifying the APIs only *allows* the actual table to be constified
themselves.
Then each table definition will have to be touched and "const" added.
See patches 17 and 18 in [7] for two examples.
Some tables in net/ are already "const" as the static definitions are
never registered themselves but only their copies are.
This seems to explain your findings.
> Best
Thanks!
> [1] https://lore.kernel.org/all/20240423-sysctl-const-handler-v3-0-e0beccb836e2@weissschuh.net/
> [2] https://lore.kernel.org/all/20240418-sysctl-const-table-arg-v2-1-4012abc31311@weissschuh.net
> [3] [PATCH v2 00/14] ASoC: Constify local snd_sof_dsp_ops
> https://lore.kernel.org/all/20240426-n-const-ops-var-v2-0-e553fe67ae82@kernel.org
> [4] [PATCH v2 00/19] backlight: Constify lcd_ops
> https://lore.kernel.org/all/20240424-video-backlight-lcd-ops-v2-0-1aaa82b07bc6@kernel.org
> [5] [PATCH 1/4] iommu: constify pointer to bus_type
> https://lore.kernel.org/all/20240216144027.185959-1-krzysztof.kozlowski@linaro.org
> [6] [PATCH 00/29] const xattr tables
> https://lore.kernel.org/all/20230930050033.41174-1-wedsonaf@gmail.com
> [7] https://lore.kernel.org/all/20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net/
>
> [8]
[snip]
> [9]
> section: .rodata obj_name : kern_table
> section: .rodata obj_name : sysctl_mount_point
> section: .rodata obj_name : addrconf_sysctl
> section: .rodata obj_name : ax25_param_table
> section: .rodata obj_name : mpls_table
> section: .rodata obj_name : mpls_dev_table
> section: .data obj_name : sld_sysctls
> section: .data obj_name : kern_panic_table
> section: .data obj_name : kern_exit_table
> section: .data obj_name : vm_table
> section: .data obj_name : signal_debug_table
> section: .data obj_name : usermodehelper_table
> section: .data obj_name : kern_reboot_table
> section: .data obj_name : user_table
> section: .bss obj_name : sched_core_sysctls
> section: .data obj_name : sched_fair_sysctls
> section: .data obj_name : sched_rt_sysctls
> section: .data obj_name : sched_dl_sysctls
> section: .data obj_name : printk_sysctls
> section: .data obj_name : pid_ns_ctl_table_vm
> section: .data obj_name : seccomp_sysctl_table
> section: .data obj_name : uts_kern_table
> section: .data obj_name : vm_oom_kill_table
> section: .data obj_name : vm_page_writeback_sysctls
> section: .data obj_name : page_alloc_sysctl_table
> section: .data obj_name : hugetlb_table
> section: .data obj_name : fs_stat_sysctls
> section: .data obj_name : fs_exec_sysctls
> section: .data obj_name : fs_pipe_sysctls
> section: .data obj_name : namei_sysctls
> section: .data obj_name : fs_dcache_sysctls
> section: .data obj_name : inodes_sysctls
> section: .data obj_name : fs_namespace_sysctls
> section: .data obj_name : dnotify_sysctls
> section: .data obj_name : inotify_table
> section: .data obj_name : epoll_table
> section: .data obj_name : aio_sysctls
> section: .data obj_name : locks_sysctls
> section: .data obj_name : coredump_sysctls
> section: .data obj_name : fs_shared_sysctls
> section: .data obj_name : fs_dqstats_table
> section: .data obj_name : root_table
> section: .data obj_name : pty_table
> section: .data obj_name : xfs_table
> section: .data obj_name : ipc_sysctls
> section: .data obj_name : key_sysctls
> section: .data obj_name : kernel_io_uring_disabled_table
> section: .data obj_name : tty_table
> section: .data obj_name : random_table
> section: .data obj_name : scsi_table
> section: .data obj_name : iwcm_ctl_table
> section: .data obj_name : net_core_table
> section: .data obj_name : netns_core_table
> section: .bss obj_name : nf_log_sysctl_table
> section: .data obj_name : nf_log_sysctl_ftable
> section: .data obj_name : vs_vars
> section: .data obj_name : vs_vars_table
> section: .data obj_name : ipv4_route_netns_table
> section: .data obj_name : ipv4_route_table
> section: .data obj_name : ip4_frags_ns_ctl_table
> section: .data obj_name : ip4_frags_ctl_table
> section: .data obj_name : ctl_forward_entry
> section: .data obj_name : ipv4_table
> section: .data obj_name : ipv4_net_table
> section: .data obj_name : unix_table
> section: .data obj_name : ipv6_route_table_template
> section: .data obj_name : ipv6_icmp_table_template
> section: .data obj_name : ip6_frags_ns_ctl_table
> section: .data obj_name : ip6_frags_ctl_table
> section: .data obj_name : ipv6_table_template
> section: .data obj_name : ipv6_rotable
> section: .data obj_name : sctp_net_table
> section: .data obj_name : sctp_table
> section: .data obj_name : smc_table
> section: .data obj_name : lowpan_frags_ns_ctl_table
> section: .data obj_name : lowpan_frags_ctl_table
next prev parent reply other threads:[~2024-05-03 14:09 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20240423075608eucas1p265e7c90f3efd6995cb240b3d2688b803@eucas1p2.samsung.com>
2024-04-23 7:54 ` [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 01/11] stackleak: don't modify ctl_table argument Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 02/11] cgroup: bpf: constify ctl_table arguments and fields Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 03/11] hugetlb: constify ctl_table arguments of utility functions Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 04/11] utsname: constify ctl_table arguments of utility function Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 05/11] neighbour: " Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 06/11] ipv4/sysctl: constify ctl_table arguments of utility functions Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 07/11] ipv6/addrconf: " Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 08/11] ipv6/ndisc: constify ctl_table arguments of utility function Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 09/11] ipvs: constify ctl_table arguments of utility functions Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 10/11] sysctl: constify ctl_table arguments of utility function Thomas Weißschuh
2024-04-23 7:54 ` [PATCH v3 11/11] sysctl: treewide: constify the ctl_table argument of handlers Thomas Weißschuh
2024-04-29 9:47 ` Heiko Carstens
2024-04-23 18:31 ` [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers Luis Chamberlain
2024-04-25 3:12 ` Jakub Kicinski
2024-04-25 7:10 ` Thomas Weißschuh
2024-04-27 7:40 ` Thomas Weißschuh
2024-04-25 11:04 ` Joel Granados
2024-04-25 20:34 ` Thomas Weißschuh
2024-05-08 11:37 ` Joel Granados
2024-05-08 17:11 ` Kees Cook
2024-05-09 1:00 ` Jakub Kicinski
2024-05-11 9:51 ` Thomas Weißschuh
2024-05-12 19:32 ` Joel Granados
2024-05-13 2:57 ` Kees Cook
2024-05-12 19:24 ` Joel Granados
2024-05-03 9:03 ` Joel Granados
2024-05-03 14:09 ` Thomas Weißschuh [this message]
2024-05-08 11:40 ` Joel Granados
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=4cda5d2d-dd92-44ef-9e7b-7b780ec795ab@t-8ch.de \
--to=linux@weissschuh.net \
--cc=apparmor@lists.ubuntu.com \
--cc=bpf@vger.kernel.org \
--cc=bridge@lists.linux.dev \
--cc=coreteam@netfilter.org \
--cc=david@fromorbit.com \
--cc=edumazet@google.com \
--cc=j.granados@samsung.com \
--cc=keescook@chromium.org \
--cc=kexec@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lvs-devel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=rds-devel@oss.oracle.com \
/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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).