Received-SPF: pass (google.com: domain of linux-kernel+bounces-110337-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 21 Mar 2024 17:27:58 +0100
From: Joel Granados <j.granados@samsung.com>
To: <wenyang.linux@foxmail.com>
CC: "Eric W . Biederman" <ebiederm@xmission.com>, Luis Chamberlain
	<mcgrof@kernel.org>, Kees Cook <keescook@chromium.org>, Christian Brauner
	<brauner@kernel.org>, Iurii Zaikin <yzaikin@google.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/9] sysctl: support encoding values directly in the
 table entry
Message-ID: <20240321162758.tkituzvff5rwnvd2@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="izn2qnfca3crqd72"
Content-Disposition: inline
In-Reply-To: <tencent_143077FB953D8B549153BB07F54C5AA4870A@qq.com>
CMS-TYPE: 201P
References: <cover.1709978655.git.wenyang.linux@foxmail.com>
	<CGME20240309103218eucas1p12d642356f7e90ebdf6a7a3101688f55a@eucas1p1.samsung.com>
	<tencent_143077FB953D8B549153BB07F54C5AA4870A@qq.com>

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

On Sat, Mar 09, 2024 at 06:31:18PM +0800, wenyang.linux@foxmail.com wrote:
> From: Wen Yang <wenyang.linux@foxmail.com>
>=20
> Eric points out: "by turning .extra1 and .extra2 into longs instead of
> keeping them as pointers and needing constants to be pointed at somewhere
> ... The only people I can see who find a significant benefit by
> consolidating all of the constants into one place are people who know how
> to stomp kernel memory."
I'm assuming that this is the "why" of the commit. Please change it so
it is more direct. Something like "Directly encode numeric values in
macros in order to ...". If you want to add Eric's opinion please add it
as a link (Please follow Documentation/process/submitting-patches.rst)


>=20
> This patch supports encoding values directly in table entries through the
> following work:
> - extra1/extra2 and min/max are placed in one union to ensure that the
>   previous code is not broken, then we have time to remove unnecessary
>   extra1/extra2 progressively;
> - since type only has two states, use one bit to represent it;
Please remove this optimization from your commit. This will conflict
with work that Thomas is doing here
https://lore.kernel.org/all/20240222-sysctl-empty-dir-v1-0-45ba9a6352e8@wei=
ssschuh.net=20

