Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

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/[email protected]/
> [2] https://lore.kernel.org/all/[email protected]
> [3] [PATCH v2 00/14] ASoC: Constify local snd_sof_dsp_ops
>     https://lore.kernel.org/all/[email protected]
> [4] [PATCH v2 00/19] backlight: Constify lcd_ops
>     https://lore.kernel.org/all/[email protected]
> [5] [PATCH 1/4] iommu: constify pointer to bus_type
>     https://lore.kernel.org/all/[email protected]
> [6] [PATCH 00/29] const xattr tables
>     https://lore.kernel.org/all/[email protected]
> [7] https://lore.kernel.org/all/[email protected]/
> 
> [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