ipc: Clamp *mni to the real IPCMNI limit

Signed-off-by Waiman Long <[email protected]>

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 <[email protected]>

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.

...

include/linux/sysctl.h +0 -0

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

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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...

include/linux/sysctl.h renamed from include/linux/sysctl.h~sysctl-add-flags-to-support-min-max-range-clamping-fix +1 -1

@@ -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;

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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...

Quoted 35 lines · Andrew Morton

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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.

Quoted 2 lines

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),

(quoted diff) +1 -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.

Quoted 52 lines · Luis R. Rodriguez

Quoted 34 lines · Andrew Morton

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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.

Quoted 2 lines

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),

(quoted diff) +1 -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:

Quoted 8 lines

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

Quoted 77 lines · Luis R. Rodriguez

Quoted 52 lines · Luis R. Rodriguez

Quoted 34 lines · Andrew Morton

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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.

Quoted 2 lines

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),

(quoted diff) +1 -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:

Quoted 8 lines

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.

Quoted 1 line

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?

Quoted 79 lines · Waiman Long

Quoted 78 lines · Luis R. Rodriguez

Quoted 52 lines · Luis R. Rodriguez

Quoted 34 lines · Andrew Morton

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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.

Quoted 2 lines

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),

(quoted diff) +1 -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:

Quoted 8 lines

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

Quoted 4 lines

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

Quoted 34 lines · Andrew Morton

Quoted 25 lines · On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <[email protected]> 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.

...

include/linux/sysctl.h struct ctl_table · line 119

Quoted 4 lines

include/linux/sysctl.h +1 -0

@@ -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.

Quoted 2 lines

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),

(quoted diff) +1 -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;
}

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.

...

Quoted 9 lines · On Thu, 1 Mar 2018 12:43:38 -0500 Waiman Long <[email protected]> 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.

...

kernel/sysctl.c struct do_proc_dointvec_minmax_conv_param { · line 2561

Quoted 7 lines

kernel/sysctl.c +7 -0

+		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.

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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +3 -0

index 448aa72..3db57af 100644
@@ -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 {

kernel/sysctl.c +1 -0

index af351ed..6c68e77 100644
@@ -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;

Quoted 46 lines · 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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +3 -0

index 448aa72..3db57af 100644
@@ -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 {

kernel/sysctl.c +1 -0

index af351ed..6c68e77 100644
@@ -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;

kernel/sysctl.c struct do_proc_dointvec_minmax_conv_param { · line 2525

Quoted 10 lines

kernel/sysctl.c +6 -0

 	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

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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +0 -0

index 448aa72..3db57af 100644
@@ -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

Quoted 20 lines · 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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +0 -0

index 448aa72..3db57af 100644
@@ -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

include/linux/sysctl.h struct ctl_table · line 141

Quoted 10 lines

include/linux/sysctl.h +3 -0

  *	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

Quoted 33 lines · Luis R. Rodriguez

Quoted 20 lines · 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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +0 -0

index 448aa72..3db57af 100644
@@ -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

include/linux/sysctl.h struct ctl_table · line 141

Quoted 10 lines

include/linux/sysctl.h +3 -0

  *	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.

Quoted 8 lines

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.

Quoted 41 lines · Waiman Long

Quoted 40 lines · Luis R. Rodriguez

Quoted 20 lines · 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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +0 -0

index 448aa72..3db57af 100644
@@ -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

include/linux/sysctl.h struct ctl_table · line 141

Quoted 10 lines

include/linux/sysctl.h +3 -0

  *	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.

Quoted 8 lines

Quoted 1 line

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?

Quoted 54 lines · Luis R. Rodriguez

Quoted 41 lines · Waiman Long

Quoted 40 lines · Luis R. Rodriguez

Quoted 20 lines · 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 <[email protected]>

---
 include/linux/sysctl.h |  3 +++
 kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 8 deletions(-)

include/linux/sysctl.h +0 -0

index 448aa72..3db57af 100644
@@ -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

include/linux/sysctl.h struct ctl_table · line 141

Quoted 10 lines

include/linux/sysctl.h +3 -0

  *	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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 1 line

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.

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 <[email protected]>

---
 ipc/ipc_sysctl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

ipc/ipc_sysctl.c +1 -0

index 8ad93c2..8eb7268 100644
@@ -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));

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 <[email protected]>

---
 ipc/ipc_sysctl.c | 13 ++++++++++++-
 ipc/sem.c        | 28 ++++++++++++++++++++++++++++
 ipc/util.h       |  4 ++++
 3 files changed, 44 insertions(+), 1 deletion(-)

ipc/ipc_sysctl.c +12 -1

index 8eb7268..2c03f57 100644
@@ -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
 	{

ipc/sem.c +16 -0

index a4af049..739dfca 100644
@@ -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) {

Quoted 72 lines · 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 <[email protected]>

---
 ipc/ipc_sysctl.c | 13 ++++++++++++-
 ipc/sem.c        | 28 ++++++++++++++++++++++++++++
 ipc/util.h       |  4 ++++
 3 files changed, 44 insertions(+), 1 deletion(-)

ipc/ipc_sysctl.c +12 -1

index 8eb7268..2c03f57 100644
@@ -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
 	{

ipc/sem.c +16 -0

index a4af049..739dfca 100644
@@ -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) {

ipc/sem.c static int sysvipc_sem_proc_show(struct seq_file *s, void *it) · line 2365

Quoted 10 lines

ipc/sem.c +10 -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

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.

Quoted 35 lines · 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.

Quoted 3 lines

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

Quoted 36 lines · Luis R. Rodriguez

Quoted 35 lines · 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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 37 lines · 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.

Quoted 1 line

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