> - two bits were used to represent the information of the above union( 0:
>   using extra1/extra2, 1: using min, 2: using max, 3: using both min/max);
> - added some helper macros.
>=20
> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Joel Granados <j.granados@samsung.com>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Iurii Zaikin <yzaikin@google.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  include/linux/sysctl.h | 108 ++++++++++++++++++++++++++++++++++++++---
>  kernel/sysctl.c        |  61 +++++++++++++++++------
>  2 files changed, 148 insertions(+), 21 deletions(-)
>=20
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index ee7d33b89e9e..1ba980219e40 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -61,6 +61,25 @@ extern const int sysctl_vals[];
> =20
>  extern const unsigned long sysctl_long_vals[];
> =20
> +#define	SYSCTL_NUMERIC_NEG_ONE			((long)-1)
> +#define	SYSCTL_NUMERIC_ZERO			(0L)
> +#define	SYSCTL_NUMERIC_ONE			(1L)
> +#define	SYSCTL_NUMERIC_TWO			(2L)
> +#define	SYSCTL_NUMERIC_THREE			(3L)
> +#define	SYSCTL_NUMERIC_FOUR			(4L)
> +#define	SYSCTL_NUMERIC_ONE_HUNDRED		(100L)
> +#define	SYSCTL_NUMERIC_TWO_HUNDRED		(200L)
> +#define	SYSCTL_NUMERIC_THREE_HUNDRED		(300L)
> +#define	SYSCTL_NUMERIC_FIVE_HUNDRED		(500L)
> +#define	SYSCTL_NUMERIC_ONE_THOUSAND		(1000L)
> +#define	SYSCTL_NUMERIC_TWO_THOUSAND		(2000L)
> +#define	SYSCTL_NUMERIC_THREE_THOUSAND		(3000L)
> +#define	SYSCTL_NUMERIC_16K			(16384L)
> +#define	SYSCTL_NUMERIC_U8_MAX			((long)U8_MAX)
> +#define	SYSCTL_NUMERIC_U16_MAX			((long)U16_MAX)
> +#define	SYSCTL_NUMERIC_INT_MAX			((long)INT_MAX)
> +#define	SYSCTL_NUMERIC_LONG_MAX			(LONG_MAX)
> +
>  typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer,
>  		size_t *lenp, loff_t *ppos);
> =20
> @@ -131,6 +150,18 @@ static inline void *proc_sys_poll_event(struct ctl_t=
able_poll *poll)
>  #define DEFINE_CTL_TABLE_POLL(name)					\
>  	struct ctl_table_poll name =3D __CTL_TABLE_POLL_INITIALIZER(name)
> =20
> +enum {
> +	SYSCTL_TABLE_TYPE_DEFAULT,
> +	SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY
> +};
> +
> +enum {
> +	SYSCTL_TABLE_EXTRA_PTR,
> +	SYSCTL_TABLE_EXTRA_LONG_INIT_MIN,
> +	SYSCTL_TABLE_EXTRA_LONG_INIT_MAX,
> +	SYSCTL_TABLE_EXTRA_LONG_INIT_MINMAX
> +};
> +
>  /* A sysctl table is an array of struct ctl_table: */
>  struct ctl_table {
>  	const char *procname;		/* Text ID for /proc/sys, or zero */
> @@ -138,20 +169,39 @@ struct ctl_table {
>  	int maxlen;
>  	umode_t mode;
>  	/**
> -	 * enum type - Enumeration to differentiate between ctl target types
> +	 * type - Indicates to differentiate between ctl target types
>  	 * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations
>  	 * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently
>  	 *                                       empty directory target to serve
>  	 *                                       as mount point.
>  	 */
> -	enum {
> -		SYSCTL_TABLE_TYPE_DEFAULT,
> -		SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY
> -	} type;
> +	u8 type:1;
As I said before. Please remove this from your patch.

> +
> +	/**
> +	 * extra_flags
> +	 * @SYSCTL_TABLE_EXTRA_PTR: flag indicating that this uses extra1/extra=
2.
> +	 * @SYSCTL_TABLE_EXTRA_LONG_INIT_MIN: flag indicating that this uses mi=
n/max
> +					      and min has been initialized.
> +	 * @SYSCTL_TABLE_EXTRA_LONG_INIT_MAX: flag indicating that this uses mi=
n/max
> +					      and max has been initialized.
> +	 * @SYSCTL_TABLE_EXTRA_LONG_INIT_MINMAX: flag indicating that this uses=
 min/max
> +						 and both have been initialized.
> +	 *
> +	 */
> +	u8 extra_flags:2;
This extra_flag is needed because now you have lost the extra bit of
information that the NULL pointer gave you. This is effectively adding 2
bits per ctl_table element for all ctl_table types; event the ones that
do not need min max. So how much will we actually save with all this?
once you have added these 2 bits and removed the static variables from
the files that are not using the pointers? Is saving read only memory
the only reason for this? If that is the case, please add some
calculations of how much we save to see if it actually make sense. To
calculate the memory gains/losses you can use the bloat-o-meter script
under the scripts directory (something similar to what we did here
https://lore.kernel.org/all/20240314-jag-sysctl_remset_net-v1-0-aa26b44d29d=
9@samsung.com)

I'll hold off on reviewing the other patches in this set until this is a
bit more clear.

> +	union {
> +		struct {
> +			void *extra1;
> +			void *extra2;
> +		};
> +		struct {
> +			long min;
> +			long max;
> +		};
> +	};
> +
>  	proc_handler *proc_handler;	/* Callback for text formatting */
>  	struct ctl_table_poll *poll;
> -	void *extra1;
> -	void *extra2;
>  } __randomize_layout;
=2E..

Best
--=20

Joel Granados

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

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

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmX8YA0ACgkQupfNUreW
QU8Dtgv/R+vs8reT8chyCAo5fHLsuI4wL2HmyFz4ewopKinHpQknmxlxnqLUR30W
E0lwjP1szd85og0ujN6Dvxco2ppPkahdUK1fuqQa5AF1+aj09gd1riBB9ebZCUj9
Zlg4vxFObhLHoyrYnGo9pyCPGvvCTgyzR+dywtwUe6M13gKTtNJtrZPcsSHv/5Ul
bxcleWONu4quZlxXr3fU0YOh1ZgXAOyluCnIWoXdNW8OHhh/CXL/8Xq3JtyoZRBg
JDYMp9YWM6itZeUIHd0DgBvFo3/xrJ8Fi3+4Xj5L3yG6UwDX0vQ0cVXyRk/5iiml
RauiPoJJbJpSaQ4GmYFzsrVht4q4s+S6SWmB75T9U9YGU+qiH22Fc9WSsgCwI+4m
83UdyG87h+czaOHWWucwRPi2M5iZepZlH67/s6mwsRIlFsn3ExtYnXLPxp+InRfD
/T1ZrNdCN18GBQ2x4jMTW/Mr2aYnef2d4sxXpEH/N2UqlCvTQo/3nP0yc89vv71L
ojdYAadJ
=yr5U
-----END PGP SIGNATURE-----

--izn2qnfca3crqd72--