[PATCH v3 0/6] ipc: Clamp *mni to the real IPCMNI limit

[PATCH v3 0/6] ipc: Clamp *mni to the real IPCMNI limit

[Waiman Long]

v2->v3:
 - Fix kdoc comment errors.
 - Incorporate comments and suggestions from Luis R. Rodriguez.
 - Add a patch to fix a typo error in fs/proc/proc_sysctl.c.

v1->v2:
 - Add kdoc comments to the do_proc_do{u}intvec_minmax_conv_param
   structures.
 - Add a new flags field to the ctl_table structure for specifying
   whether range clamping should be activated instead of adding new
   sysctl parameter handlers.
 - Clamp the semmni value embedded in the multi-values sem parameter.

v1 patch: https://lkml.org/lkml/2018/2/19/453
v2 patch: https://lkml.org/lkml/2018/2/27/627

The sysctl parameters msgmni, shmmni and semmni have an inherent limit
of IPC_MNI (32k). However, users may not be aware of that because they
can write a value much higher than that without getting any error or
notification. Reading the parameters back will show the newly written
values which are not real.

Enforcing the limit by failing sysctl parameter write, however, can
break existing user applications. To address this delemma, a new flags
field is introduced into the ctl_table. The value CTL_FLAGS_CLAMP_RANGE
can be added to any ctl_table entries to enable a looser range clamping
without returning any error. For example,

  .flags = CTL_FLAGS_CLAMP_RANGE,

This flags value are now used for the range checking of shmmni,
msgmni and semmni without breaking existing applications. If any out
of range value is written to those sysctl parameters, the following
warning will be printed instead.

  Kernel parameter "shmmni" was set out of range [0, 32768], clamped to 32768.

Reading the values back will show 32768 instead of some fake values.

Waiman Long (6):
  proc/sysctl: Fix typo in sysctl_check_table_array()
  sysctl: Add kdoc comments to do_proc_do{u}intvec_minmax_conv_param
  sysctl: Add flags to support min/max range clamping
  sysctl: Warn when a clamped sysctl parameter is set out of range
  ipc: Clamp msgmni and shmmni to the real IPCMNI limit
  ipc: Clamp semmni to the real IPCMNI limit

 fs/proc/proc_sysctl.c  |   2 +-
 include/linux/sysctl.h |  17 +++++++++
 ipc/ipc_sysctl.c       |  33 ++++++++++++++--
 ipc/sem.c              |  28 ++++++++++++++
 ipc/util.h             |   4 ++
 kernel/sysctl.c        | 102 ++++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 172 insertions(+), 14 deletions(-)

-- 
1.8.3.1

[PATCH v3 1/6] proc/sysctl: Fix typo in sysctl_check_table_array()

[Waiman Long]

Signed-off-by: Waiman Long <longman@redhat.com>
---
 fs/proc/proc_sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index c41ab26..493c975 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1086,7 +1086,7 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 	if ((table->proc_handler == proc_douintvec) ||
 	    (table->proc_handler == proc_douintvec_minmax)) {
 		if (table->maxlen != sizeof(unsigned int))
-			err |= sysctl_err(path, table, "array now allowed");
+			err |= sysctl_err(path, table, "array not allowed");
 	}
 
 	return err;
-- 
1.8.3.1

Re: [PATCH v3 1/6] proc/sysctl: Fix typo in sysctl_check_table_array()

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:35PM -0500, Waiman Long wrote:
> Signed-off-by: Waiman Long <longman@redhat.com>

Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>

  Luis

[PATCH v3 2/6] sysctl: Add kdoc comments to do_proc_do{u}intvec_minmax_conv_param

[Waiman Long]

Kdoc comments are added to the do_proc_dointvec_minmax_conv_param
and do_proc_douintvec_minmax_conv_param structures thare are used
internally for range checking.

The error codes returned by proc_dointvec_minmax() and
proc_douintvec_minmax() are also documented.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/sysctl.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f98f28c..d2aa6b4 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2500,6 +2500,15 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 }
 #endif
 
+/**
+ * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
+ * @min: pointer to minimum allowable value
+ * @max: pointer to maximum allowable value
+ *
+ * The do_proc_dointvec_minmax_conv_param structure provides the
+ * minimum and maximum values for doing range checking for those sysctl
+ * parameters that use the proc_dointvec_minmax() handler.
+ */
 struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
@@ -2543,7 +2552,7 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
  * This routine will ensure the values are within the range specified by
  * table->extra1 (min) and table->extra2 (max).
  *
- * Returns 0 on success.
+ * Returns 0 on success or -EINVAL on write when the range check fails.
  */
 int proc_dointvec_minmax(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -2556,6 +2565,15 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 				do_proc_dointvec_minmax_conv, &param);
 }
 
+/**
+ * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure
+ * @min: pointer to minimum allowable value
+ * @max: pointer to maximum allowable value
+ *
+ * The do_proc_douintvec_minmax_conv_param structure provides the
+ * minimum and maximum values for doing range checking for those sysctl
+ * parameters that use the proc_douintvec_minmax() handler.
+ */
 struct do_proc_douintvec_minmax_conv_param {
 	unsigned int *min;
 	unsigned int *max;
@@ -2603,7 +2621,7 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
  * check for UINT_MAX to avoid having to support wrap around uses from
  * userspace.
  *
- * Returns 0 on success.
+ * Returns 0 on success or -ERANGE on write when the range check fails.
  */
 int proc_douintvec_minmax(struct ctl_table *table, int write,
 			  void __user *buffer, size_t *lenp, loff_t *ppos)
-- 
1.8.3.1

Re: [PATCH v3 2/6] sysctl: Add kdoc comments to do_proc_do{u}intvec_minmax_conv_param

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:36PM -0500, Waiman Long wrote:
> Kdoc comments are added to the do_proc_dointvec_minmax_conv_param
> and do_proc_douintvec_minmax_conv_param structures thare are used
> internally for range checking.
> 
> The error codes returned by proc_dointvec_minmax() and
> proc_douintvec_minmax() are also documented.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>

Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>

  Luis

[PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Waiman Long]

When minimum/maximum values are specified for a sysctl parameter in
the ctl_table structure with proc_dointvec_minmax() handler, update
to that parameter will fail with error if the given value is outside
of the required range.

There are use cases where it may be better to clamp the value of
the sysctl parameter to the given range without failing the update,
especially if the users are not aware of the actual range limits.
Reading the value back after the update will now be a good practice
to see if the provided value exceeds the range limits.

To provide this less restrictive form of range checking, a new flags
field is added to the ctl_table structure. The new field is a 16-bit
value that just fits into the hole left by the 16-bit umode_t field
without increasing the size of the structure.

When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
any update from the userspace will be clamped to the given range
without error.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/sysctl.h | 14 ++++++++++++++
 kernel/sysctl.c        | 48 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index b769ecf..448aa72 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -116,6 +116,7 @@ struct ctl_table
 	void *data;
 	int maxlen;
 	umode_t mode;
+	uint16_t flags;
 	struct ctl_table *child;	/* Deprecated */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	struct ctl_table_poll *poll;
@@ -123,6 +124,19 @@ struct ctl_table
 	void *extra2;
 } __randomize_layout;
 
+/**
+ * enum ctl_table_flags - flags for the ctl table
+ *
+ * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
+ *	flexibly clamped to min/max range in case the user provided
+ *	an incorrect value.
+ *
+ * At most 16 different flags will be allowed.
+ */
+enum ctl_table_flags {
+	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
+};
+
 struct ctl_node {
 	struct rb_node node;
 	struct ctl_table_header *header;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index d2aa6b4..af351ed 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2504,6 +2504,7 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
  * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
  * @min: pointer to minimum allowable value
  * @max: pointer to maximum allowable value
+ * @flags: pointer to flags
  *
  * The do_proc_dointvec_minmax_conv_param structure provides the
  * minimum and maximum values for doing range checking for those sysctl
@@ -2512,6 +2513,7 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
 struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
+	uint16_t *flags;
 };
 
 static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
