Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Mon, 4 Dec 2023 09:43:14 +0100
From:   Joel Granados <j.granados@samsung.com>
To:     Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
CC:     Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Iurii Zaikin <yzaikin@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-hardening@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH RFC 0/7] sysctl: constify sysctl ctl_tables
Message-ID: <20231204084314.i7tvvejhlppdz5zd@localhost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
        protocol="application/pgp-signature"; boundary="qjmppfb42xwa7qzq"
Content-Disposition: inline
In-Reply-To: <e3932680-d284-4e13-9c0c-f202d588bf60@t-8ch.de>
CMS-TYPE: 201P
References: <CGME20231125125305eucas1p2ebdf870dd8ef46ea9d346f727b832439@eucas1p2.samsung.com>
        <20231125-const-sysctl-v1-0-5e881b0e0290@weissschuh.net>
        <20231127101323.sdnibmf7c3d5ovye@localhost>
        <475cd5fa-f0cc-4b8b-9e04-458f6d143178@t-8ch.de>
        <20231201163120.depfyngsxdiuchvc@localhost>
        <e3932680-d284-4e13-9c0c-f202d588bf60@t-8ch.de>
Precedence: bulk

--qjmppfb42xwa7qzq
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hey

I see that you sent a V2. I'll try to get to it at the end of the week.

On Sun, Dec 03, 2023 at 04:37:01PM +0100, Thomas Wei=DFschuh wrote:
> Hi Joel,
>=20
> On 2023-12-01 17:31:20+0100, Joel Granados wrote:
> > Hey Thomas.
> >=20
> > Thx for the clarifications. I did more of a deep dive into your set and
> > have additional comments (in line). I think const-ing all this is a good
> > approach. The way forward is to be able to see the entire patch set of
> > changes in a V1 or a shared repo somewhere to have a better picture of
> > what is going on. By the "entire patchset" I mean all the changes that
> > you described in the "full process".
>=20
> All the changes will be a lot. I don't think the incremental value to
> migrate all proc_handlers versus the work is useful for the discussion.
> I can however write up my proposed changes for the sysctl core properly
> and submit them as part of the next revision.
Looking forward to seeing them in V2

>=20
> > On Tue, Nov 28, 2023 at 09:18:30AM +0100, Thomas Wei=DFschuh wrote:
> > > Hi Joel,
> > >=20
> > > On 2023-11-27 11:13:23+0100, Joel Granados wrote:
> > > > In general I would like to see more clarity with the motivation and=
 I
> > > > would also expect some system testing. My comments inline:

<--- snip --->

> > is all sysctl code and cannot be chunked up because of dependencies,
> > then it should be ok to do it in one go.
> >=20
> > > > > * Migrate all other sysctl handlers to proc_handler_new.
> > > > > * Drop the old proc_handler_field.
> > > > > * Fix the sysctl core to not modify the tables anymore.
> > > > > * Adapt public sysctl APIs to take "const struct ctl_table *".
> > > > > * Teach checkpatch.pl to warn on non-const "struct ctl_table"
> > > > >   definitions.
>=20
> > Have you considered how to ignore the cases where the ctl_tables are
> > supposed to be non-const when they are defined (like in the network
> > code that we were discussing earlier)
>=20
> As it would be a checkpatch warning it can be ignore while writing the
> patch and it won't trigger afterwards.
I mention coccinelle it is able to identify const vs non-const uses of
the ctl_table and only warn on the cases where it makes sense. This
would remove false negatives from pushing patches through.

>=20
> > > > > * Migrate definitions of "struct ctl_table" to "const" where appl=
icable.
> > These migrations are treewide and are usually reviewed by a wider
> > audience. You might need to chunk it up to make the review more palpable
> > for the other maintainers.
>=20
> Ack.
>=20
> > > > > =20
> > > > >=20
> > > > > Notes:
> > > > >=20
> > > > > Just casting the function pointers around would trigger
> > > > > CFI (control flow integrity) warnings.
> > > > >=20
> > > > > The name of the new handler "proc_handler_new" is a bit too long =
messing
> > > > > up the alignment of the table definitions.
> > > > > Maybe "proc_handler2" or "proc_handler_c" for (const) would be be=
tter.
> > >=20
> > > > indeed the name does not say much. "_new" looses its meaning quite =
fast
> > > > :)
> > >=20
> > > Hopefully somebody comes up with a better name!
>=20
> > I would like to avoid this all together and just do add the const to the
> > existing "proc_handler"
>=20
> Ack.
>=20
> > >=20
> > > > In my experience these tree wide modifications are quite tricky. Ha=
ve you
> > > > run any tests to see that everything is as it was? sysctl selftests=
 and
> > > > 0-day come to mind.
> > >=20
> > > I managed to miss one change in my initial submission:
> > > With the hunk below selftests and typing emails work.
> > >=20
> > > --- a/fs/proc/proc_sysctl.c
> > > +++ b/fs/proc/proc_sysctl.c
> > > @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path,=
 struct ctl_table_header *header)
> > >                         else
> > >                                 err |=3D sysctl_check_table_array(pat=
h, entry);
> > >                 }
> > > -               if (!entry->proc_handler)
> > > +               if (!entry->proc_handler && !entry->proc_handler_new)
> > >                         err |=3D sysctl_err(path, entry, "No proc_han=
dler");
> > > =20
> > >                 if ((entry->mode & (S_IRUGO|S_IWUGO)) !=3D entry->mod=
e)
> > >=20
> > > > [..]
> > >=20
> > > [0] 43a7206b0963 ("driver core: class: make class_register() take a c=
onst *")
> > > [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmai=
l.com/
>=20
> Thanks for the feedback!
>=20
> Thomas

--=20

Joel Granados

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

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

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmVtkRwACgkQupfNUreW
QU+y6gwAjmwiTajL3mLXXNgL5w17CaV42APC0OLNo5wj9QChvb9jjfHkSUyhM5Yu
jkfQdXAvoGv/T1iKWIiO+lD35CNuLrZ+4Jdahw8jk0lPNp7B8qbRTfd4GcVtFwQX
kcOTEZ1hRi7/cXf9yqllN8L5QZx0aALg8N3CM1/d/19nnZFZK+RfhS3ELSps6iIv
xv4VLF+1WbtnKgMQBm9DR6mPt8igvuTY/0sT2haTM8JQ/SomWeID2ouHqViYD+CU
hENTI608WD3x5oB/oozWwGfJhKEncYBbUKGpQH6hxnsFU5s9CjWF28j2bHCCSXW6
F3XKm4c+2bkch6oPvgP0h27ZJVNHZAKUXIUc3zCGwFg6D6w/PitbPkcaGa6wco7s
P+rx6EWENX/6zD0iUReFSE+1WRYpqkh3j/qDLaO2Zc8M84tTAgRxv+ZsIlTEWSFz
sRYZvi/y9k5cjrmmqhNCDPeNV/sCiPw85ftak5aNh8/5gJDdb2Ei8HCXj3ryu6Hp
2WUuz9HZ
=KDf2
-----END PGP SIGNATURE-----

--qjmppfb42xwa7qzq--