Re: [RESEND PATCH v2 1/9] sysctl: support encoding values directly in the table entry

From: Joel Granados
Date: Tue Mar 19 2024 - 12:51:03 EST

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@xxxxxxxxxxx wrote:
> From: Wen Yang <wenyang.linux@xxxxxxxxxxx>
>
> 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 consolidating
> all of the constants into one place are people who know how to stomp
> kernel memory.
>
> 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.
>
> Suggested-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> Signed-off-by: Wen Yang <wenyang.linux@xxxxxxxxxxx>
> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Joel Granados <j.granados@xxxxxxxxxxx>
> Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> Cc: Iurii Zaikin <yzaikin@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
> include/linux/sysctl.h | 108 ++++++++++++++++++++++++++++++++++++++---
> kernel/sysctl.c | 61 +++++++++++++++++------
> 2 files changed, 148 insertions(+), 21 deletions(-)
>
> 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[];
>
> extern const unsigned long sysctl_long_vals[];
>
> +#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);
>
> @@ -131,6 +150,18 @@ static inline void *proc_sys_poll_event(struct ctl_table_poll *poll)
> #define DEFINE_CTL_TABLE_POLL(name) \
> struct ctl_table_poll name = __CTL_TABLE_POLL_INITIALIZER(name)
>
> +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/extra2.
> + * @SYSCTL_TABLE_EXTRA_LONG_INIT_MIN: flag indicating that this uses min/max
> + and min has been initialized.
> + * @SYSCTL_TABLE_EXTRA_LONG_INIT_MAX: flag indicating that this uses min/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;
>
> struct ctl_node {
> @@ -213,6 +263,50 @@ struct ctl_table_root {
> #define register_sysctl(path, table) \
> register_sysctl_sz(path, table, ARRAY_SIZE(table))
>
> +#define CTL_TABLE_ENTRY(_name, _data, _len, _mode, _func) \
> + { \
> + .procname = _name, \
> + .data = _data, \
> + .maxlen = _len, \
> + .mode = _mode, \
> + .proc_handler = _func, \
> + .extra_flags = SYSCTL_TABLE_EXTRA_PTR, \
> + }
> +
> +#define CTL_TABLE_ENTRY_MIN(_name, _data, _len, _mode, _func, _min) \
> + { \
> + .procname = _name, \
> + .data = _data, \
> + .maxlen = _len, \
> + .mode = _mode, \
> + .proc_handler = _func, \
> + .min = _min, \
> + .extra_flags = SYSCTL_TABLE_EXTRA_LONG_INIT_MIN, \
> + }
> +
> +#define CTL_TABLE_ENTRY_MAX(_name, _data, _len, _mode, _func, _max) \
> + { \
> + .procname = _name, \
> + .data = _data, \
> + .maxlen = _len, \
> + .mode = _mode, \
> + .proc_handler = _func, \
> + .max = _max, \
> + .extra_flags = SYSCTL_TABLE_EXTRA_LONG_INIT_MAX, \
> + }
> +
> +#define CTL_TABLE_ENTRY_MINMAX(_name, _data, _len, _mode, _func, _min, _max) \
> + { \
> + .procname = _name, \
> + .data = _data, \
> + .maxlen = _len, \
> + .mode = _mode, \
> + .proc_handler = _func, \
> + .min = _min, \
> + .max = _max, \
> + .extra_flags = SYSCTL_TABLE_EXTRA_LONG_INIT_MINMAX, \
> + }
> +
> #ifdef CONFIG_SYSCTL
>
> 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;
> };
>
> +static void do_int_conv_param_init(const struct ctl_table *table,
> + struct do_proc_dointvec_minmax_conv_param *param)
> +{
> + if (table->extra_flags == SYSCTL_TABLE_EXTRA_PTR) {
> + param->min = table->extra1;
> + param->max = table->extra2;
> + } else {
> + param->min = (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MIN) ?
> + (int *)&table->min : NULL;
> + param->max = (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 = {
> - .min = (int *) table->extra1,
> - .max = (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;
> };
>
> +static void do_uint_conv_param_init(const struct ctl_table *table,
> + struct do_proc_douintvec_minmax_conv_param *param)
> +{
> + if (table->extra_flags == SYSCTL_TABLE_EXTRA_PTR) {
> + param->min = table->extra1;
> + param->max = table->extra2;
> + } else {
> + param->min = (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MIN) ?
> + (unsigned int *)&table->min : NULL;
> + param->max = (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 long *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 = {
> - .min = (unsigned int *) table->extra1,
> - .max = (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,
> }
>
> i = data;
> - min = table->extra1;
> - max = table->extra2;
> + if (table->extra_flags == SYSCTL_TABLE_EXTRA_PTR) {
> + min = table->extra1;
> + max = table->extra2;
> + } else {
> + min = (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MIN) ?
> + &table->min : NULL;
> + max = (table->extra_flags & SYSCTL_TABLE_EXTRA_LONG_INIT_MAX) ?
> + &table->max : NULL;
> + }
> +
> vleft = table->maxlen / sizeof(unsigned long);
> left = *lenp;
>
> @@ -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 = {
> - .min = (int *) table->extra1,
> - .max = (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);
> }
> --
> 2.25.1
>

--

Joel Granados

Attachment: signature.asc
Description: PGP signature