@@ -2521,9 +2523,21 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 	struct do_proc_dointvec_minmax_conv_param *param = data;
 	if (write) {
 		int val = *negp ? -*lvalp : *lvalp;
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
-			return -EINVAL;
+		bool clamp = param->flags &&
+			   (*param->flags & CTL_FLAGS_CLAMP_RANGE);
+
+		if (param->min && *param->min > val) {
+			if (clamp)
+				val = *param->min;
+			else
+				return -EINVAL;
+		}
+		if (param->max && *param->max < val) {
+			if (clamp)
+				val = *param->max;
+			else
+				return -EINVAL;
+		}
 		*valp = val;
 	} else {
 		int val = *valp;
@@ -2552,7 +2566,8 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
  * This routine will ensure the values are within the range specified by
  * table->extra1 (min) and table->extra2 (max).
  *
- * Returns 0 on success or -EINVAL on write when the range check fails.
+ * Returns 0 on success or -EINVAL on write when the range check fails
+ * without the CTL_FLAGS_CLAMP_RANGE flag.
  */
 int proc_dointvec_minmax(struct ctl_table *table, int write,
 		  void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -2560,6 +2575,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 	struct do_proc_dointvec_minmax_conv_param param = {
 		.min = (int *) table->extra1,
 		.max = (int *) table->extra2,
+		.flags = &table->flags,
 	};
 	return do_proc_dointvec(table, write, buffer, lenp, ppos,
 				do_proc_dointvec_minmax_conv, &param);
@@ -2569,6 +2585,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
  * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure
  * @min: pointer to minimum allowable value
  * @max: pointer to maximum allowable value
+ * @flags: pointer to flags
  *
  * The do_proc_douintvec_minmax_conv_param structure provides the
  * minimum and maximum values for doing range checking for those sysctl
@@ -2577,6 +2594,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 struct do_proc_douintvec_minmax_conv_param {
 	unsigned int *min;
 	unsigned int *max;
+	uint16_t *flags;
 };
 
 static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
@@ -2587,14 +2605,24 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 
 	if (write) {
 		unsigned int val = *lvalp;
+		bool clamp = param->flags &&
+			   (*param->flags & CTL_FLAGS_CLAMP_RANGE);
 
 		if (*lvalp > UINT_MAX)
 			return -EINVAL;
 
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
-			return -ERANGE;
-
+		if (param->min && *param->min > val) {
+			if (clamp)
+				val = *param->min;
+			else
+				return -ERANGE;
+		}
+		if (param->max && *param->max < val) {
+			if (clamp)
+				val = *param->max;
+			else
+				return -ERANGE;
+		}
 		*valp = val;
 	} else {
 		unsigned int val = *valp;
@@ -2621,7 +2649,8 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
  * check for UINT_MAX to avoid having to support wrap around uses from
  * userspace.
  *
- * Returns 0 on success or -ERANGE on write when the range check fails.
+ * Returns 0 on success or -ERANGE on write when the range check fails
+ * without the CTL_FLAGS_CLAMP_RANGE flag.
  */
 int proc_douintvec_minmax(struct ctl_table *table, int write,
 			  void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -2629,6 +2658,7 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
 	struct do_proc_douintvec_minmax_conv_param param = {
 		.min = (unsigned int *) table->extra1,
 		.max = (unsigned int *) table->extra2,
+		.flags = &table->flags,
 	};
 	return do_proc_douintvec(table, write, buffer, lenp, ppos,
 				 do_proc_douintvec_minmax_conv, &param);
-- 
1.8.3.1

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Andrew Morton]

On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:

> When minimum/maximum values are specified for a sysctl parameter in
> the ctl_table structure with proc_dointvec_minmax() handler, update
> to that parameter will fail with error if the given value is outside
> of the required range.
> 
> There are use cases where it may be better to clamp the value of
> the sysctl parameter to the given range without failing the update,
> especially if the users are not aware of the actual range limits.
> Reading the value back after the update will now be a good practice
> to see if the provided value exceeds the range limits.
> 
> To provide this less restrictive form of range checking, a new flags
> field is added to the ctl_table structure. The new field is a 16-bit
> value that just fits into the hole left by the 16-bit umode_t field
> without increasing the size of the structure.
> 
> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
> any update from the userspace will be clamped to the given range
> without error.
> 
> ...
>
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -116,6 +116,7 @@ struct ctl_table
>  	void *data;
>  	int maxlen;
>  	umode_t mode;
> +	uint16_t flags;

It would be nice to make this have type `enum ctl_table_flags', but I
guess there's then no reliable way of forcing it to be 16-bit.

I guess this is the best we can do...

--- a/include/linux/sysctl.h~sysctl-add-flags-to-support-min-max-range-clamping-fix
+++ a/include/linux/sysctl.h
@@ -116,7 +116,7 @@ struct ctl_table
 	void *data;
 	int maxlen;
 	umode_t mode;
-	uint16_t flags;
+	uint16_t flags;			/* enum ctl_table_flags */
 	struct ctl_table *child;	/* Deprecated */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	struct ctl_table_poll *poll;
@@ -125,7 +125,7 @@ struct ctl_table
 } __randomize_layout;
 
 /**
- * enum ctl_table_flags - flags for the ctl table
+ * enum ctl_table_flags - flags for the ctl table (struct ctl_table.flags)
  *
  * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
  *	flexibly clamped to min/max range in case the user provided


>  	struct ctl_table *child;	/* Deprecated */
>  	proc_handler *proc_handler;	/* Callback for text formatting */
>  	struct ctl_table_poll *poll;

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Waiman Long]

On 03/01/2018 04:31 PM, Andrew Morton wrote:
> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
>
>> When minimum/maximum values are specified for a sysctl parameter in
>> the ctl_table structure with proc_dointvec_minmax() handler, update
>> to that parameter will fail with error if the given value is outside
>> of the required range.
>>
>> There are use cases where it may be better to clamp the value of
>> the sysctl parameter to the given range without failing the update,
>> especially if the users are not aware of the actual range limits.
>> Reading the value back after the update will now be a good practice
>> to see if the provided value exceeds the range limits.
>>
>> To provide this less restrictive form of range checking, a new flags
>> field is added to the ctl_table structure. The new field is a 16-bit
>> value that just fits into the hole left by the 16-bit umode_t field
>> without increasing the size of the structure.
>>
>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
>> any update from the userspace will be clamped to the given range
>> without error.
>>
>> ...
>>
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -116,6 +116,7 @@ struct ctl_table
>>  	void *data;
>>  	int maxlen;
>>  	umode_t mode;
>> +	uint16_t flags;
> It would be nice to make this have type `enum ctl_table_flags', but I
> guess there's then no reliable way of forcing it to be 16-bit.
> I guess this is the best we can do...
>
> --- a/include/linux/sysctl.h~sysctl-add-flags-to-support-min-max-range-clamping-fix
> +++ a/include/linux/sysctl.h
> @@ -116,7 +116,7 @@ struct ctl_table
>  	void *data;
>  	int maxlen;
>  	umode_t mode;
> -	uint16_t flags;
> +	uint16_t flags;			/* enum ctl_table_flags */
>  	struct ctl_table *child;	/* Deprecated */
>  	proc_handler *proc_handler;	/* Callback for text formatting */
>  	struct ctl_table_poll *poll;
> @@ -125,7 +125,7 @@ struct ctl_table
>  } __randomize_layout;
>  
>  /**
> - * enum ctl_table_flags - flags for the ctl table
> + * enum ctl_table_flags - flags for the ctl table (struct ctl_table.flags)
>   *
>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>   *	flexibly clamped to min/max range in case the user provided

The flags field is actually a bit mask of various bits defined in the
enum type. So I will not change the type of flags to enum
ctl_table_flags as it is not strictly an enum type. However, I can
certainly add the suggested comment change in the next version of the patch.

Cheers,
Longman

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
> 
> > When minimum/maximum values are specified for a sysctl parameter in
> > the ctl_table structure with proc_dointvec_minmax() handler, update
> > to that parameter will fail with error if the given value is outside
> > of the required range.
> > 
> > There are use cases where it may be better to clamp the value of
> > the sysctl parameter to the given range without failing the update,
> > especially if the users are not aware of the actual range limits.
> > Reading the value back after the update will now be a good practice
> > to see if the provided value exceeds the range limits.
> > 
> > To provide this less restrictive form of range checking, a new flags
> > field is added to the ctl_table structure. The new field is a 16-bit
> > value that just fits into the hole left by the 16-bit umode_t field
> > without increasing the size of the structure.
> > 
> > When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
> > any update from the userspace will be clamped to the given range
> > without error.
> > 
> > ...
> >
> > --- a/include/linux/sysctl.h
> > +++ b/include/linux/sysctl.h
> > @@ -116,6 +116,7 @@ struct ctl_table
> >  	void *data;
> >  	int maxlen;
> >  	umode_t mode;
> > +	uint16_t flags;
> 
> It would be nice to make this have type `enum ctl_table_flags', but I
> guess there's then no reliable way of forcing it to be 16-bit.
> 
> I guess this is the best we can do...
> 

We can add this to the enum:

enum ctl_table_flags {                                                                                                                                                                       
       CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
+	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
}; 


Then also:

#define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)

at the end of the definition, then a helper which can be used during
parsing:

static int check_ctl_table_flags(u16 flags)
{
	if (flags & ~(CTL_TABLE_FLAGS_ALL))
		return -ERANGE;
	return 0;
}

Waiman please evaluate and add.

  Luis

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 05:51:09PM +0000, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
> > On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
> > 
> > > When minimum/maximum values are specified for a sysctl parameter in
> > > the ctl_table structure with proc_dointvec_minmax() handler, update
> > > to that parameter will fail with error if the given value is outside
> > > of the required range.
> > > 
> > > There are use cases where it may be better to clamp the value of
> > > the sysctl parameter to the given range without failing the update,
> > > especially if the users are not aware of the actual range limits.
> > > Reading the value back after the update will now be a good practice
> > > to see if the provided value exceeds the range limits.
> > > 
> > > To provide this less restrictive form of range checking, a new flags
> > > field is added to the ctl_table structure. The new field is a 16-bit
> > > value that just fits into the hole left by the 16-bit umode_t field
> > > without increasing the size of the structure.
> > > 
> > > When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
> > > any update from the userspace will be clamped to the given range
> > > without error.
> > > 
> > > ...
> > >
> > > --- a/include/linux/sysctl.h
> > > +++ b/include/linux/sysctl.h
> > > @@ -116,6 +116,7 @@ struct ctl_table
> > >  	void *data;
> > >  	int maxlen;
> > >  	umode_t mode;
> > > +	uint16_t flags;
> > 
> > It would be nice to make this have type `enum ctl_table_flags', but I
> > guess there's then no reliable way of forcing it to be 16-bit.
> > 
> > I guess this is the best we can do...
> > 
> 
> We can add this to the enum:
> 
> enum ctl_table_flags {                                                                                                                                                                       
>        CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
> +	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
> }; 
> 
> 
> Then also:
> 
> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)
> 
> at the end of the definition, then a helper which can be used during
> parsing:
> 
> static int check_ctl_table_flags(u16 flags)
> {
> 	if (flags & ~(CTL_TABLE_FLAGS_ALL))
> 		return -ERANGE;
> 	return 0;
> }
> 
> Waiman please evaluate and add.

Also, I guess we have ... max bit used and max allowed (16) really, where one is the
max allowed bit field given current definitions, the other is the max flag possible
setting in the future. We might as well go with the smaller one, which is the current
max, so it can just be

enum ctl_table_flags {
	CTL_FLAGS_CLAMP_RANGE	 = BIT(0),
	__CTL_FLAGS_CLAMP_MAX    = BIT(1),
};


#define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX))-1)

That way we just check against the actual max defined, now the max allowed on
the entire flag setting.

  Luis

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Waiman Long]

On 03/08/2018 12:57 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 08, 2018 at 05:51:09PM +0000, Luis R. Rodriguez wrote:
>> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
>>> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
>>>
>>>> When minimum/maximum values are specified for a sysctl parameter in
>>>> the ctl_table structure with proc_dointvec_minmax() handler, update
>>>> to that parameter will fail with error if the given value is outside
>>>> of the required range.
>>>>
>>>> There are use cases where it may be better to clamp the value of
>>>> the sysctl parameter to the given range without failing the update,
>>>> especially if the users are not aware of the actual range limits.
>>>> Reading the value back after the update will now be a good practice
>>>> to see if the provided value exceeds the range limits.
>>>>
>>>> To provide this less restrictive form of range checking, a new flags
>>>> field is added to the ctl_table structure. The new field is a 16-bit
>>>> value that just fits into the hole left by the 16-bit umode_t field
>>>> without increasing the size of the structure.
>>>>
>>>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
>>>> any update from the userspace will be clamped to the given range
>>>> without error.
>>>>
>>>> ...
>>>>
>>>> --- a/include/linux/sysctl.h
>>>> +++ b/include/linux/sysctl.h
>>>> @@ -116,6 +116,7 @@ struct ctl_table
>>>>  	void *data;
>>>>  	int maxlen;
>>>>  	umode_t mode;
>>>> +	uint16_t flags;
>>> It would be nice to make this have type `enum ctl_table_flags', but I
>>> guess there's then no reliable way of forcing it to be 16-bit.
>>>
>>> I guess this is the best we can do...
>>>
>> We can add this to the enum:
>>
>> enum ctl_table_flags {                                                                                                                                                                       
>>        CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
>> +	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
>> }; 
>>
>>
>> Then also:
>>
>> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)
>>
>> at the end of the definition, then a helper which can be used during
>> parsing:
>>
>> static int check_ctl_table_flags(u16 flags)
>> {
>> 	if (flags & ~(CTL_TABLE_FLAGS_ALL))
>> 		return -ERANGE;
>> 	return 0;
>> }
>>
>> Waiman please evaluate and add.
> Also, I guess we have ... max bit used and max allowed (16) really, where one is the
> max allowed bit field given current definitions, the other is the max flag possible
> setting in the future. We might as well go with the smaller one, which is the current
> max, so it can just be
>
> enum ctl_table_flags {
> 	CTL_FLAGS_CLAMP_RANGE	 = BIT(0),
> 	__CTL_FLAGS_CLAMP_MAX    = BIT(1),
> };
>
>
> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX))-1)
>
> That way we just check against the actual max defined, now the max allowed on
> the entire flag setting.
>
>   Luis

Yes, I can certainly add check to see if the flags are out of range.
However, I would like to know your opinion of what to do when an invalid
flag bit is set. Do we just print a warning in the ring buffer or fail
the registration of the ctl table?

Cheers,
Longman

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 02:35:32PM -0500, Waiman Long wrote:
> On 03/08/2018 12:57 PM, Luis R. Rodriguez wrote:
> > On Thu, Mar 08, 2018 at 05:51:09PM +0000, Luis R. Rodriguez wrote:
> >> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
> >>> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
> >>>
> >>>> When minimum/maximum values are specified for a sysctl parameter in
> >>>> the ctl_table structure with proc_dointvec_minmax() handler, update
> >>>> to that parameter will fail with error if the given value is outside
> >>>> of the required range.
> >>>>
> >>>> There are use cases where it may be better to clamp the value of
> >>>> the sysctl parameter to the given range without failing the update,
> >>>> especially if the users are not aware of the actual range limits.
> >>>> Reading the value back after the update will now be a good practice
> >>>> to see if the provided value exceeds the range limits.
> >>>>
> >>>> To provide this less restrictive form of range checking, a new flags
> >>>> field is added to the ctl_table structure. The new field is a 16-bit
> >>>> value that just fits into the hole left by the 16-bit umode_t field
> >>>> without increasing the size of the structure.
> >>>>
> >>>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
> >>>> any update from the userspace will be clamped to the given range
> >>>> without error.
> >>>>
> >>>> ...
> >>>>
> >>>> --- a/include/linux/sysctl.h
> >>>> +++ b/include/linux/sysctl.h
> >>>> @@ -116,6 +116,7 @@ struct ctl_table
> >>>>  	void *data;
> >>>>  	int maxlen;
> >>>>  	umode_t mode;
> >>>> +	uint16_t flags;
> >>> It would be nice to make this have type `enum ctl_table_flags', but I
> >>> guess there's then no reliable way of forcing it to be 16-bit.
> >>>
> >>> I guess this is the best we can do...
> >>>
> >> We can add this to the enum:
> >>
> >> enum ctl_table_flags {                                                                                                                                                                       
> >>        CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
> >> +	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
> >> }; 
> >>
> >>
> >> Then also:
> >>
> >> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)
> >>
> >> at the end of the definition, then a helper which can be used during
> >> parsing:
> >>
> >> static int check_ctl_table_flags(u16 flags)
> >> {
> >> 	if (flags & ~(CTL_TABLE_FLAGS_ALL))
> >> 		return -ERANGE;
> >> 	return 0;
> >> }
> >>
> >> Waiman please evaluate and add.
> > Also, I guess we have ... max bit used and max allowed (16) really, where one is the
> > max allowed bit field given current definitions, the other is the max flag possible
> > setting in the future. We might as well go with the smaller one, which is the current
> > max, so it can just be
> >
> > enum ctl_table_flags {
> > 	CTL_FLAGS_CLAMP_RANGE	 = BIT(0),
> > 	__CTL_FLAGS_CLAMP_MAX    = BIT(1),
> > };
> >
> >
> > #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX))-1)
> >
> > That way we just check against the actual max defined, now the max allowed on
> > the entire flag setting.
> >
> >   Luis
> 
> Yes, I can certainly add check to see if the flags are out of range.
> However, I would like to know your opinion of what to do when an invalid
> flag bit is set. Do we just print a warning in the ring buffer or fail
> the registration of the ctl table?

We should fail setting. See sysctl_check_table_array(), that should just
reject the entry.

  Luis

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Waiman Long]

On 03/08/2018 03:45 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 08, 2018 at 02:35:32PM -0500, Waiman Long wrote:
>> On 03/08/2018 12:57 PM, Luis R. Rodriguez wrote:
>>> On Thu, Mar 08, 2018 at 05:51:09PM +0000, Luis R. Rodriguez wrote:
>>>> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
>>>>> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
>>>>>
>>>>>> When minimum/maximum values are specified for a sysctl parameter in
>>>>>> the ctl_table structure with proc_dointvec_minmax() handler, update
>>>>>> to that parameter will fail with error if the given value is outside
>>>>>> of the required range.
>>>>>>
>>>>>> There are use cases where it may be better to clamp the value of
>>>>>> the sysctl parameter to the given range without failing the update,
>>>>>> especially if the users are not aware of the actual range limits.
>>>>>> Reading the value back after the update will now be a good practice
>>>>>> to see if the provided value exceeds the range limits.
>>>>>>
>>>>>> To provide this less restrictive form of range checking, a new flags
>>>>>> field is added to the ctl_table structure. The new field is a 16-bit
>>>>>> value that just fits into the hole left by the 16-bit umode_t field
>>>>>> without increasing the size of the structure.
>>>>>>
>>>>>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
>>>>>> any update from the userspace will be clamped to the given range
>>>>>> without error.
>>>>>>
>>>>>> ...
>>>>>>
>>>>>> --- a/include/linux/sysctl.h
>>>>>> +++ b/include/linux/sysctl.h
>>>>>> @@ -116,6 +116,7 @@ struct ctl_table
>>>>>>  	void *data;
>>>>>>  	int maxlen;
>>>>>>  	umode_t mode;
>>>>>> +	uint16_t flags;
>>>>> It would be nice to make this have type `enum ctl_table_flags', but I
>>>>> guess there's then no reliable way of forcing it to be 16-bit.
>>>>>
>>>>> I guess this is the best we can do...
>>>>>
>>>> We can add this to the enum:
>>>>
>>>> enum ctl_table_flags {                                                                                                                                                                       
>>>>        CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
>>>> +	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
>>>> }; 
>>>>
>>>>
>>>> Then also:
>>>>
>>>> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)
>>>>
>>>> at the end of the definition, then a helper which can be used during
>>>> parsing:
>>>>
>>>> static int check_ctl_table_flags(u16 flags)
>>>> {
>>>> 	if (flags & ~(CTL_TABLE_FLAGS_ALL))
>>>> 		return -ERANGE;
>>>> 	return 0;
>>>> }
>>>>
>>>> Waiman please evaluate and add.
>>> Also, I guess we have ... max bit used and max allowed (16) really, where one is the
>>> max allowed bit field given current definitions, the other is the max flag possible
>>> setting in the future. We might as well go with the smaller one, which is the current
>>> max, so it can just be
>>>
>>> enum ctl_table_flags {
>>> 	CTL_FLAGS_CLAMP_RANGE	 = BIT(0),
>>> 	__CTL_FLAGS_CLAMP_MAX    = BIT(1),
>>> };
>>>
>>>
>>> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX))-1)
>>>
>>> That way we just check against the actual max defined, now the max allowed on
>>> the entire flag setting.
>>>
>>>   Luis
>> Yes, I can certainly add check to see if the flags are out of range.
>> However, I would like to know your opinion of what to do when an invalid
>> flag bit is set. Do we just print a warning in the ring buffer or fail
>> the registration of the ctl table?
> We should fail setting. See sysctl_check_table_array(), that should just
> reject the entry.
>
>   Luis

OK, got it.

Cheers,
Longman

Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Waiman Long]

On 03/08/2018 12:51 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
>> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> wrote:
>>
>>> When minimum/maximum values are specified for a sysctl parameter in
>>> the ctl_table structure with proc_dointvec_minmax() handler, update
>>> to that parameter will fail with error if the given value is outside
>>> of the required range.
>>>
>>> There are use cases where it may be better to clamp the value of
>>> the sysctl parameter to the given range without failing the update,
>>> especially if the users are not aware of the actual range limits.
>>> Reading the value back after the update will now be a good practice
>>> to see if the provided value exceeds the range limits.
>>>
>>> To provide this less restrictive form of range checking, a new flags
>>> field is added to the ctl_table structure. The new field is a 16-bit
>>> value that just fits into the hole left by the 16-bit umode_t field
>>> without increasing the size of the structure.
>>>
>>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
>>> any update from the userspace will be clamped to the given range
>>> without error.
>>>
>>> ...
>>>
>>> --- a/include/linux/sysctl.h
>>> +++ b/include/linux/sysctl.h
>>> @@ -116,6 +116,7 @@ struct ctl_table
>>>  	void *data;
>>>  	int maxlen;
>>>  	umode_t mode;
>>> +	uint16_t flags;
>> It would be nice to make this have type `enum ctl_table_flags', but I
>> guess there's then no reliable way of forcing it to be 16-bit.
>>
>> I guess this is the best we can do...
>>

Actually, I can make the flags just an unsigned integer. I chose
uint16_t for backporting purpose. For upstream code, it is not a concern
at all. That will allow more bits for expansion in the future.

> We can add this to the enum:
>
> enum ctl_table_flags {                                                                                                                                                                       
>        CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
> +	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
> }; 
>
>
> Then also:
>
> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)
>
> at the end of the definition, then a helper which can be used during
> parsing:
>
> static int check_ctl_table_flags(u16 flags)
> {
> 	if (flags & ~(CTL_TABLE_FLAGS_ALL))
> 		return -ERANGE;
> 	return 0;
> }

I don't think that helper function works unless the parameter is changed
to int instead of u16. I will just change the flags to uint and forget
about checking for out-of-range bit.

Cheers,
Longman

[PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

Even with clamped sysctl parameters, it is still not that straight
forward to figure out the exact range of those parameters. One may
try to write extreme parameter values to see if they get clamped.
To make it easier, a warning with the expected range will now be
printed in the kernel ring buffer when a clamped sysctl parameter
receives an out of range value.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 448aa72..3db57af 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -130,11 +130,14 @@ struct ctl_table
  * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
  *	flexibly clamped to min/max range in case the user provided
  *	an incorrect value.
+ * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
+ * 	had been issued for that entry.
  *
  * At most 16 different flags will be allowed.
  */
 enum ctl_table_flags {
 	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
+	CTL_FLAGS_OOR_WARNED		= BIT(1),
 };
 
 struct ctl_node {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index af351ed..6c68e77 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2505,6 +2505,7 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
  * @min: pointer to minimum allowable value
  * @max: pointer to maximum allowable value
  * @flags: pointer to flags
+ * @name: sysctl parameter name
  *
  * The do_proc_dointvec_minmax_conv_param structure provides the
  * minimum and maximum values for doing range checking for those sysctl
@@ -2514,31 +2515,50 @@ struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
 	uint16_t *flags;
+	const char *name;
 };
 
+/* Out of range warning message */
+#define proc_ctl_warn(type, ...)				  \
+	pr_warn("Kernel parameter \"%s\" was set out of range [%" \
+	#type ", %" #type "], clamped to %" #type ".\n", __VA_ARGS__)
+
 static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 					int *valp,
 					int write, void *data)
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
+
 	if (write) {
 		int val = *negp ? -*lvalp : *lvalp;
+		bool clamped = false;
 		bool clamp = param->flags &&
 			   (*param->flags & CTL_FLAGS_CLAMP_RANGE);
 
 		if (param->min && *param->min > val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->min;
-			else
+				clamped = true;
+			} else {
 				return -EINVAL;
+			}
 		}
 		if (param->max && *param->max < val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->max;
-			else
+				clamped = true;
+			} else {
 				return -EINVAL;
+			}
 		}
 		*valp = val;
+		if (clamped && param->name &&
+		   !(*param->flags & CTL_FLAGS_OOR_WARNED)) {
+			proc_ctl_warn(d, param->name,
+				param->min ? *param->min : -INT_MAX,
+				param->max ? *param->max :  INT_MAX, val);
+			*param->flags |= CTL_FLAGS_OOR_WARNED;
+		}
 	} else {
 		int val = *valp;
 		if (val < 0) {
@@ -2576,6 +2596,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 		.min = (int *) table->extra1,
 		.max = (int *) table->extra2,
 		.flags = &table->flags,
+		.name  = table->procname,
 	};
 	return do_proc_dointvec(table, write, buffer, lenp, ppos,
 				do_proc_dointvec_minmax_conv, &param);
@@ -2586,6 +2607,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
  * @min: pointer to minimum allowable value
  * @max: pointer to maximum allowable value
  * @flags: pointer to flags
+ * @name: sysctl parameter name
  *
  * The do_proc_douintvec_minmax_conv_param structure provides the
  * minimum and maximum values for doing range checking for those sysctl
@@ -2595,6 +2617,7 @@ struct do_proc_douintvec_minmax_conv_param {
 	unsigned int *min;
 	unsigned int *max;
 	uint16_t *flags;
+	const char *name;
 };
 
 static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
@@ -2605,6 +2628,7 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 
 	if (write) {
 		unsigned int val = *lvalp;
+		bool clamped = false;
 		bool clamp = param->flags &&
 			   (*param->flags & CTL_FLAGS_CLAMP_RANGE);
 
@@ -2612,18 +2636,29 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 			return -EINVAL;
 
 		if (param->min && *param->min > val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->min;
-			else
+				clamped = true;
+			} else {
 				return -ERANGE;
+			}
 		}
 		if (param->max && *param->max < val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->max;
-			else
+				clamped = true;
+			} else {
 				return -ERANGE;
+			}
 		}
 		*valp = val;
