Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
Hey Joel, On 2024-05-03 11:03:32+, 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-e0beccb83...@weissschuh.net/ > [2] > https://lore.kernel.org/all/20240418-sysctl-const-table-arg-v2-1-4012abc31...@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-e553fe67a...@kernel.org > [4] [PATCH v2 00/19] backlight: Constify lcd_ops > > https://lore.kernel.org/all/20240424-video-backlight-lcd-ops-v2-0-1aaa82b07...@kernel.org > [5] [PATCH 1/4] iommu: constify pointer to bus_type > > https://lore.kernel.org/all/20240216144027.185959-1-krzysztof.kozlow...@linaro.org > [6] [PATCH 00/29] const xattr tables > https://lore.kernel.org/all/20230930050033.41174-1-wedso...@gmail.com > [7] > https://lore.kernel.org/all/20231204-const-sysctl-v2-0-7a5060b11...@weissschuh.net/ > > [8] [snip] > [9] > section: .rodataobj_name : kern_table > section: .rodataobj_name : sysctl_mount_point > section: .rodataobj_name : addrconf_sysctl > section: .rodataobj_name : ax25_param_table > section: .rodataobj_name : mpls_table > section: .rodataobj_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
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
Hey Thomas
Here is my feedback for your outstanding constification patches [1] and [2].
# 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".
4. Please publish the final result somewhere. This is important so someone can
take over in case you need to stop.
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?
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.
# 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?
Best
[1]
https://lore.kernel.org/all/20240423-sysctl-const-handler-v3-0-e0beccb83...@weissschuh.net/
[2]
https://lore.kernel.org/all/20240418-sysctl-const-table-arg-v2-1-4012abc31...@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-e553fe67a...@kernel.org
[4] [PATCH v2 00/19] backlight: Constify lcd_ops
https://lore.kernel.org/all/20240424-video-backlight-lcd-ops-v2-0-1aaa82b07...@kernel.org
[5] [PATCH 1/4] iommu: constify pointer to bus_type
https://lore.kernel.org/all/20240216144027.185959-1-krzysztof.kozlow...@linaro.org
[6] [PATCH 00/29] const xattr tables
https://lore.kernel.org/all/20230930050033.41174-1-wedso...@gmail.com
[7]
https://lore.kernel.org/all/20231204-const-sysctl-v2-0-7a5060b11...@weissschuh.net/
[8]
#!/usr/bin/python3
import subprocess
import re
def exec_cmd( cmd ):
try:
result = subprocess.run(cmd, shell=True, text=True, check=True,
capture_output=True)
output_lines = result.stdout.splitlines()
return output_lines
except Exception as e:
print(f"An error occurred: {e}")
return []
def remove_tokens_re(lines, regex_patterns, uniq = True):
filtered_lines = []
seen_lines = set()
regexes = [re.compile(pattern) for pattern in regex_patterns]
for line in lines:
for regex in regexes:
line = regex.sub('', line) # Replace matches with empty string
if uniq:
if line not in seen_lines:
seen_lines.add(line)
filtered_lines.append(line)
else:
filtered_lines.append(line)
return filtered_lines
def filter_in_lines(lines, regex_patterns):
filtered_lines = []
regexes = [re.compile(pattern) for pattern in regex_patterns]
for line in lines:
if any(regex.search(line) for regex in regexes):
filtered_lines.append(line)
return filtered_lines
cmd = "git grep 'static \(const \)\?struct ctl_table '"
regex_patterns = ['[\}]*;$', ' = \{', '\[.*\]', '.*\.(c|h):[ \t]*static
(const )?struct ctl_table ']
ctl_table_structs = remove_tokens_re(exec_cmd( cmd ), regex_patterns)
