Received-SPF: pass (google.com: domain of linux-kernel+bounces-107865-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 19 Mar 2024 17:50:49 +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>, Dave Young <dyoung@redhat.com>, Iurii Zaikin
	<yzaikin@google.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [RESEND PATCH v2 1/9] sysctl: support encoding values directly
 in the table entry
Message-ID: <20240319165049.d7m4jvwb4qyjtjhq@joelS2.panther.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="awwnjpzdp4nogjz5"
Content-Disposition: inline
In-Reply-To: <tencent_0D6ABA209A4980742DE6003FBFE7FE2A3207@qq.com>
CMS-TYPE: 201P
References: <CGME20240319155826eucas1p2af22771faac0985e0097d38e96daac4b@eucas1p2.samsung.com>
	<tencent_0D6ABA209A4980742DE6003FBFE7FE2A3207@qq.com>

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

This is a resend on the one 10 days ago. Right?
I have it in my todo list and will get to it eventually. Please stop
resending it if there is no change.

Thx

On Tue, Mar 19, 2024 at 11:57:42PM +0800, wenyang.linux@foxmail.com wrote:
> From: Wen Yang <wenyang.linux@foxmail.com>
>=20
> Eric pointed 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 consolidati=
ng
>   all of the constants into one place are people who know how to stomp
>   kernel memory.
>=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;
> - 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;
> +
> +	/**
> +	 * 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;
> +	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;
> =20
>  struct ctl_node {
> @@ -213,6 +263,50 @@ struct ctl_table_root {
>  #define register_sysctl(path, table)	\
>  	register_sysctl_sz(path, table, ARRAY_SIZE(table))
> =20
> +#define CTL_TABLE_ENTRY(_name, _data, _len, _mode, _func)		      \
> +	{								      \
> +		.procname =3D _name,					      \
> +		.data =3D _data,						      \
> +		.maxlen =3D _len,						      \
> +		.mode =3D _mode,						      \
> +		.proc_handler =3D _func,					      \
> +		.extra_flags =3D SYSCTL_TABLE_EXTRA_PTR,			      \
> +	}
> +
> +#define CTL_TABLE_ENTRY_MIN(_name, _data, _len, _mode, _func, _min)	    =
  \
> +	{								      \
> +		.procname =3D _name,					      \
> +		.data =3D _data,						      \
> +		.maxlen =3D _len,						      \
> +		.mode =3D _mode,						      \
> +		.proc_handler =3D _func,					      \
> +		.min =3D _min,						      \
> +		.extra_flags =3D SYSCTL_TABLE_EXTRA_LONG_INIT_MIN,	      \
> +	}
> +
> +#define CTL_TABLE_ENTRY_MAX(_name, _data, _len, _mode, _func, _max)	    =
  \
> +	{								      \
> +		.procname =3D _name,					      \
> +		.data =3D _data,						      \
> +		.maxlen =3D _len,						      \
> +		.mode =3D _mode,						      \
> +		.proc_handler =3D _func,					      \
> +		.max =3D _max,						      \
> +		.extra_flags =3D SYSCTL_TABLE_EXTRA_LONG_INIT_MAX,	      \
> +	}
> +
> +#define CTL_TABLE_ENTRY_MINMAX(_name, _data, _len, _mode, _func, _min, _=
max)  \
> +	{								      \
> +		.procname =3D _name,					      \
> +		.data =3D _data,						      \
> +		.maxlen =3D _len,						      \
> +		.mode =3D _mode,						      \
> +		.proc_handler =3D _func,					      \
> +		.min =3D _min,						      \
> +		.max =3D _max,						      \
> +		.extra_flags =3D SYSCTL_TABLE_EXTRA_LONG_INIT_MINMAX,	      \
> +	}
> +
>  #ifdef CONFIG_SYSCTL
> =20
>  void proc_sys_poll_notify(struct ctl_table_poll *poll);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 157f7ce2942d..144c441236ab 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -822,6 +822,20 @@ struct do_proc_dointvec_minmax_conv_param {
>  	int *max;
>  };
> =20
> +static void do_int_conv_param_init(const struct ctl_table *table,
> +		struct do_proc_dointvec_minmax_conv_param *param)
> +{
> +	if (table->extra_flags =3D=3D SYSCTL_TABLE_EXTRA_PTR) {
> +		param->min =3D table->extra1;
> +		param->max =3D table->extra2;
> +	} else {
> +		param->min =3D (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MIN)=
 ?
> +			(int *)&table->min : NULL;
> +		param->max =3D (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MAX)=
 ?
> +			(int *)&table->max : NULL;
> +	}
> +}
> +
>  static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>  					int *valp,
>  					int write, void *data)
> @@ -867,10 +881,9 @@ static int do_proc_dointvec_minmax_conv(bool *negp, =
unsigned long *lvalp,
>  int proc_dointvec_minmax(struct ctl_table *table, int write,
>  		  void *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	struct do_proc_dointvec_minmax_conv_param param =3D {
> -		.min =3D (int *) table->extra1,
> -		.max =3D (int *) table->extra2,
> -	};
> +	struct do_proc_dointvec_minmax_conv_param param;
> +
> +	do_int_conv_param_init(table, &param);
>  	return do_proc_dointvec(table, write, buffer, lenp, ppos,
>  				do_proc_dointvec_minmax_conv, &param);
>  }
> @@ -889,6 +902,20 @@ struct do_proc_douintvec_minmax_conv_param {
>  	unsigned int *max;
>  };
> =20
> +static void do_uint_conv_param_init(const struct ctl_table *table,
> +		struct do_proc_douintvec_minmax_conv_param *param)
> +{
> +	if (table->extra_flags =3D=3D SYSCTL_TABLE_EXTRA_PTR) {
> +		param->min =3D table->extra1;
> +		param->max =3D table->extra2;
> +	} else {
> +		param->min =3D (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MIN)=
 ?
> +			(unsigned int *)&table->min : NULL;
> +		param->max =3D (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MAX)=
 ?
> +			(unsigned int *)&table->max : NULL;
> +	}
> +}
> +
>  static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>  					 unsigned int *valp,
>  					 int write, void *data)
> @@ -936,10 +963,9 @@ static int do_proc_douintvec_minmax_conv(unsigned lo=
ng *lvalp,
>  int proc_douintvec_minmax(struct ctl_table *table, int write,
>  			  void *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	struct do_proc_douintvec_minmax_conv_param param =3D {
> -		.min =3D (unsigned int *) table->extra1,
> -		.max =3D (unsigned int *) table->extra2,
> -	};
> +	struct do_proc_douintvec_minmax_conv_param param;
> +
> +	do_uint_conv_param_init(table, &param);
>  	return do_proc_douintvec(table, write, buffer, lenp, ppos,
>  				 do_proc_douintvec_minmax_conv, &param);
>  }
> @@ -1038,8 +1064,16 @@ static int __do_proc_doulongvec_minmax(void *data,=
 struct ctl_table *table,
>  	}
> =20
>  	i =3D data;
> -	min =3D table->extra1;
> -	max =3D table->extra2;
> +	if (table->extra_flags =3D=3D SYSCTL_TABLE_EXTRA_PTR) {
> +		min =3D table->extra1;
> +		max =3D table->extra2;
> +	} else {
> +		min =3D (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MIN) ?
> +			&table->min : NULL;
> +		max =3D (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MAX) ?
> +			&table->max : NULL;
> +	}
> +
>  	vleft =3D table->maxlen / sizeof(unsigned long);
>  	left =3D *lenp;
> =20
> @@ -1274,10 +1308,9 @@ int proc_dointvec_jiffies(struct ctl_table *table,=
 int write,
>  int proc_dointvec_ms_jiffies_minmax(struct ctl_table *table, int write,
>  			  void *buffer, size_t *lenp, loff_t *ppos)
>  {
> -	struct do_proc_dointvec_minmax_conv_param param =3D {
> -		.min =3D (int *) table->extra1,
> -		.max =3D (int *) table->extra2,
> -	};
> +	struct do_proc_dointvec_minmax_conv_param param;
> +
> +	do_int_conv_param_init(table, &param);
>  	return do_proc_dointvec(table, write, buffer, lenp, ppos,
>  			do_proc_dointvec_ms_jiffies_minmax_conv, &param);
>  }
> --=20
> 2.25.1
>=20

--=20

Joel Granados

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

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

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmX5wmgACgkQupfNUreW
QU8ndgv/bkdmOja/hof1ULkEBwf6L+9zlPykni2lXJGXqv1I7FIFEMg/nFQcIJJr
yW5kjWUcIV/uv867RfcJnVpYMtmmEQQwL84/zk0KLzV+IpYPSqCcwgIsji8RI4GH
qPvaeeKnfbNYfPcXbnNNTZipe/MKx9M3r870ihXa4bf0kj6QItl9Koz2IiqxT+UN
BtYW3aXldn3S5taIbrVo5FnISq2BNkYu0XlgHogAl6ixI8oin/vIZ170FosrVMM9
1C6qAM2IbVVz7JWXGg79/DhBOcdkLfkRJCTzVYz2JdcmMjTNSdssTUOc6a41ZfGK
1hXXulqGd/e3+A5WSnkyYeBOATzlyLTZQdSr6rC3LnYVSzTeMbF0do3vn290R0Dp
RGChv9Fbvt8Jxt3uUAW2Ddd2b9VB4/NntgRzvDSFdST7w+61SWR8w5C6wEVFumTj
OXa+ZBBv/GCPY0mCtlvx4uxJZsxGCB8BWZS8nqDxxDyeXlgJDQll6qI9O3LYlsdG
QvslUi/M
=J3F2
-----END PGP SIGNATURE-----

--awwnjpzdp4nogjz5--