+		if (clamped && param->name &&
+		   !(*param->flags & CTL_FLAGS_OOR_WARNED)) {
+			proc_ctl_warn(u, param->name,
+				param->min ? *param->min : 0,
+				param->max ? *param->max : UINT_MAX, val);
+			*param->flags |= CTL_FLAGS_OOR_WARNED;
+		}
 	} else {
 		unsigned int val = *valp;
 		*lvalp = (unsigned long) val;
@@ -2659,6 +2694,7 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
 		.min = (unsigned int *) table->extra1,
 		.max = (unsigned int *) table->extra2,
 		.flags = &table->flags,
+		.name  = table->procname,
 	};
 	return do_proc_douintvec(table, write, buffer, lenp, ppos,
 				 do_proc_douintvec_minmax_conv, &param);
-- 
1.8.3.1

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Andrew Morton]

On Thu,  1 Mar 2018 12:43:38 -0500 Waiman Long <longman@redhat.com> wrote:

> Even with clamped sysctl parameters, it is still not that straight
> forward to figure out the exact range of those parameters. One may
> try to write extreme parameter values to see if they get clamped.
> To make it easier, a warning with the expected range will now be
> printed in the kernel ring buffer when a clamped sysctl parameter
> receives an out of range value.
> 
> ...
>
> +		if (clamped && param->name &&
> +		   !(*param->flags & CTL_FLAGS_OOR_WARNED)) {
> +			proc_ctl_warn(d, param->name,
> +				param->min ? *param->min : -INT_MAX,
> +				param->max ? *param->max :  INT_MAX, val);
> +			*param->flags |= CTL_FLAGS_OOR_WARNED;
> +		}

