Received-SPF: pass (google.com: domain of linux-nfs+bounces-3142-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 3 May 2024 11:03:32 +0200
From: Joel Granados <j.granados@samsung.com>
To: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
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
Message-ID: <20240503090332.irkiwn73dgznjflz@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="pu6elde4hy573vso"
Content-Disposition: inline
In-Reply-To: <20240423-sysctl-const-handler-v3-0-e0beccb836e2@weissschuh.net>
CMS-TYPE: 201P
References: <CGME20240423075608eucas1p265e7c90f3efd6995cb240b3d2688b803@eucas1p2.samsung.com>
	<20240423-sysctl-const-handler-v3-0-e0beccb836e2@weissschuh.net>

--pu6elde4hy573vso
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 a=
nd
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". Lik=
e 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 t=
he
   attack surface. This is especially relevant for structures that have fun=
ction
   pointers (like ctl_table); having these in .rodata means that these poin=
ters
   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 supp=
osed
   to change or its obvious that a function is re-entrant. Actually a lot o=
f 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 n=
eed
to be my three points, but it should be more than just "put things into
=2Erodata". 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
=2Erodata than I expected (the results are in [9]) Why is that? Is it somet=
hing
that has not been posted to the lists yet?=20

Best

[1] https://lore.kernel.org/all/20240423-sysctl-const-handler-v3-0-e0beccb8=
36e2@weissschuh.net/
[2] https://lore.kernel.org/all/20240418-sysctl-const-table-arg-v2-1-4012ab=
c31311@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-1aaa8=
2b07bc6@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@wei=
ssschuh.net/

[8]
    #!/usr/bin/python3

    import subprocess
    import re

    def exec_cmd( cmd ):
        try:
            result =3D subprocess.run(cmd, shell=3DTrue, text=3DTrue, check=
=3DTrue, capture_output=3DTrue)
            output_lines =3D 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 =3D True):
        filtered_lines =3D []
        seen_lines =3D set()
        regexes =3D [re.compile(pattern) for pattern in regex_patterns]

        for line in lines:
            for regex in regexes:
                line =3D 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 =3D []
        regexes =3D [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 =3D "git grep 'static \(const \)\?struct ctl_table '"
    regex_patterns =3D ['[\}]*;$', ' =3D \{', '\[.*\]', '.*\.(c|h):[ \t]*st=
atic (const )?struct ctl_table ']
    ctl_table_structs =3D remove_tokens_re(exec_cmd( cmd ), regex_patterns)

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

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

    regex_patterns =3D ['^.*\(', '\)[ ]+.*$']
    sec_names =3D remove_tokens_re( output_lines, regex_patterns, uniq=3DFa=
lse)

    for i in range(len(sec_names)):
        obj_name =3D 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

--pu6elde4hy573vso
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmY0qGMACgkQupfNUreW
QU9meQwAhSiWxhtL9djecqOkREsAyj1iIrA1uEV4NTghMsTyhC1r83HyDUEWGOJo
fTZ8jrYwxeDy9qnd5vC+hEYgx/tBWXv+79vssjd28m2Mn4DogQlWABKsPJlPw/8R
IB9dau7hME9YLSiZQaKoL8DuqnQ4jvdN/zvWcuYHl2s/jF+qzsI3DT4Dy98S1Sr8
63rSYg/sulLme7ov/3gAQ3ceSPYlLRUfYdWLlGkUYT2iIjcBIybKotaKGkERq8xL
Z2PcPpWj15gYh6Ewd0GL7AMq2M/dSGYYxPIbv0Mnjpe5AV5HzGZ06MUg4Tt11p1z
5Fchr0Hg24b2UYcx4mjbsDCy/OyF2oA8cjjj79ODXcgpsVxWX8GZdcFh/gNmsPw5
dKZyo4v1ZSvDmu8uDCcaV92GLelxc2YfGHYEfbE+hhJR0YaAukrL8mfqPJIanYv+
zzIuChlVlLB5xoY0CT3XgLo8NJrc/ERnY8JibAauNNLQvTjHV/tbd0UknFQQC4XL
gDaMqz5j
=1R4C
-----END PGP SIGNATURE-----

--pu6elde4hy573vso--