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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-e0beccb836e2@xxxxxxxxxxxxxx/
[2] https://lore.kernel.org/all/20240418-sysctl-const-table-arg-v2-1-4012abc31311@xxxxxxxxxxxxxx
[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@xxxxxxxxxx
[4] [PATCH v2 00/19] backlight: Constify lcd_ops
    https://lore.kernel.org/all/20240424-video-backlight-lcd-ops-v2-0-1aaa82b07bc6@xxxxxxxxxx
[5] [PATCH 1/4] iommu: constify pointer to bus_type
    https://lore.kernel.org/all/20240216144027.185959-1-krzysztof.kozlowski@xxxxxxxxxx
[6] [PATCH 00/29] const xattr tables
    https://lore.kernel.org/all/20230930050033.41174-1-wedsonaf@xxxxxxxxx
[7] https://lore.kernel.org/all/20231204-const-sysctl-v2-0-7a5060b11447@xxxxxxxxxxxxxx/

[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)

    cmd = "readelf -X -Ws build/vmlinux"
    regex_patterns = ['.*OBJECT.*']
    output_lines = filter_in_lines(exec_cmd( cmd ), regex_patterns);

    regex_patterns = ['^.*OBJECT', '[ \t]+[A-Z]+[ \t]+[A-Z]+.*\(.*\)[ \t]+']
    obj_elems = remove_tokens_re( output_lines, regex_patterns, uniq=False)

    regex_patterns = ['^.*\(', '\)[ ]+.*$']
    sec_names = remove_tokens_re( output_lines, regex_patterns, uniq=False)

    for i in range(len(sec_names)):
        obj_name = obj_elems[i]
        if obj_name in ctl_table_structs:
            print ("section: {}\t\tobj_name : {}". format(sec_names[i], obj_name))

[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

--

Joel Granados

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Networking Development]     [Linux OMAP]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     SCTP

  Powered by Linux