The handling of ctl_table.flags looks racy on SMP or preemptible. 
That's not at all a serious problem in this usage, but such handling of
ctl_table.flags may be a problem in the future.  Which means that if
some future user of this field *is* sensitive to races then people are
going to have to come back to this code and add the needed locking.

So we should at least think about what that locking is to be, and
document it in some fashion.  Do we already hold an appropriate lock at
this time?  If so, what is it?

If some such future user of ctl_table.flags has to add a new lock to
the ctl_table for this purpose then we just eliminated your use-16-bit
space saving trick and we may as well use a ulong and operate on it
with bitops.

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 03/01/2018 04:38 PM, Andrew Morton wrote:
> On Thu,  1 Mar 2018 12:43:38 -0500 Waiman Long <longman@redhat.com> wrote:
>
>> Even with clamped sysctl parameters, it is still not that straight
>> forward to figure out the exact range of those parameters. One may
>> try to write extreme parameter values to see if they get clamped.
>> To make it easier, a warning with the expected range will now be
>> printed in the kernel ring buffer when a clamped sysctl parameter
>> receives an out of range value.
>>
>> ...
>>
>> +		if (clamped && param->name &&
>> +		   !(*param->flags & CTL_FLAGS_OOR_WARNED)) {
>> +			proc_ctl_warn(d, param->name,
>> +				param->min ? *param->min : -INT_MAX,
>> +				param->max ? *param->max :  INT_MAX, val);
>> +			*param->flags |= CTL_FLAGS_OOR_WARNED;
>> +		}
> The handling of ctl_table.flags looks racy on SMP or preemptible. 
> That's not at all a serious problem in this usage, but such handling of
> ctl_table.flags may be a problem in the future.  Which means that if
> some future user of this field *is* sensitive to races then people are
> going to have to come back to this code and add the needed locking.
>
> So we should at least think about what that locking is to be, and
> document it in some fashion.  Do we already hold an appropriate lock at
> this time?  If so, what is it?
>
> If some such future user of ctl_table.flags has to add a new lock to
> the ctl_table for this purpose then we just eliminated your use-16-bit
> space saving trick and we may as well use a ulong and operate on it
> with bitops.

We don't actually need locking if it is only the flags that we are
worrying about. Doing some kind of atomic bit setting should be enough.
I should probably add some comment to elaborate a bit more on this.
Thanks for reminding me about this forward looking concern.

Cheers,
Longman

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
> Even with clamped sysctl parameters, it is still not that straight
> forward to figure out the exact range of those parameters. One may
> try to write extreme parameter values to see if they get clamped.
> To make it easier, a warning with the expected range will now be
> printed in the kernel ring buffer when a clamped sysctl parameter
> receives an out of range value.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  include/linux/sysctl.h |  3 +++
>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 448aa72..3db57af 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -130,11 +130,14 @@ struct ctl_table
>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>   *	flexibly clamped to min/max range in case the user provided
>   *	an incorrect value.
> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
> + * 	had been issued for that entry.
>   *
>   * At most 16 different flags will be allowed.
>   */
>  enum ctl_table_flags {
>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
>  };
>  
>  struct ctl_node {
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index af351ed..6c68e77 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2505,6 +2505,7 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>   * @min: pointer to minimum allowable value
>   * @max: pointer to maximum allowable value
>   * @flags: pointer to flags
> + * @name: sysctl parameter name
>   *
>   * The do_proc_dointvec_minmax_conv_param structure provides the
>   * minimum and maximum values for doing range checking for those sysctl
> @@ -2514,31 +2515,50 @@ struct do_proc_dointvec_minmax_conv_param {
>  	int *min;
>  	int *max;
>  	uint16_t *flags;
> +	const char *name;
>  };
>  
> +/* Out of range warning message */
> +#define proc_ctl_warn(type, ...)				  \
> +	pr_warn("Kernel parameter \"%s\" was set out of range [%" \
> +	#type ", %" #type "], clamped to %" #type ".\n", __VA_ARGS__)
> +

Usage of pr_*() macros are best used if and when you first
also define

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Also if we can avoid adding a new define for a pr_warn wrapper
even better. I'd much prefer to have a helper static void routine
which gets some params and does the print than have a define.

However you choose, I just don't want a #define around a simple
pr_warn().

   Luis

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 03/08/2018 01:11 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
>> Even with clamped sysctl parameters, it is still not that straight
>> forward to figure out the exact range of those parameters. One may
>> try to write extreme parameter values to see if they get clamped.
>> To make it easier, a warning with the expected range will now be
>> printed in the kernel ring buffer when a clamped sysctl parameter
>> receives an out of range value.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  include/linux/sysctl.h |  3 +++
>>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 448aa72..3db57af 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -130,11 +130,14 @@ struct ctl_table
>>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>>   *	flexibly clamped to min/max range in case the user provided
>>   *	an incorrect value.
>> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
>> + * 	had been issued for that entry.
>>   *
>>   * At most 16 different flags will be allowed.
>>   */
>>  enum ctl_table_flags {
>>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
>> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
>>  };
>>  
>>  struct ctl_node {
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index af351ed..6c68e77 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -2505,6 +2505,7 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>>   * @min: pointer to minimum allowable value
>>   * @max: pointer to maximum allowable value
>>   * @flags: pointer to flags
>> + * @name: sysctl parameter name
>>   *
>>   * The do_proc_dointvec_minmax_conv_param structure provides the
>>   * minimum and maximum values for doing range checking for those sysctl
>> @@ -2514,31 +2515,50 @@ struct do_proc_dointvec_minmax_conv_param {
>>  	int *min;
>>  	int *max;
>>  	uint16_t *flags;
>> +	const char *name;
>>  };
>>  
>> +/* Out of range warning message */
>> +#define proc_ctl_warn(type, ...)				  \
>> +	pr_warn("Kernel parameter \"%s\" was set out of range [%" \
>> +	#type ", %" #type "], clamped to %" #type ".\n", __VA_ARGS__)
>> +
> Usage of pr_*() macros are best used if and when you first
> also define
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> Also if we can avoid adding a new define for a pr_warn wrapper
> even better. I'd much prefer to have a helper static void routine
> which gets some params and does the print than have a define.
>
> However you choose, I just don't want a #define around a simple
> pr_warn().
>
>    Luis

Sure. I will rework the patch to eliminate the define.

Cheers,
Longman

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
> Even with clamped sysctl parameters, it is still not that straight
> forward to figure out the exact range of those parameters. One may
> try to write extreme parameter values to see if they get clamped.
> To make it easier, a warning with the expected range will now be
> printed in the kernel ring buffer when a clamped sysctl parameter
> receives an out of range value.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  include/linux/sysctl.h |  3 +++
>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 448aa72..3db57af 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -130,11 +130,14 @@ struct ctl_table
>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>   *	flexibly clamped to min/max range in case the user provided
>   *	an incorrect value.
> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
> + * 	had been issued for that entry.
>   *
>   * At most 16 different flags will be allowed.
>   */
>  enum ctl_table_flags {
>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
>  };

Ugh, no. Now I see why you had to set this flag later.

You are not using this flag to "warn" but rather for an internal
status checker if you have warned or not. Internal flags should
not be something the user sets. If we want a flag for warning
that's one thing. If we need a flag to keep tabs if we have
warned or not that needs to be kept separately and internally,
nothing the user has to do set or reset.

  Luis

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 03/08/2018 01:31 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
>> Even with clamped sysctl parameters, it is still not that straight
>> forward to figure out the exact range of those parameters. One may
>> try to write extreme parameter values to see if they get clamped.
>> To make it easier, a warning with the expected range will now be
>> printed in the kernel ring buffer when a clamped sysctl parameter
>> receives an out of range value.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  include/linux/sysctl.h |  3 +++
>>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 448aa72..3db57af 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -130,11 +130,14 @@ struct ctl_table
>>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>>   *	flexibly clamped to min/max range in case the user provided
>>   *	an incorrect value.
>> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
>> + * 	had been issued for that entry.
>>   *
>>   * At most 16 different flags will be allowed.
>>   */
>>  enum ctl_table_flags {
>>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
>> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
>>  };
> Ugh, no. Now I see why you had to set this flag later.
>
> You are not using this flag to "warn" but rather for an internal
> status checker if you have warned or not. Internal flags should
> not be something the user sets. If we want a flag for warning
> that's one thing. If we need a flag to keep tabs if we have
> warned or not that needs to be kept separately and internally,
> nothing the user has to do set or reset.
>
>   Luis

What I want to do is a printk_once for each sysctl parameter. So the
flag is used as a marker that a warning has been printed.

I do understand that it gets somewhat ugly in the case of msgmni and
shmmni because of the copying back of the flag. Another alternative that
had been suggested by Kees is to use prink_ratelimited. That we don't
need that flag at all.

Cheers,
Longman

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 02:57:09PM -0500, Waiman Long wrote:
> On 03/08/2018 01:31 PM, Luis R. Rodriguez wrote:
> > On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
> >> Even with clamped sysctl parameters, it is still not that straight
> >> forward to figure out the exact range of those parameters. One may
> >> try to write extreme parameter values to see if they get clamped.
> >> To make it easier, a warning with the expected range will now be
> >> printed in the kernel ring buffer when a clamped sysctl parameter
> >> receives an out of range value.
> >>
> >> Signed-off-by: Waiman Long <longman@redhat.com>
> >> ---
> >>  include/linux/sysctl.h |  3 +++
> >>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
> >>  2 files changed, 47 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> >> index 448aa72..3db57af 100644
> >> --- a/include/linux/sysctl.h
> >> +++ b/include/linux/sysctl.h
> >> @@ -130,11 +130,14 @@ struct ctl_table
> >>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
> >>   *	flexibly clamped to min/max range in case the user provided
> >>   *	an incorrect value.
> >> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
> >> + * 	had been issued for that entry.
> >>   *
> >>   * At most 16 different flags will be allowed.
> >>   */
> >>  enum ctl_table_flags {
> >>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
> >> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
> >>  };
> > Ugh, no. Now I see why you had to set this flag later.
> >
> > You are not using this flag to "warn" but rather for an internal
> > status checker if you have warned or not. Internal flags should
> > not be something the user sets. If we want a flag for warning
> > that's one thing. If we need a flag to keep tabs if we have
> > warned or not that needs to be kept separately and internally,
> > nothing the user has to do set or reset.
> >
> >   Luis
> 
> What I want to do is a printk_once for each sysctl parameter. So the
> flag is used as a marker that a warning has been printed.
> 
> I do understand that it gets somewhat ugly in the case of msgmni and
> shmmni because of the copying back of the flag. Another alternative that
> had been suggested by Kees is to use prink_ratelimited. That we don't
> need that flag at all.

However it is done, a user flag should not be used also for internal
flag settings. That's just gross. Internal state machine stuff should
remain far from what the user is able to modify.

Also, why can't it just use pr_warn_once() and be done with it?

  Luis

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 03/08/2018 03:49 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 08, 2018 at 02:57:09PM -0500, Waiman Long wrote:
>> On 03/08/2018 01:31 PM, Luis R. Rodriguez wrote:
>>> On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
>>>> Even with clamped sysctl parameters, it is still not that straight
>>>> forward to figure out the exact range of those parameters. One may
>>>> try to write extreme parameter values to see if they get clamped.
>>>> To make it easier, a warning with the expected range will now be
>>>> printed in the kernel ring buffer when a clamped sysctl parameter
>>>> receives an out of range value.
>>>>
>>>> Signed-off-by: Waiman Long <longman@redhat.com>
>>>> ---
>>>>  include/linux/sysctl.h |  3 +++
>>>>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
>>>>  2 files changed, 47 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>>>> index 448aa72..3db57af 100644
>>>> --- a/include/linux/sysctl.h
>>>> +++ b/include/linux/sysctl.h
>>>> @@ -130,11 +130,14 @@ struct ctl_table
>>>>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>>>>   *	flexibly clamped to min/max range in case the user provided
>>>>   *	an incorrect value.
>>>> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
>>>> + * 	had been issued for that entry.
>>>>   *
>>>>   * At most 16 different flags will be allowed.
>>>>   */
>>>>  enum ctl_table_flags {
>>>>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
>>>> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
>>>>  };
>>> Ugh, no. Now I see why you had to set this flag later.
>>>
>>> You are not using this flag to "warn" but rather for an internal
>>> status checker if you have warned or not. Internal flags should
>>> not be something the user sets. If we want a flag for warning
>>> that's one thing. If we need a flag to keep tabs if we have
>>> warned or not that needs to be kept separately and internally,
>>> nothing the user has to do set or reset.
>>>
>>>   Luis
>> What I want to do is a printk_once for each sysctl parameter. So the
>> flag is used as a marker that a warning has been printed.
>>
>> I do understand that it gets somewhat ugly in the case of msgmni and
>> shmmni because of the copying back of the flag. Another alternative that
>> had been suggested by Kees is to use prink_ratelimited. That we don't
>> need that flag at all.
> However it is done, a user flag should not be used also for internal
> flag settings. That's just gross. Internal state machine stuff should
> remain far from what the user is able to modify.
>
> Also, why can't it just use pr_warn_once() and be done with it?

Different sysctl parameters can use the same minmax proc_handler. Using
pr_warn_once() means mistake in one will prevent mistakes in other
parameters from showing up.

Cheers,
Longman 

Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 04:40:17PM -0500, Waiman Long wrote:
> On 03/08/2018 03:49 PM, Luis R. Rodriguez wrote:
> > On Thu, Mar 08, 2018 at 02:57:09PM -0500, Waiman Long wrote:
> >> On 03/08/2018 01:31 PM, Luis R. Rodriguez wrote:
> >>> On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
> >>>> Even with clamped sysctl parameters, it is still not that straight
> >>>> forward to figure out the exact range of those parameters. One may
> >>>> try to write extreme parameter values to see if they get clamped.
> >>>> To make it easier, a warning with the expected range will now be
> >>>> printed in the kernel ring buffer when a clamped sysctl parameter
> >>>> receives an out of range value.
> >>>>
> >>>> Signed-off-by: Waiman Long <longman@redhat.com>
> >>>> ---
> >>>>  include/linux/sysctl.h |  3 +++
> >>>>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
> >>>>  2 files changed, 47 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> >>>> index 448aa72..3db57af 100644
> >>>> --- a/include/linux/sysctl.h
> >>>> +++ b/include/linux/sysctl.h
> >>>> @@ -130,11 +130,14 @@ struct ctl_table
> >>>>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
> >>>>   *	flexibly clamped to min/max range in case the user provided
> >>>>   *	an incorrect value.
> >>>> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
> >>>> + * 	had been issued for that entry.
> >>>>   *
> >>>>   * At most 16 different flags will be allowed.
> >>>>   */
> >>>>  enum ctl_table_flags {
> >>>>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
> >>>> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
> >>>>  };
> >>> Ugh, no. Now I see why you had to set this flag later.
> >>>
> >>> You are not using this flag to "warn" but rather for an internal
> >>> status checker if you have warned or not. Internal flags should
> >>> not be something the user sets. If we want a flag for warning
> >>> that's one thing. If we need a flag to keep tabs if we have
> >>> warned or not that needs to be kept separately and internally,
> >>> nothing the user has to do set or reset.
> >>>
> >>>   Luis
> >> What I want to do is a printk_once for each sysctl parameter. So the
> >> flag is used as a marker that a warning has been printed.
> >>
> >> I do understand that it gets somewhat ugly in the case of msgmni and
> >> shmmni because of the copying back of the flag. Another alternative that
> >> had been suggested by Kees is to use prink_ratelimited. That we don't
> >> need that flag at all.
> > However it is done, a user flag should not be used also for internal
> > flag settings. That's just gross. Internal state machine stuff should
> > remain far from what the user is able to modify.
> >
> > Also, why can't it just use pr_warn_once() and be done with it?
> 
> Different sysctl parameters can use the same minmax proc_handler. Using
> pr_warn_once() means mistake in one will prevent mistakes in other
> parameters from showing up.

OK then use a separate internal set of flags for internal book keeping.
Its nothing the user should ever have to set.

  Luis

[PATCH v3 5/6] ipc: Clamp msgmni and shmmni to the real IPCMNI limit

[Waiman Long]

A user can write arbitrary integer values to msgmni and shmmni sysctl
parameters without getting error, but the actual limit is really
IPCMNI (32k). This can mislead users as they think they can get a
value that is not real.

Enforcing the limit by failing the sysctl parameter write, however,
can break existing user applications. Instead, the range clamping flag
is set to enforce the limit without failing existing user code. Users
can easily figure out if the sysctl parameter value is out of range
by either reading back the parameter value or checking the kernel
ring buffer for warning.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 ipc/ipc_sysctl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 8ad93c2..8eb7268 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -41,12 +41,21 @@ static int proc_ipc_dointvec(struct ctl_table *table, int write,
 static int proc_ipc_dointvec_minmax(struct ctl_table *table, int write,
 	void __user *buffer, size_t *lenp, loff_t *ppos)
 {
+	int ret;
 	struct ctl_table ipc_table;
 
 	memcpy(&ipc_table, table, sizeof(ipc_table));
 	ipc_table.data = get_ipc(table);
 
-	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
+	ret = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
+
+	/*
+	 * Copy back the CTL_FLAGS_OOR_WARNED flag which may be set in
+	 * the temporary ctl_table entry.
+	 */
+	table->flags |= (ipc_table.flags & CTL_FLAGS_OOR_WARNED);
+
+	return ret;
 }
 
 static int proc_ipc_dointvec_minmax_orphans(struct ctl_table *table, int write,
@@ -99,6 +108,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int zero;
 static int one = 1;
 static int int_max = INT_MAX;
+static int ipc_mni = IPCMNI;
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -120,7 +130,10 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 		.data		= &init_ipc_ns.shm_ctlmni,
 		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &ipc_mni,
+		.flags		= CTL_FLAGS_CLAMP_RANGE,
 	},
 	{
 		.procname	= "shm_rmid_forced",
@@ -147,7 +160,8 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 		.mode		= 0644,
 		.proc_handler	= proc_ipc_dointvec_minmax,
 		.extra1		= &zero,
-		.extra2		= &int_max,
+		.extra2		= &ipc_mni,
+		.flags		= CTL_FLAGS_CLAMP_RANGE,
 	},
 	{
 		.procname	= "auto_msgmni",
-- 
1.8.3.1

Re: [PATCH v3 5/6] ipc: Clamp msgmni and shmmni to the real IPCMNI limit

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:39PM -0500, Waiman Long wrote:
> A user can write arbitrary integer values to msgmni and shmmni sysctl
> parameters without getting error, but the actual limit is really
> IPCMNI (32k). This can mislead users as they think they can get a
> value that is not real.
> 
> Enforcing the limit by failing the sysctl parameter write, however,
> can break existing user applications. Instead, the range clamping flag
> is set to enforce the limit without failing existing user code. Users
> can easily figure out if the sysctl parameter value is out of range
> by either reading back the parameter value or checking the kernel
> ring buffer for warning.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  ipc/ipc_sysctl.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> index 8ad93c2..8eb7268 100644
> --- a/ipc/ipc_sysctl.c
> +++ b/ipc/ipc_sysctl.c
> @@ -41,12 +41,21 @@ static int proc_ipc_dointvec(struct ctl_table *table, int write,
>  static int proc_ipc_dointvec_minmax(struct ctl_table *table, int write,
>  	void __user *buffer, size_t *lenp, loff_t *ppos)
>  {
> +	int ret;
>  	struct ctl_table ipc_table;
>  
>  	memcpy(&ipc_table, table, sizeof(ipc_table));
>  	ipc_table.data = get_ipc(table);
>  
> -	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
> +	ret = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
> +
> +	/*
> +	 * Copy back the CTL_FLAGS_OOR_WARNED flag which may be set in
> +	 * the temporary ctl_table entry.
> +	 */
> +	table->flags |= (ipc_table.flags & CTL_FLAGS_OOR_WARNED);

Again, why is this needed? Cant' we do this for the developer somehow?
Seems fragile, and if we can do it why not?

  Luis

[PATCH v3 6/6] ipc: Clamp semmni to the real IPCMNI limit

[Waiman Long]

This patch clamps the semmni value (fourth element of sem_ctls[]
array) to within the [0, IPCMNI] range and prints a warning message
once when an out-of-range value is being written.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 ipc/ipc_sysctl.c | 13 ++++++++++++-
 ipc/sem.c        | 28 ++++++++++++++++++++++++++++
 ipc/util.h       |  4 ++++
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 8eb7268..2c03f57 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -97,12 +97,22 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
 }
 
+static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
+	void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	int ret = proc_ipc_dointvec(table, write, buffer, lenp, ppos);
+
+	sem_check_semmni(table, current->nsproxy->ipc_ns);
+	return ret;
+}
+
 #else
 #define proc_ipc_doulongvec_minmax NULL
 #define proc_ipc_dointvec	   NULL
 #define proc_ipc_dointvec_minmax   NULL
 #define proc_ipc_dointvec_minmax_orphans   NULL
 #define proc_ipc_auto_msgmni	   NULL
+#define proc_ipc_sem_dointvec	   NULL
 #endif
 
 static int zero;
@@ -186,7 +196,8 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 		.data		= &init_ipc_ns.sem_ctls,
 		.maxlen		= 4*sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_sem_dointvec,
+		.flags		= CTL_FLAGS_CLAMP_RANGE,
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
diff --git a/ipc/sem.c b/ipc/sem.c
index a4af049..739dfca 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -2337,3 +2337,31 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
 	return 0;
 }
 #endif
+
+#ifdef CONFIG_PROC_SYSCTL
+/*
+ * Check to see if semmni is out of range and clamp it if necessary.
+ */
+void sem_check_semmni(struct ctl_table *table, struct ipc_namespace *ns)
+{
+	bool clamped = false;
+
+	if (!(table->flags & CTL_FLAGS_CLAMP_RANGE))
+		return;
+
+	/*
+	 * Clamp semmni to the range [0, IPCMNI].
+	 */
+	if (ns->sc_semmni < 0) {
+		ns->sc_semmni = 0;
+		clamped = true;
+	}
+	if (ns->sc_semmni > IPCMNI) {
+		ns->sc_semmni = IPCMNI;
+		clamped = true;
+	}
+	if (clamped)
+		pr_warn_once("Kernel parameter \"sem[3]\" was set out of range [%d, %d], clamped to %d.\n",
+			     0, IPCMNI, ns->sc_semmni);
+}
+#endif
diff --git a/ipc/util.h b/ipc/util.h
index 89b8ec1..af57394 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -206,6 +206,10 @@ int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
 void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 		void (*free)(struct ipc_namespace *, struct kern_ipc_perm *));
 
+#ifdef CONFIG_PROC_SYSCTL
+extern void sem_check_semmni(struct ctl_table *table, struct ipc_namespace *ns);
+#endif
+
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 struct compat_ipc_perm {
-- 
1.8.3.1

Re: [PATCH v3 6/6] ipc: Clamp semmni to the real IPCMNI limit

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:40PM -0500, Waiman Long wrote:
> This patch clamps the semmni value (fourth element of sem_ctls[]
> array) to within the [0, IPCMNI] range and prints a warning message
> once when an out-of-range value is being written.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  ipc/ipc_sysctl.c | 13 ++++++++++++-
>  ipc/sem.c        | 28 ++++++++++++++++++++++++++++
>  ipc/util.h       |  4 ++++
>  3 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> index 8eb7268..2c03f57 100644
> --- a/ipc/ipc_sysctl.c
> +++ b/ipc/ipc_sysctl.c
> @@ -97,12 +97,22 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>  	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
>  }
>  
> +static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
> +	void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int ret = proc_ipc_dointvec(table, write, buffer, lenp, ppos);
> +
> +	sem_check_semmni(table, current->nsproxy->ipc_ns);
> +	return ret;
> +}
> +
>  #else
>  #define proc_ipc_doulongvec_minmax NULL
>  #define proc_ipc_dointvec	   NULL
>  #define proc_ipc_dointvec_minmax   NULL
>  #define proc_ipc_dointvec_minmax_orphans   NULL
>  #define proc_ipc_auto_msgmni	   NULL
> +#define proc_ipc_sem_dointvec	   NULL
>  #endif
>  
>  static int zero;
> @@ -186,7 +196,8 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>  		.data		= &init_ipc_ns.sem_ctls,
>  		.maxlen		= 4*sizeof(int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_ipc_dointvec,
> +		.proc_handler	= proc_ipc_sem_dointvec,
> +		.flags		= CTL_FLAGS_CLAMP_RANGE,
>  	},
>  #ifdef CONFIG_CHECKPOINT_RESTORE
>  	{
> diff --git a/ipc/sem.c b/ipc/sem.c
> index a4af049..739dfca 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -2337,3 +2337,31 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
>  	return 0;
>  }
>  #endif
> +
> +#ifdef CONFIG_PROC_SYSCTL
> +/*
> + * Check to see if semmni is out of range and clamp it if necessary.
> + */
> +void sem_check_semmni(struct ctl_table *table, struct ipc_namespace *ns)
> +{
> +	bool clamped = false;
> +
> +	if (!(table->flags & CTL_FLAGS_CLAMP_RANGE))
> +		return;
> +
> +	/*
> +	 * Clamp semmni to the range [0, IPCMNI].
> +	 */
> +	if (ns->sc_semmni < 0) {
> +		ns->sc_semmni = 0;
> +		clamped = true;
> +	}
> +	if (ns->sc_semmni > IPCMNI) {
> +		ns->sc_semmni = IPCMNI;
> +		clamped = true;
> +	}
> +	if (clamped)
> +		pr_warn_once("Kernel parameter \"sem[3]\" was set out of range [%d, %d], clamped to %d.\n",
> +			     0, IPCMNI, ns->sc_semmni);

Why are the users issuing a warning, wouldn't the API do the warning
on its own, specially since we're adding a warn flag?

  Luis

Re: [PATCH v3 6/6] ipc: Clamp semmni to the real IPCMNI limit

[Waiman Long]

On 03/08/2018 01:15 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 12:43:40PM -0500, Waiman Long wrote:
>> This patch clamps the semmni value (fourth element of sem_ctls[]
>> array) to within the [0, IPCMNI] range and prints a warning message
>> once when an out-of-range value is being written.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  ipc/ipc_sysctl.c | 13 ++++++++++++-
>>  ipc/sem.c        | 28 ++++++++++++++++++++++++++++
>>  ipc/util.h       |  4 ++++
>>  3 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
>> index 8eb7268..2c03f57 100644
>> --- a/ipc/ipc_sysctl.c
>> +++ b/ipc/ipc_sysctl.c
>> @@ -97,12 +97,22 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>>  	return proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
>>  }
>>  
>> +static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
>> +	void __user *buffer, size_t *lenp, loff_t *ppos)
>> +{
>> +	int ret = proc_ipc_dointvec(table, write, buffer, lenp, ppos);
>> +
>> +	sem_check_semmni(table, current->nsproxy->ipc_ns);
>> +	return ret;
>> +}
>> +
>>  #else
>>  #define proc_ipc_doulongvec_minmax NULL
>>  #define proc_ipc_dointvec	   NULL
>>  #define proc_ipc_dointvec_minmax   NULL
>>  #define proc_ipc_dointvec_minmax_orphans   NULL
>>  #define proc_ipc_auto_msgmni	   NULL
>> +#define proc_ipc_sem_dointvec	   NULL
>>  #endif
>>  
>>  static int zero;
>> @@ -186,7 +196,8 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>>  		.data		= &init_ipc_ns.sem_ctls,
>>  		.maxlen		= 4*sizeof(int),
>>  		.mode		= 0644,
>> -		.proc_handler	= proc_ipc_dointvec,
>> +		.proc_handler	= proc_ipc_sem_dointvec,
>> +		.flags		= CTL_FLAGS_CLAMP_RANGE,
>>  	},
>>  #ifdef CONFIG_CHECKPOINT_RESTORE
>>  	{
>> diff --git a/ipc/sem.c b/ipc/sem.c
>> index a4af049..739dfca 100644
>> --- a/ipc/sem.c
>> +++ b/ipc/sem.c
>> @@ -2337,3 +2337,31 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it)
>>  	return 0;
>>  }
>>  #endif
>> +
>> +#ifdef CONFIG_PROC_SYSCTL
>> +/*
>> + * Check to see if semmni is out of range and clamp it if necessary.
>> + */
>> +void sem_check_semmni(struct ctl_table *table, struct ipc_namespace *ns)
>> +{
>> +	bool clamped = false;
>> +
>> +	if (!(table->flags & CTL_FLAGS_CLAMP_RANGE))
>> +		return;
>> +
>> +	/*
>> +	 * Clamp semmni to the range [0, IPCMNI].
>> +	 */
>> +	if (ns->sc_semmni < 0) {
>> +		ns->sc_semmni = 0;
>> +		clamped = true;
>> +	}
>> +	if (ns->sc_semmni > IPCMNI) {
>> +		ns->sc_semmni = IPCMNI;
>> +		clamped = true;
>> +	}
>> +	if (clamped)
>> +		pr_warn_once("Kernel parameter \"sem[3]\" was set out of range [%d, %d], clamped to %d.\n",
>> +			     0, IPCMNI, ns->sc_semmni);
> Why are the users issuing a warning, wouldn't the API do the warning
> on its own, specially since we're adding a warn flag?
>
>   Luis

This is a special case as the semmni parameter is actually embedded in
the sem sysctl paramter as the last argument of the 4. So the previous
sysctl patches won't have any effect on this one. I did this patch to
make all the IPC *mni parameters have the same behavior for consistency
purpose.

Cheers,
Longman

Re: [PATCH v3 0/6] ipc: Clamp *mni to the real IPCMNI limit

[Luis R. Rodriguez]

On Thu, Mar 01, 2018 at 12:43:34PM -0500, Waiman Long wrote:
> v2->v3:
>  - Fix kdoc comment errors.
>  - Incorporate comments and suggestions from Luis R. Rodriguez.
>  - Add a patch to fix a typo error in fs/proc/proc_sysctl.c.
> 
> v1->v2:
>  - Add kdoc comments to the do_proc_do{u}intvec_minmax_conv_param
>    structures.
>  - Add a new flags field to the ctl_table structure for specifying
>    whether range clamping should be activated instead of adding new
>    sysctl parameter handlers.
>  - Clamp the semmni value embedded in the multi-values sem parameter.
> 
> v1 patch: https://lkml.org/lkml/2018/2/19/453
> v2 patch: https://lkml.org/lkml/2018/2/27/627
> 
> The sysctl parameters msgmni, shmmni and semmni have an inherent limit
> of IPC_MNI (32k). However, users may not be aware of that because they
> can write a value much higher than that without getting any error or
> notification. Reading the parameters back will show the newly written
> values which are not real.
> 
> Enforcing the limit by failing sysctl parameter write, however, can
> break existing user applications. To address this delemma, a new flags
> field is introduced into the ctl_table. The value CTL_FLAGS_CLAMP_RANGE
> can be added to any ctl_table entries to enable a looser range clamping
> without returning any error. For example,
> 
>   .flags = CTL_FLAGS_CLAMP_RANGE,
> 
> This flags value are now used for the range checking of shmmni,
> msgmni and semmni without breaking existing applications. If any out
> of range value is written to those sysctl parameters, the following
> warning will be printed instead.
> 
>   Kernel parameter "shmmni" was set out of range [0, 32768], clamped to 32768.
> 
> Reading the values back will show 32768 instead of some fake values.

I don't see any addition of respective tests cases, I thought I asked
for this. Please add respective tests cases for all the API you are
adding on lib/test_sysctl.c and respective tests on
tools/testing/selftests/sysctl/sysctl.sh

  Luis

Re: [PATCH v3 0/6] ipc: Clamp *mni to the real IPCMNI limit

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 06:23:35PM +0000, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 12:43:34PM -0500, Waiman Long wrote:
> > v2->v3:
> >  - Fix kdoc comment errors.
> >  - Incorporate comments and suggestions from Luis R. Rodriguez.
> >  - Add a patch to fix a typo error in fs/proc/proc_sysctl.c.
> > 
> > v1->v2:
> >  - Add kdoc comments to the do_proc_do{u}intvec_minmax_conv_param
> >    structures.
> >  - Add a new flags field to the ctl_table structure for specifying
> >    whether range clamping should be activated instead of adding new
> >    sysctl parameter handlers.
> >  - Clamp the semmni value embedded in the multi-values sem parameter.
> > 
> > v1 patch: https://lkml.org/lkml/2018/2/19/453
> > v2 patch: https://lkml.org/lkml/2018/2/27/627
> > 
> > The sysctl parameters msgmni, shmmni and semmni have an inherent limit
> > of IPC_MNI (32k). However, users may not be aware of that because they
> > can write a value much higher than that without getting any error or
> > notification. Reading the parameters back will show the newly written
> > values which are not real.
> > 
> > Enforcing the limit by failing sysctl parameter write, however, can
> > break existing user applications. To address this delemma, a new flags
> > field is introduced into the ctl_table. The value CTL_FLAGS_CLAMP_RANGE
> > can be added to any ctl_table entries to enable a looser range clamping
> > without returning any error. For example,
> > 
> >   .flags = CTL_FLAGS_CLAMP_RANGE,
> > 
> > This flags value are now used for the range checking of shmmni,
> > msgmni and semmni without breaking existing applications. If any out
> > of range value is written to those sysctl parameters, the following
> > warning will be printed instead.
> > 
> >   Kernel parameter "shmmni" was set out of range [0, 32768], clamped to 32768.
> > 
> > Reading the values back will show 32768 instead of some fake values.
> 
> I don't see any addition of respective tests cases, I thought I asked
> for this. Please add respective tests cases for all the API you are
> adding on lib/test_sysctl.c and respective tests on
> tools/testing/selftests/sysctl/sysctl.sh

Andrew,

If its not too much trouble please only apply the first two patches of this
series. The rest should be dropped. I think it would be a mistake for us to
take start carrying the rest at this point in time.

  Luis

Re: [PATCH v3 0/6] ipc: Clamp *mni to the real IPCMNI limit

[Waiman Long]

On 03/08/2018 01:38 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 08, 2018 at 06:23:35PM +0000, Luis R. Rodriguez wrote:
>> On Thu, Mar 01, 2018 at 12:43:34PM -0500, Waiman Long wrote:
>>> v2->v3:
>>>  - Fix kdoc comment errors.
>>>  - Incorporate comments and suggestions from Luis R. Rodriguez.
>>>  - Add a patch to fix a typo error in fs/proc/proc_sysctl.c.
>>>
>>> v1->v2:
>>>  - Add kdoc comments to the do_proc_do{u}intvec_minmax_conv_param
>>>    structures.
>>>  - Add a new flags field to the ctl_table structure for specifying
>>>    whether range clamping should be activated instead of adding new
>>>    sysctl parameter handlers.
>>>  - Clamp the semmni value embedded in the multi-values sem parameter.
>>>
>>> v1 patch: https://lkml.org/lkml/2018/2/19/453
>>> v2 patch: https://lkml.org/lkml/2018/2/27/627
>>>
>>> The sysctl parameters msgmni, shmmni and semmni have an inherent limit
>>> of IPC_MNI (32k). However, users may not be aware of that because they
>>> can write a value much higher than that without getting any error or
>>> notification. Reading the parameters back will show the newly written
>>> values which are not real.
>>>
>>> Enforcing the limit by failing sysctl parameter write, however, can
>>> break existing user applications. To address this delemma, a new flags
>>> field is introduced into the ctl_table. The value CTL_FLAGS_CLAMP_RANGE
>>> can be added to any ctl_table entries to enable a looser range clamping
>>> without returning any error. For example,
>>>
>>>   .flags = CTL_FLAGS_CLAMP_RANGE,
>>>
>>> This flags value are now used for the range checking of shmmni,
>>> msgmni and semmni without breaking existing applications. If any out
>>> of range value is written to those sysctl parameters, the following
>>> warning will be printed instead.
>>>
>>>   Kernel parameter "shmmni" was set out of range [0, 32768], clamped to 32768.
>>>
>>> Reading the values back will show 32768 instead of some fake values.
>> I don't see any addition of respective tests cases, I thought I asked
>> for this. Please add respective tests cases for all the API you are
>> adding on lib/test_sysctl.c and respective tests on
>> tools/testing/selftests/sysctl/sysctl.sh
> Andrew,
>
> If its not too much trouble please only apply the first two patches of this
> series. The rest should be dropped. I think it would be a mistake for us to
> take start carrying the rest at this point in time.
>
>   Luis

So I am going to assume that the first 2 will be in and drop them in my
v4 patch.

Cheers,
Longman

Re: [PATCH v3 0/6] ipc: Clamp *mni to the real IPCMNI limit

[Waiman Long]

On 03/08/2018 01:23 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 01, 2018 at 12:43:34PM -0500, Waiman Long wrote:
>> v2->v3:
>>  - Fix kdoc comment errors.
>>  - Incorporate comments and suggestions from Luis R. Rodriguez.
>>  - Add a patch to fix a typo error in fs/proc/proc_sysctl.c.
>>
>> v1->v2:
>>  - Add kdoc comments to the do_proc_do{u}intvec_minmax_conv_param
>>    structures.
>>  - Add a new flags field to the ctl_table structure for specifying
>>    whether range clamping should be activated instead of adding new
>>    sysctl parameter handlers.
>>  - Clamp the semmni value embedded in the multi-values sem parameter.
>>
>> v1 patch: https://lkml.org/lkml/2018/2/19/453
>> v2 patch: https://lkml.org/lkml/2018/2/27/627
>>
>> The sysctl parameters msgmni, shmmni and semmni have an inherent limit
>> of IPC_MNI (32k). However, users may not be aware of that because they
>> can write a value much higher than that without getting any error or
>> notification. Reading the parameters back will show the newly written
>> values which are not real.
>>
>> Enforcing the limit by failing sysctl parameter write, however, can
>> break existing user applications. To address this delemma, a new flags
>> field is introduced into the ctl_table. The value CTL_FLAGS_CLAMP_RANGE
>> can be added to any ctl_table entries to enable a looser range clamping
>> without returning any error. For example,
>>
>>   .flags = CTL_FLAGS_CLAMP_RANGE,
>>
>> This flags value are now used for the range checking of shmmni,
>> msgmni and semmni without breaking existing applications. If any out
>> of range value is written to those sysctl parameters, the following
>> warning will be printed instead.
>>
>>   Kernel parameter "shmmni" was set out of range [0, 32768], clamped to 32768.
>>
>> Reading the values back will show 32768 instead of some fake values.
> I don't see any addition of respective tests cases, I thought I asked
> for this. Please add respective tests cases for all the API you are
> adding on lib/test_sysctl.c and respective tests on
> tools/testing/selftests/sysctl/sysctl.sh
>
>   Luis

I probably missed that. I will add a test case in the next version.

-Longman