ipc: Clamp *mni to the real IPCMNI limit

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.

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 if either the proc_dointvec_minmax() or the proc_douintvec_minmax() handlers is used.

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

---
 include/linux/sysctl.h | 15 +++++++++++++++
 kernel/sysctl.c        | 48 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 54 insertions(+), 9 deletions(-)

include/linux/sysctl.h +1 -0

index b769ecf..963e363 100644
@@ -116,6 +116,7 @@ struct ctl_table
 	void *data;
 	int maxlen;
 	umode_t mode;
+	unsigned int flags;
 	struct ctl_table *child;	/* Deprecated */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	struct ctl_table_poll *poll;

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

Quoted 7 lines

To provide this less restrictive form of range checking, a new flags field is added to the ctl_table 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 if either the proc_dointvec_minmax() or the proc_douintvec_minmax() handlers is used.

You keep missing to document both on commit log and kdoc which end on the range is selected if you happen to go over. To be clear it is unclear from reading the commit log if a default is set if you go over if you pick another value. In this case it is conditional if you go over we pick the high range max, and if you go below the lower range minimum set allowed.

What happens if no low range is set and that is what the issue in terms of range triggers?

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.

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 if either the proc_dointvec_minmax() or the proc_douintvec_minmax() handlers is used.

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

---
 include/linux/sysctl.h | 15 +++++++++++++++
 kernel/sysctl.c        | 48 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 54 insertions(+), 9 deletions(-)

include/linux/sysctl.h +15 -0

index b769ecf..963e363 100644
@@ -116,6 +116,7 @@ struct ctl_table
 	void *data;
 	int maxlen;
 	umode_t mode;
+	unsigned int flags;
 	struct ctl_table *child;	/* Deprecated */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	struct ctl_table_poll *poll;

@@ -123,6 +124,20 @@ struct ctl_table
 	void *extra2;
 } __randomize_layout;
 
+/**
+ * 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
+ *	an incorrect value.
+ */
+enum ctl_table_flags {
+	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
+	__CTL_FLAGS_MAX			= BIT(1),
+};
+
+#define CTL_TABLE_FLAGS_ALL	(__CTL_FLAGS_MAX - 1)
+
 struct ctl_node {
 	struct rb_node node;
 	struct ctl_table_header *header;

kernel/sysctl.c +39 -9

index d2aa6b4..3d65f41 100644
@@ -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;
+	unsigned int *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;
+	unsigned int *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);

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

Quoted 5 lines

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.

What use cases? Who will break. Examples would be good.

Quoted 7 lines

To provide this less restrictive form of range checking, a new flags field is added to the ctl_table 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 if either the proc_dointvec_minmax() or the proc_douintvec_minmax() handlers is used.

As this is constructed it will increase the size of ctl_table for everyone. Better would be to add a new function that behaves similary but differently than to burden struct ctl_table for the rest of time.

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

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

---
 fs/proc/proc_sysctl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

fs/proc/proc_sysctl.c +0 -0

index 493c975..67c0c82 100644

Quoted 2 lines · Waiman Long

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

This should be merged with the first patch otherwise there are atomic points in time on the commit log history where invalid values are allowed and that makes no sense.

This can probably be expanded to verify semantics further. Details below.

Quoted 10 lines

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

---
 fs/proc/proc_sysctl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

fs/proc/proc_sysctl.c +0 -0

index 493c975..67c0c82 100644

fs/proc/proc_sysctl.c static int sysctl_check_table_array(const char *path, struct ctl_table *table) · line 1100

Quoted 9 lines

fs/proc/proc_sysctl.c +6 -0

@@ -1092,6 +1092,16 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 	return err;
 }
 
+static int sysctl_check_flags(const char *path, struct ctl_table *table)
+{
+	int err = 0;
+
+	if (table->flags & ~CTL_TABLE_FLAGS_ALL)
+		err = sysctl_err(path, table, "invalid flags");

What if a range for the upper limit is set but not the lower limit and the user goes over the lower limit?

How about the inverse?

Do we need both ranges set?

Luis

Quoted 35 lines · Luis R. Rodriguez

Quoted 2 lines · Waiman Long

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

This should be merged with the first patch otherwise there are atomic points in time on the commit log history where invalid values are allowed and that makes no sense.

This can probably be expanded to verify semantics further. Details below.

Quoted 10 lines

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

---
 fs/proc/proc_sysctl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

fs/proc/proc_sysctl.c +0 -0

index 493c975..67c0c82 100644

fs/proc/proc_sysctl.c static int sysctl_check_table_array(const char *path, struct ctl_table *table) · line 1100

Quoted 9 lines

fs/proc/proc_sysctl.c +6 -0

@@ -1092,6 +1092,16 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 	return err;
 }
 
+static int sysctl_check_flags(const char *path, struct ctl_table *table)
+{
+	int err = 0;
+
+	if (table->flags & ~CTL_TABLE_FLAGS_ALL)
+		err = sysctl_err(path, table, "invalid flags");

What if a range for the upper limit is set but not the lower limit and the user goes over the lower limit?

How about the inverse?

Do we need both ranges set?

Quoted 1 line

Luis

This check is just to make sure that no invalid flag bit is set. Range clamping is just one of flag bits, though this is the only one currently supported. In fact, it is allowed that the minimum or maximum can be left unspecified. In this case, no minimum or maximum checking will be done. So I don't see anything related to range checking should be put here.

Quoted 37 lines · Waiman Long

Quoted 36 lines · Luis R. Rodriguez

Quoted 2 lines · Waiman Long

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

This should be merged with the first patch otherwise there are atomic points in time on the commit log history where invalid values are allowed and that makes no sense.

This can probably be expanded to verify semantics further. Details below.

Quoted 10 lines

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

---
 fs/proc/proc_sysctl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

fs/proc/proc_sysctl.c +0 -0

index 493c975..67c0c82 100644

fs/proc/proc_sysctl.c static int sysctl_check_table_array(const char *path, struct ctl_table *table) · line 1100

Quoted 9 lines

fs/proc/proc_sysctl.c +6 -0

@@ -1092,6 +1092,16 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table)
 	return err;
 }
 
+static int sysctl_check_flags(const char *path, struct ctl_table *table)
+{
+	int err = 0;
+
+	if (table->flags & ~CTL_TABLE_FLAGS_ALL)
+		err = sysctl_err(path, table, "invalid flags");

What if a range for the upper limit is set but not the lower limit and the user goes over the lower limit?

How about the inverse?

Do we need both ranges set?

Luis

Quoted 5 lines

This check is just to make sure that no invalid flag bit is set. Range clamping is just one of flag bits, though this is the only one currently supported. In fact, it is allowed that the minimum or maximum can be left unspecified. In this case, no minimum or maximum checking will be done. So I don't see anything related to range checking should be put here.

What if minimum is greater than maximum?

Luis

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

Quoted 2 lines · On Mon, 12 Mar 2018 16:15:40 -0400 Waiman Long <[email protected]> wrote:

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

Why? What's wrong with the old code, what value does this change add, etc.

Quoted 6 lines · Andrew Morton

Quoted 2 lines · On Mon, 12 Mar 2018 16:15:40 -0400 Waiman Long <[email protected]> wrote:

Checking code is added to check for invalid flags in the ctl_table and return error if an unknown flag is used.

Why? What's wrong with the old code, what value does this change add, etc.

This is an additional checking code requested by 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 into the kernel ring buffer when a clamped sysctl parameter receives an out of range value.

The pr_warn_ratelimited() macro is used to limit the number of warning messages that can be printed within a given period of time.

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

---
 kernel/sysctl.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

kernel/sysctl.c +1 -0

index 3d65f41..14aca92 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

kernel/sysctl.c +9 -4

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

Quoted 28 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 into the kernel ring buffer when a clamped sysctl parameter receives an out of range value.

The pr_warn_ratelimited() macro is used to limit the number of warning messages that can be printed within a given period of time.

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

---
 kernel/sysctl.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 8 deletions(-)

kernel/sysctl.c +1 -0

index 3d65f41..14aca92 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

kernel/sysctl.c struct do_proc_dointvec_minmax_conv_param { · line 2524

Quoted 10 lines

kernel/sysctl.c +5 -0

@@ -2514,31 +2515,48 @@ struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
 	unsigned int *flags;
+	const char *name;
 };
 
+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+#define pr_fmt(fmt)	"sysctl: " fmt

No. This needs to be defined at the top of the file, please look at other uses on kernel/*.c and just use:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

The filename already provides the name we want.

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 into the kernel ring buffer when a clamped sysctl parameter receives an out of range value.

The pr_warn_ratelimited() macro is used to limit the number of warning messages that can be printed within a given period of time.

...

Quoted 10 lines · On Mon, 12 Mar 2018 16:15:41 -0400 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 into the kernel ring buffer when a clamped sysctl parameter receives an out of range value.

The pr_warn_ratelimited() macro is used to limit the number of warning messages that can be printed within a given period of time.

kernel/sysctl.c struct do_proc_dointvec_minmax_conv_param { · line 2524

Quoted 6 lines

...

kernel/sysctl.c +4 -0

+#ifdef pr_fmt
+#undef pr_fmt
+#endif
+#define pr_fmt(fmt)	"sysctl: " fmt

Why is it necessary to undefine pr_fmt? That's a somewhat unusual thing to do.

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 | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

ipc/ipc_sysctl.c +7 -2

index 8ad93c2..1955dd4 100644
@@ -99,6 +99,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 +121,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 +151,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",

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

Which applications examples please.

I am seeing this patchset late but it looks like a whole lot of changes to avoid a theoretical possibility.

Changes that have an impact on more than just the ipc code you are patching.

That makes me feel very uncomfortable with these changes.

Eric

Quoted 14 lines · Eric W. Biederman

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

Which applications examples please.

I am seeing this patchset late but it looks like a whole lot of changes to avoid a theoretical possibility.

Quoted 6 lines

Changes that have an impact on more than just the ipc code you are patching.

That makes me feel very uncomfortable with these changes.

Eric

This patchset is constructed to address a customer request that there is no easy way to find out the actual usable range of a sysctl parameter. In this particular case, the customer wants to use more than 32k shared memory segments. They can put in a large value into shmmni, but the application didn't work properly because shmmni was internally clamped to 32k without any visible sign that a smaller limit has been imposed.

Out of a concern that there might be customers out there setting those sysctl parameters outside of the allowable range without knowing it, just enforcing the right limits may have the undesirable consequence of breaking their existing setup scripts. I don't have concrete example of what customers are doing that, but it won't look good if we wait until the complaints come in.

The new code won't affect existing code unless the necessary flag is set. So would you mind elaborating what other impact do you see that will affect other non-IPC code in an undesirable way?

Quoted 35 lines · Waiman Long

Quoted 14 lines · Eric W. Biederman

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

Which applications examples please.

I am seeing this patchset late but it looks like a whole lot of changes to avoid a theoretical possibility.

Quoted 6 lines

Changes that have an impact on more than just the ipc code you are patching.

That makes me feel very uncomfortable with these changes.

Eric

This patchset is constructed to address a customer request that there is no easy way to find out the actual usable range of a sysctl parameter. In this particular case, the customer wants to use more than 32k shared memory segments. They can put in a large value into shmmni, but the application didn't work properly because shmmni was internally clamped to 32k without any visible sign that a smaller limit has been imposed.

Out of a concern that there might be customers out there setting those sysctl parameters outside of the allowable range without knowing it, just enforcing the right limits may have the undesirable consequence of breaking their existing setup scripts. I don't have concrete example of what customers are doing that, but it won't look good if we wait until the complaints come in.

Quoted 3 lines

The new code won't affect existing code unless the necessary flag is set. So would you mind elaborating what other impact do you see that will affect other non-IPC code in an undesirable way?

The increase in size of struct ctl_table. Every caller is affected. Plus it increases everyone's cognitive load to figure out what is this flags field as they fill out ctl_table.

Just introducing a proc_dointvec_minmax_clamped follows the existing pattern and it makes it easier for everyone who both read the code.

It strikes me as quite peculiar that the response to bug report where the complaint is an error is not given, is to continue the current behavior without giving an error.

Arguably the simplest fix here would be to kill IPCMNI entirely. Assign the shmids from a sequence counter. And place those structures in a rbtree indexed by shmni. There are 32bit fields but I don't think we must keep the low 16bits for an index into an array and the high 16bits as the actual sequence number.

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace. It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

This can be implemented trivially with the current code using idr_alloc_cyclic.

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid. That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly. New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

Waiman please take a look at this and run it through some tests etc, I am pretty certain something like this patch is all you need to do to sort out ipc assignment. Not messing with sysctls needed.

 include/linux/ipc.h           |  2 --
 include/linux/ipc_namespace.h |  1 -
 ipc/ipc_sysctl.c              |  6 ++--
 ipc/namespace.c               | 11 ++----
 ipc/util.c                    | 80 ++++++++++---------------------------------
 ipc/util.h                    | 11 +-----
 6 files changed, 25 insertions(+), 86 deletions(-)

include/linux/ipc.h +0 -2

index 821b2f260992..6cc2df7f7ac9 100644
@@ -8,8 +8,6 @@
 #include <uapi/linux/ipc.h>
 #include <linux/refcount.h>
 
-#define IPCMNI 32768  /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
-
 /* used by in-kernel data structures */
 struct kern_ipc_perm {
 	spinlock_t	lock;

include/linux/ipc_namespace.h +0 -1

index b5630c8eb2f3..cab33b6a8236 100644
@@ -15,7 +15,6 @@ struct user_namespace;
 
 struct ipc_ids {
 	int in_use;
-	unsigned short seq;
 	bool tables_initialized;
 	struct rw_semaphore rwsem;
 	struct idr ipcs_idr;

ipc/ipc_sysctl.c +2 -2

index 8ad93c29f511..a599963d58bf 100644
@@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
-		.procname	= "sem_next_id",
+		.procname	= "sem_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
 		.mode		= 0644,

@@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{
-		.procname	= "msg_next_id",
+		.procname	= "msg_nextid",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
 		.mode		= 0644,

ipc/namespace.c +2 -9

index f59a89966f92..84eaeba9e96c 100644
@@ -109,20 +109,13 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 {
 	struct kern_ipc_perm *perm;
 	int next_id;
-	int total, in_use;
 
 	down_write(&ids->rwsem);
-
-	in_use = ids->in_use;
-
-	for (total = 0, next_id = 0; total < in_use; next_id++) {
-		perm = idr_find(&ids->ipcs_idr, next_id);
-		if (perm == NULL)
-			continue;
+	next_id = 0;
+	while ((perm = idr_get_next(&ids->ipcs_idr, &next_id))) {
 		rcu_read_lock();
 		ipc_lock_object(perm);
 		free(ns, perm);
-		total++;
 	}
 	up_write(&ids->rwsem);
 }

ipc/util.c +9 -41

index 4ed5a17dd06f..ce6bf18e54df 100644
@@ -118,7 +118,6 @@ int ipc_init_ids(struct ipc_ids *ids)
 {
 	int err;
 	ids->in_use = 0;
-	ids->seq = 0;
 	init_rwsem(&ids->rwsem);
 	err = rhashtable_init(&ids->key_ht, &ipc_kht_params);
 	if (err)

@@ -192,46 +191,18 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
 	return NULL;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-/*
- * Specify desired id for next allocated IPC object.
- */
-#define ipc_idr_alloc(ids, new)						\
-	idr_alloc(&(ids)->ipcs_idr, (new),				\
-		  (ids)->next_id < 0 ? 0 : ipcid_to_idx((ids)->next_id),\
-		  0, GFP_NOWAIT)
 
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
-			      struct kern_ipc_perm *new)
+static int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
 {
-	if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
-		new->seq = ids->seq++;
-		if (ids->seq > IPCID_SEQ_MAX)
-			ids->seq = 0;
-	} else {
-		new->seq = ipcid_to_seqx(ids->next_id);
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	if (ids->next_id >= 0) {
+		idr_set_cursor(&ids->ipcs_idr, ids->next_id);
 		ids->next_id = -1;
 	}
-
-	return SEQ_MULTIPLIER * new->seq + id;
-}
-
-#else
-#define ipc_idr_alloc(ids, new)					\
-	idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT)
-
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
-			      struct kern_ipc_perm *new)
-{
-	new->seq = ids->seq++;
-	if (ids->seq > IPCID_SEQ_MAX)
-		ids->seq = 0;
-
-	return SEQ_MULTIPLIER * new->seq + id;
+#endif
+	return idr_alloc_cyclic(&ids->ipcs_idr, (new), 0, 0, GFP_NOWAIT);
 }
 
-#endif /* CONFIG_CHECKPOINT_RESTORE */
-
 /**
  * ipc_addid - add an ipc identifier
  * @ids: ipc identifier set

@@ -251,9 +222,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	kgid_t egid;
 	int id, err;
 
-	if (limit > IPCMNI)
-		limit = IPCMNI;
-
 	if (!ids->tables_initialized || ids->in_use >= limit)
 		return -ENOSPC;
 

@@ -290,7 +258,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	if (id > ids->max_id)
 		ids->max_id = id;
 
-	new->id = ipc_buildid(id, ids, new);
+	new->id = id;
 
 	return id;
 }

@@ -430,7 +398,7 @@ static void ipc_kht_remove(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
  */
 void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
 {
-	int lid = ipcid_to_idx(ipcp->id);
+	int lid = ipcp->id;
 
 	idr_remove(&ids->ipcs_idr, lid);
 	ipc_kht_remove(ids, ipcp);

@@ -563,7 +531,7 @@ void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out)
 struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id)
 {
 	struct kern_ipc_perm *out;
-	int lid = ipcid_to_idx(id);
+	int lid = id;
 
 	if (unlikely(!ids->tables_initialized))
 		return ERR_PTR(-EINVAL);

@@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 					      loff_t *new_pos)
 {

ipc/util.c +9 -12

-	if (total >= ids->in_use)
+	/* Out of range - return NULL to terminate iteration */
+	if (pos > INT_MAX)
 		return NULL;
 
-	for (; pos < IPCMNI; pos++) {
-		ipc = idr_find(&ids->ipcs_idr, pos);
-		if (ipc != NULL) {
-			*new_pos = pos + 1;
-			rcu_read_lock();
-			ipc_lock_object(ipc);
-			return ipc;
-		}
-	}
+	ipc = idr_get_next(&ids->ipcs_idr, &id);
+	if (!ipc)
+		return NULL;
 
-	/* Out of range - return NULL to terminate iteration */
-	return NULL;
+	*new_pos = id + 1;
+	rcu_read_lock();
+	ipc_lock_object(ipc);
+	return ipc;
 }
 
 static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)

ipc/util.h +0 -9

index 89b8ec176fc4..de8e27367f0c 100644
@@ -15,8 +15,6 @@
 #include <linux/err.h>
 #include <linux/ipc_namespace.h>
 
-#define SEQ_MULTIPLIER	(IPCMNI)
-
 int sem_init(void);
 int msg_init(void);
 void shm_init(void);

@@ -93,10 +91,6 @@ void __init ipc_init_proc_interface(const char *path, const char *header,
 #define IPC_MSG_IDS	1
 #define IPC_SHM_IDS	2
 
-#define ipcid_to_idx(id) ((id) % SEQ_MULTIPLIER)
-#define ipcid_to_seqx(id) ((id) / SEQ_MULTIPLIER)
-#define IPCID_SEQ_MAX min_t(int, INT_MAX/SEQ_MULTIPLIER, USHRT_MAX)
-
 /* must be called with ids->rwsem acquired for writing */
 int ipc_addid(struct ipc_ids *, struct kern_ipc_perm *, int);
 

@@ -120,9 +114,6 @@ static inline int ipc_get_maxid(struct ipc_ids *ids)
 	if (ids->in_use == 0)
 		return -1;
 
-	if (ids->in_use == IPCMNI)
-		return IPCMNI - 1;
-
 	return ids->max_id;
 }
 

ipc/namespace.c +0 -7

@@ -109,20 +109,13 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 {
 	struct kern_ipc_perm *perm;
 	int next_id;
-	int total, in_use;
 
 	down_write(&ids->rwsem);
-
-	in_use = ids->in_use;
-
-	for (total = 0, next_id = 0; total < in_use; next_id++) {
-		perm = idr_find(&ids->ipcs_idr, next_id);
-		if (perm == NULL)

ipc/util.c +3 -0

@@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 					      loff_t *new_pos)
 {
 	struct kern_ipc_perm *ipc;
+	int id;
 
+	/* Out of range - return NULL to terminate iteration */
+	if (pos > INT_MAX)
 		return NULL;

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace. It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

This can be implemented trivially with the current code using idr_alloc_cyclic.

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid. That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly. New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")

Signed-off-by "Eric W. Biederman" <[email protected]>

---

Waiman please take a look at this and run it through some tests etc, I am pretty certain something like this patch is all you need to do to sort out ipc assignment. Not messing with sysctls needed.

 include/linux/ipc.h           |  2 --
 include/linux/ipc_namespace.h |  1 -
 ipc/ipc_sysctl.c              |  6 ++--
 ipc/namespace.c               | 11 ++----
 ipc/util.c                    | 80 ++++++++++---------------------------------
 ipc/util.h                    | 11 +-----
 6 files changed, 25 insertions(+), 86 deletions(-)

include/linux/ipc.h +0 -2

index 821b2f260992..6cc2df7f7ac9 100644
@@ -8,8 +8,6 @@
 #include <uapi/linux/ipc.h>
 #include <linux/refcount.h>
 
-#define IPCMNI 32768  /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
-
 /* used by in-kernel data structures */
 struct kern_ipc_perm {
 	spinlock_t	lock;

include/linux/ipc_namespace.h +0 -1

index b5630c8eb2f3..cab33b6a8236 100644
@@ -15,7 +15,6 @@ struct user_namespace;
 
 struct ipc_ids {
 	int in_use;
-	unsigned short seq;
 	bool tables_initialized;
 	struct rw_semaphore rwsem;
 	struct idr ipcs_idr;

ipc/ipc_sysctl.c +2 -2

index 8ad93c29f511..a599963d58bf 100644
@@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
-		.procname	= "sem_next_id",
+		.procname	= "sem_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
 		.mode		= 0644,

@@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{
-		.procname	= "msg_next_id",
+		.procname	= "msg_nextid",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
 		.mode		= 0644,

ipc/namespace.c +2 -9

index f59a89966f92..84eaeba9e96c 100644
@@ -109,20 +109,13 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 {
 	struct kern_ipc_perm *perm;
 	int next_id;
-	int total, in_use;
 
 	down_write(&ids->rwsem);
-
-	in_use = ids->in_use;
-
-	for (total = 0, next_id = 0; total < in_use; next_id++) {
-		perm = idr_find(&ids->ipcs_idr, next_id);
-		if (perm == NULL)
-			continue;
+	next_id = 0;
+	while ((perm = idr_get_next(&ids->ipcs_idr, &next_id))) {
 		rcu_read_lock();
 		ipc_lock_object(perm);
 		free(ns, perm);
-		total++;
 	}
 	up_write(&ids->rwsem);
 }

ipc/util.c +9 -41

index 4ed5a17dd06f..ce6bf18e54df 100644
@@ -118,7 +118,6 @@ int ipc_init_ids(struct ipc_ids *ids)
 {
 	int err;
 	ids->in_use = 0;
-	ids->seq = 0;
 	init_rwsem(&ids->rwsem);
 	err = rhashtable_init(&ids->key_ht, &ipc_kht_params);
 	if (err)

@@ -192,46 +191,18 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
 	return NULL;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-/*
- * Specify desired id for next allocated IPC object.
- */
-#define ipc_idr_alloc(ids, new)						\
-	idr_alloc(&(ids)->ipcs_idr, (new),				\
-		  (ids)->next_id < 0 ? 0 : ipcid_to_idx((ids)->next_id),\
-		  0, GFP_NOWAIT)
 
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
-			      struct kern_ipc_perm *new)
+static int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
 {
-	if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
-		new->seq = ids->seq++;
-		if (ids->seq > IPCID_SEQ_MAX)
-			ids->seq = 0;
-	} else {
-		new->seq = ipcid_to_seqx(ids->next_id);
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	if (ids->next_id >= 0) {
+		idr_set_cursor(&ids->ipcs_idr, ids->next_id);
 		ids->next_id = -1;
 	}
-
-	return SEQ_MULTIPLIER * new->seq + id;
-}
-
-#else
-#define ipc_idr_alloc(ids, new)					\
-	idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT)
-
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
-			      struct kern_ipc_perm *new)
-{
-	new->seq = ids->seq++;
-	if (ids->seq > IPCID_SEQ_MAX)
-		ids->seq = 0;
-
-	return SEQ_MULTIPLIER * new->seq + id;
+#endif
+	return idr_alloc_cyclic(&ids->ipcs_idr, (new), 0, 0, GFP_NOWAIT);
 }
 
-#endif /* CONFIG_CHECKPOINT_RESTORE */
-
 /**
  * ipc_addid - add an ipc identifier
  * @ids: ipc identifier set

@@ -251,9 +222,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	kgid_t egid;
 	int id, err;
 
-	if (limit > IPCMNI)
-		limit = IPCMNI;
-
 	if (!ids->tables_initialized || ids->in_use >= limit)
 		return -ENOSPC;
 

@@ -290,7 +258,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	if (id > ids->max_id)
 		ids->max_id = id;
 
-	new->id = ipc_buildid(id, ids, new);
+	new->id = id;
 
 	return id;
 }

@@ -430,7 +398,7 @@ static void ipc_kht_remove(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
  */
 void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
 {
-	int lid = ipcid_to_idx(ipcp->id);
+	int lid = ipcp->id;
 
 	idr_remove(&ids->ipcs_idr, lid);
 	ipc_kht_remove(ids, ipcp);

@@ -563,7 +531,7 @@ void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out)
 struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id)
 {
 	struct kern_ipc_perm *out;
-	int lid = ipcid_to_idx(id);
+	int lid = id;
 
 	if (unlikely(!ids->tables_initialized))
 		return ERR_PTR(-EINVAL);

@@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 					      loff_t *new_pos)
 {

ipc/util.c +9 -12

-	if (total >= ids->in_use)
+	/* Out of range - return NULL to terminate iteration */
+	if (pos > INT_MAX)
 		return NULL;
 
-	for (; pos < IPCMNI; pos++) {
-		ipc = idr_find(&ids->ipcs_idr, pos);
-		if (ipc != NULL) {
-			*new_pos = pos + 1;
-			rcu_read_lock();
-			ipc_lock_object(ipc);
-			return ipc;
-		}
-	}
+	ipc = idr_get_next(&ids->ipcs_idr, &id);
+	if (!ipc)
+		return NULL;
 
-	/* Out of range - return NULL to terminate iteration */
-	return NULL;
+	*new_pos = id + 1;
+	rcu_read_lock();
+	ipc_lock_object(ipc);
+	return ipc;
 }
 
 static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)

ipc/util.h +0 -9

index 89b8ec176fc4..de8e27367f0c 100644
@@ -15,8 +15,6 @@
 #include <linux/err.h>
 #include <linux/ipc_namespace.h>
 
-#define SEQ_MULTIPLIER	(IPCMNI)
-
 int sem_init(void);
 int msg_init(void);
 void shm_init(void);

@@ -93,10 +91,6 @@ void __init ipc_init_proc_interface(const char *path, const char *header,
 #define IPC_MSG_IDS	1
 #define IPC_SHM_IDS	2
 
-#define ipcid_to_idx(id) ((id) % SEQ_MULTIPLIER)
-#define ipcid_to_seqx(id) ((id) / SEQ_MULTIPLIER)
-#define IPCID_SEQ_MAX min_t(int, INT_MAX/SEQ_MULTIPLIER, USHRT_MAX)
-
 /* must be called with ids->rwsem acquired for writing */
 int ipc_addid(struct ipc_ids *, struct kern_ipc_perm *, int);
 

@@ -120,9 +114,6 @@ static inline int ipc_get_maxid(struct ipc_ids *ids)
 	if (ids->in_use == 0)
 		return -1;
 
-	if (ids->in_use == IPCMNI)
-		return IPCMNI - 1;
-
 	return ids->max_id;
 }
 

Quoted 93 lines · Eric W. Biederman

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace. It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

This can be implemented trivially with the current code using idr_alloc_cyclic.

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid. That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly. New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

Waiman please take a look at this and run it through some tests etc, I am pretty certain something like this patch is all you need to do to sort out ipc assignment. Not messing with sysctls needed.

 include/linux/ipc.h           |  2 --
 include/linux/ipc_namespace.h |  1 -
 ipc/ipc_sysctl.c              |  6 ++--
 ipc/namespace.c               | 11 ++----
 ipc/util.c                    | 80 ++++++++++---------------------------------
 ipc/util.h                    | 11 +-----
 6 files changed, 25 insertions(+), 86 deletions(-)

include/linux/ipc.h +0 -2

index 821b2f260992..6cc2df7f7ac9 100644
@@ -8,8 +8,6 @@
 #include <uapi/linux/ipc.h>
 #include <linux/refcount.h>
 
-#define IPCMNI 32768  /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
-
 /* used by in-kernel data structures */
 struct kern_ipc_perm {
 	spinlock_t	lock;

include/linux/ipc_namespace.h +0 -1

index b5630c8eb2f3..cab33b6a8236 100644
@@ -15,7 +15,6 @@ struct user_namespace;
 
 struct ipc_ids {
 	int in_use;
-	unsigned short seq;
 	bool tables_initialized;
 	struct rw_semaphore rwsem;
 	struct idr ipcs_idr;

ipc/ipc_sysctl.c +2 -3

index 8ad93c29f511..a599963d58bf 100644
@@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
-		.procname	= "sem_next_id",
+		.procname	= "sem_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
 		.mode		= 0644,

@@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{
-		.procname	= "msg_next_id",
+		.procname	= "msg_nextid",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
 		.mode		= 0644,

@@ -194,7 +194,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{
-		.procname	= "shm_next_id",

ipc/ipc_sysctl.c static struct ctl_table ipc_kern_table[] = { · line 200

Quoted 4 lines

ipc/ipc_sysctl.c +1 -0

+		.procname	= "shm_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
 		.mode		= 0644,

So you are changing the names of existing sysctl parameters. Will it be better to add new sysctl to indicate that the rule has changed instead?

Quoted 109 lines

ipc/util.c +9 -44

index 4ed5a17dd06f..ce6bf18e54df 100644
@@ -118,7 +118,6 @@ int ipc_init_ids(struct ipc_ids *ids)
 {
 	int err;
 	ids->in_use = 0;
-	ids->seq = 0;
 	init_rwsem(&ids->rwsem);
 	err = rhashtable_init(&ids->key_ht, &ipc_kht_params);
 	if (err)

@@ -192,46 +191,18 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
 	return NULL;
 }
 
-#ifdef CONFIG_CHECKPOINT_RESTORE
-/*
- * Specify desired id for next allocated IPC object.
- */
-#define ipc_idr_alloc(ids, new)						\
-	idr_alloc(&(ids)->ipcs_idr, (new),				\
-		  (ids)->next_id < 0 ? 0 : ipcid_to_idx((ids)->next_id),\
-		  0, GFP_NOWAIT)
 
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
-			      struct kern_ipc_perm *new)
+static int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
 {
-	if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
-		new->seq = ids->seq++;
-		if (ids->seq > IPCID_SEQ_MAX)
-			ids->seq = 0;
-	} else {
-		new->seq = ipcid_to_seqx(ids->next_id);
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	if (ids->next_id >= 0) {
+		idr_set_cursor(&ids->ipcs_idr, ids->next_id);
 		ids->next_id = -1;
 	}
-
-	return SEQ_MULTIPLIER * new->seq + id;
-}
-
-#else
-#define ipc_idr_alloc(ids, new)					\
-	idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT)
-
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
-			      struct kern_ipc_perm *new)
-{
-	new->seq = ids->seq++;
-	if (ids->seq > IPCID_SEQ_MAX)
-		ids->seq = 0;
-
-	return SEQ_MULTIPLIER * new->seq + id;
+#endif
+	return idr_alloc_cyclic(&ids->ipcs_idr, (new), 0, 0, GFP_NOWAIT);
 }
 
-#endif /* CONFIG_CHECKPOINT_RESTORE */
-
 /**
  * ipc_addid - add an ipc identifier
  * @ids: ipc identifier set

@@ -251,9 +222,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	kgid_t egid;
 	int id, err;
 
-	if (limit > IPCMNI)
-		limit = IPCMNI;
-
 	if (!ids->tables_initialized || ids->in_use >= limit)
 		return -ENOSPC;
 

@@ -290,7 +258,7 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
 	if (id > ids->max_id)
 		ids->max_id = id;
 
-	new->id = ipc_buildid(id, ids, new);
+	new->id = id;
 
 	return id;
 }

@@ -430,7 +398,7 @@ static void ipc_kht_remove(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
  */
 void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
 {
-	int lid = ipcid_to_idx(ipcp->id);
+	int lid = ipcp->id;
 
 	idr_remove(&ids->ipcs_idr, lid);
 	ipc_kht_remove(ids, ipcp);

@@ -563,7 +531,7 @@ void ipc64_perm_to_ipc_perm(struct ipc64_perm *in, struct ipc_perm *out)
 struct kern_ipc_perm *ipc_obtain_object_idr(struct ipc_ids *ids, int id)
 {
 	struct kern_ipc_perm *out;
-	int lid = ipcid_to_idx(id);
+	int lid = id;
 
 	if (unlikely(!ids->tables_initialized))
 		return ERR_PTR(-EINVAL);

@@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 					      loff_t *new_pos)
 {
 	struct kern_ipc_perm *ipc;
-	int total, id;
-
-	total = 0;

ipc/util.c static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos, · line 728

Quoted 6 lines

ipc/util.c +1 -5

-	for (id = 0; id < pos && total < ids->in_use; id++) {
-		ipc = idr_find(&ids->ipcs_idr, id);
-		if (ipc != NULL)
-			total++;
-	}
+	int id;

I think you need to initialize id to pos. Right?

Quoted 70 lines

ipc/util.c +9 -12

-	if (total >= ids->in_use)
+	/* Out of range - return NULL to terminate iteration */
+	if (pos > INT_MAX)
 		return NULL;
 
-	for (; pos < IPCMNI; pos++) {
-		ipc = idr_find(&ids->ipcs_idr, pos);
-		if (ipc != NULL) {
-			*new_pos = pos + 1;
-			rcu_read_lock();
-			ipc_lock_object(ipc);
-			return ipc;
-		}
-	}
+	ipc = idr_get_next(&ids->ipcs_idr, &id);
+	if (!ipc)
+		return NULL;
 
-	/* Out of range - return NULL to terminate iteration */
-	return NULL;
+	*new_pos = id + 1;
+	rcu_read_lock();
+	ipc_lock_object(ipc);
+	return ipc;
 }
 
 static void *sysvipc_proc_next(struct seq_file *s, void *it, loff_t *pos)

ipc/util.h +1 -10

index 89b8ec176fc4..de8e27367f0c 100644
@@ -15,8 +15,6 @@
 #include <linux/err.h>
 #include <linux/ipc_namespace.h>
 
-#define SEQ_MULTIPLIER	(IPCMNI)
-
 int sem_init(void);
 int msg_init(void);
 void shm_init(void);

@@ -93,10 +91,6 @@ void __init ipc_init_proc_interface(const char *path, const char *header,
 #define IPC_MSG_IDS	1
 #define IPC_SHM_IDS	2
 
-#define ipcid_to_idx(id) ((id) % SEQ_MULTIPLIER)
-#define ipcid_to_seqx(id) ((id) / SEQ_MULTIPLIER)
-#define IPCID_SEQ_MAX min_t(int, INT_MAX/SEQ_MULTIPLIER, USHRT_MAX)
-
 /* must be called with ids->rwsem acquired for writing */
 int ipc_addid(struct ipc_ids *, struct kern_ipc_perm *, int);
 

@@ -120,9 +114,6 @@ static inline int ipc_get_maxid(struct ipc_ids *ids)
 	if (ids->in_use == 0)
 		return -1;
 
-	if (ids->in_use == IPCMNI)
-		return IPCMNI - 1;
-
 	return ids->max_id;
 }
 

@@ -163,7 +154,7 @@ extern int store_msg(void __user *dest, struct msg_msg *msg, size_t len);
 
 static inline int ipc_checkid(struct kern_ipc_perm *ipcp, int uid)
 {
-	return uid / SEQ_MULTIPLIER != ipcp->seq;
+	return uid != ipcp->seq;
 }
 

ipc/util.h extern int store_msg(void __user *dest, struct msg_msg *msg, size_t len); · line 160

Quoted 1 line

ipc/util.h +0 -0

 static inline void ipc_lock_object(struct kern_ipc_perm *perm)

I don't know the history why the id management of SysV IPC was designed in such a convoluted way, but the patch does make sense to me.

Quoted 93 lines · Waiman Long

Quoted 92 lines · Eric W. Biederman

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace. It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

This can be implemented trivially with the current code using idr_alloc_cyclic.

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid. That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly. New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

Waiman please take a look at this and run it through some tests etc, I am pretty certain something like this patch is all you need to do to sort out ipc assignment. Not messing with sysctls needed.

 include/linux/ipc.h           |  2 --
 include/linux/ipc_namespace.h |  1 -
 ipc/ipc_sysctl.c              |  6 ++--
 ipc/namespace.c               | 11 ++----
 ipc/util.c                    | 80 ++++++++++---------------------------------
 ipc/util.h                    | 11 +-----
 6 files changed, 25 insertions(+), 86 deletions(-)

include/linux/ipc.h +0 -2

index 821b2f260992..6cc2df7f7ac9 100644
@@ -8,8 +8,6 @@
 #include <uapi/linux/ipc.h>
 #include <linux/refcount.h>
 
-#define IPCMNI 32768  /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
-
 /* used by in-kernel data structures */
 struct kern_ipc_perm {
 	spinlock_t	lock;

include/linux/ipc_namespace.h +0 -1

index b5630c8eb2f3..cab33b6a8236 100644
@@ -15,7 +15,6 @@ struct user_namespace;
 
 struct ipc_ids {
 	int in_use;
-	unsigned short seq;
 	bool tables_initialized;
 	struct rw_semaphore rwsem;
 	struct idr ipcs_idr;

ipc/ipc_sysctl.c +2 -2

index 8ad93c29f511..a599963d58bf 100644
@@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
-		.procname	= "sem_next_id",
+		.procname	= "sem_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
 		.mode		= 0644,

@@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{
-		.procname	= "msg_next_id",
+		.procname	= "msg_nextid",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
 		.mode		= 0644,

@@ -194,7 +194,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{

Quoted 8 lines

ipc/ipc_sysctl.c static struct ctl_table ipc_kern_table[] = { · line 200

Quoted 5 lines

ipc/ipc_sysctl.c +1 -1

-		.procname	= "shm_next_id",
+		.procname	= "shm_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
 		.mode		= 0644,

So you are changing the names of existing sysctl parameters. Will it be better to add new sysctl to indicate that the rule has changed instead?

In practice I am replacing one set of sysctls with another, that work very similarly but not quite the same. As we can't keep the existing semantics removing the old sysctl seems correct. Likewise adding a new sysctl with slightly changed semantics seems correct.

This needs an accompanying patch to CRIU to see which sysctls are available and to change it's behavior based upon that. The practical question is what makes it easiest not to confuse CRIU.

Not having the sysctl should be something that CRIU detects today and the old versions should fail gracefully. But testing is needed. Adding a new sysctl to say the behavior has changed and reusing the old names won't have the same effect of disabling existing versions of CRIU.

Quoted 3 lines

I don't know the history why the id management of SysV IPC was designed in such a convoluted way, but the patch does make sense to me.

I don't have the full history and we might wind up finding more as we run this patch through it's paces.

The earliest history I know is what I read in Advanced Programming in the Unix Environment (which predates linux). It described the ipc ids as assigned from a counter that wraps. I thought like my patch implements. On closer reading it has a counter that increases each time the slot is used, and then wraps. Exactly like Linux before my patch. *Grrr*

The existing structure of the bifurcated is present in Linux 1.0. At that time SHMMNI was 256. SHMMNI was the size of a static array of shm segments. The high 24 bits held a sequence number that was incremented when a segment was removed at the time. Presumably the upper bits were incremented to avoid swiftly reusing the same shm ids.

Hmm. I took a quick look at FreeBSD10 and it has the exact same split in the id. So userspace may actually depend upon that split.

Which comes down to the fundamental question what depends upon what. How do other operating systems like Solaris handle this?

Does any nix flavor support more that 16bits worth of shm segments?

The API has been deprecated for the last 20 years and we are still keeping it alive. Sigh.

Still there is fundamentally only one thing the kernel can do if we wish to increase the number of shm segments.

Please take my patch and test it out and see if you can find anything that cares about the change. Except for needing id reuse to be infrequent I can not imagine that there is anything that cares.

It could very reasonably be argued that my when shmmni is < INT_MAX my patch implements a version of the existing algorithm. As we go through all of the possible ids before we reuse any of them.

Eric

Quoted 112 lines · Eric W. Biederman

Quoted 93 lines · Waiman Long

Quoted 92 lines · Eric W. Biederman

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace. It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

This can be implemented trivially with the current code using idr_alloc_cyclic.

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid. That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly. New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

Updates: 03f595668017 ("ipc: add sysctl to specify desired next object id")

Signed-off-by "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>

---

Waiman please take a look at this and run it through some tests etc, I am pretty certain something like this patch is all you need to do to sort out ipc assignment. Not messing with sysctls needed.

 include/linux/ipc.h           |  2 --
 include/linux/ipc_namespace.h |  1 -
 ipc/ipc_sysctl.c              |  6 ++--
 ipc/namespace.c               | 11 ++----
 ipc/util.c                    | 80 ++++++++++---------------------------------
 ipc/util.h                    | 11 +-----
 6 files changed, 25 insertions(+), 86 deletions(-)

include/linux/ipc.h +0 -2

index 821b2f260992..6cc2df7f7ac9 100644
@@ -8,8 +8,6 @@
 #include <uapi/linux/ipc.h>
 #include <linux/refcount.h>
 
-#define IPCMNI 32768  /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
-
 /* used by in-kernel data structures */
 struct kern_ipc_perm {
 	spinlock_t	lock;

include/linux/ipc_namespace.h +0 -1

index b5630c8eb2f3..cab33b6a8236 100644
@@ -15,7 +15,6 @@ struct user_namespace;
 
 struct ipc_ids {
 	int in_use;
-	unsigned short seq;
 	bool tables_initialized;
 	struct rw_semaphore rwsem;
 	struct idr ipcs_idr;

ipc/ipc_sysctl.c +2 -2

index 8ad93c29f511..a599963d58bf 100644
@@ -176,7 +176,7 @@ static struct ctl_table ipc_kern_table[] = {
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
-		.procname	= "sem_next_id",
+		.procname	= "sem_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SEM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SEM_IDS].next_id),
 		.mode		= 0644,

@@ -185,7 +185,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{
-		.procname	= "msg_next_id",
+		.procname	= "msg_nextid",
 		.data		= &init_ipc_ns.ids[IPC_MSG_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_MSG_IDS].next_id),
 		.mode		= 0644,

@@ -194,7 +194,7 @@ static struct ctl_table ipc_kern_table[] = {
 		.extra2		= &int_max,
 	},
 	{

Quoted 8 lines

ipc/ipc_sysctl.c static struct ctl_table ipc_kern_table[] = { · line 200

Quoted 5 lines

ipc/ipc_sysctl.c +1 -1

-		.procname	= "shm_next_id",
+		.procname	= "shm_nextid",
 		.data		= &init_ipc_ns.ids[IPC_SHM_IDS].next_id,
 		.maxlen		= sizeof(init_ipc_ns.ids[IPC_SHM_IDS].next_id),
 		.mode		= 0644,

So you are changing the names of existing sysctl parameters. Will it be better to add new sysctl to indicate that the rule has changed instead?

In practice I am replacing one set of sysctls with another, that work very similarly but not quite the same. As we can't keep the existing semantics removing the old sysctl seems correct. Likewise adding a new sysctl with slightly changed semantics seems correct.

This needs an accompanying patch to CRIU to see which sysctls are available and to change it's behavior based upon that. The practical question is what makes it easiest not to confuse CRIU.

Quoted 5 lines

Not having the sysctl should be something that CRIU detects today and the old versions should fail gracefully. But testing is needed. Adding a new sysctl to say the behavior has changed and reusing the old names won't have the same effect of disabling existing versions of CRIU.

That is fine as long as CRIU is the only user.

Quoted 19 lines

Quoted 3 lines

I don't know the history why the id management of SysV IPC was designed in such a convoluted way, but the patch does make sense to me.

I don't have the full history and we might wind up finding more as we run this patch through it's paces.

The earliest history I know is what I read in Advanced Programming in the Unix Environment (which predates linux). It described the ipc ids as assigned from a counter that wraps. I thought like my patch implements. On closer reading it has a counter that increases each time the slot is used, and then wraps. Exactly like Linux before my patch. *Grrr*

The existing structure of the bifurcated is present in Linux 1.0. At that time SHMMNI was 256. SHMMNI was the size of a static array of shm segments. The high 24 bits held a sequence number that was incremented when a segment was removed at the time. Presumably the upper bits were incremented to avoid swiftly reusing the same shm ids.

Quoted 2 lines

Hmm. I took a quick look at FreeBSD10 and it has the exact same split in the id. So userspace may actually depend upon that split.

Backward compatibility is the part that I am most worry about this patch. That is also the reason I asked why the ID is generated in such a way.

My original thinking was to have an extended mode where the IPCMNI becomes 8M from 32k. That will reduce the sequence number from 16 bits to 8 bits. The extended mode is enabled by adding, for example, a boot option. So this will be an opt-in feature instead of as a default.

Quoted 3 lines

Which comes down to the fundamental question what depends upon what. How do other operating systems like Solaris handle this?

I don't know how Solaris handle this, but I know they support up to 2^24 shm segments.

Quoted 13 lines

Does any nix flavor support more that 16bits worth of shm segments?

The API has been deprecated for the last 20 years and we are still keeping it alive. Sigh.

Still there is fundamentally only one thing the kernel can do if we wish to increase the number of shm segments.

Please take my patch and test it out and see if you can find anything that cares about the change. Except for needing id reuse to be infrequent I can not imagine that there is anything that cares.

Quoted 6 lines

It could very reasonably be argued that my when shmmni is < INT_MAX my patch implements a version of the existing algorithm. As we go through all of the possible ids before we reuse any of them.

Eric

Thanks for the patch, I am still thinking about what is the best way to handle this.

Cheers,
Longman

Cc'ing mtk, Manfred and linux-api.

See below.

Quoted 14 lines · Waiman Long

Quoted 13 lines · Eric W. Biederman

Quoted 11 lines · Waiman Long

Quoted 10 lines · Eric W. Biederman

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace.  It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid.  That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly.  New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

From a memory consumption point of view, 6 layers of tree will consume

Quoted 5 lines · Manfred Spraul

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

The radix tree uses a branching factor of 64 entries (6 bits) per level. The maximum ID is 31 bits (positive signed 32-bit integer). So the worst case for a single object is 6 pointer dereferences to find the object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12 cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2 (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.

Hello Mathew,

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Manfred Spraul

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

Quoted 6 lines

The radix tree uses a branching factor of 64 entries (6 bits) per level. The maximum ID is 31 bits (positive signed 32-bit integer). So the worst case for a single object is 6 pointer dereferences to find the object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12 cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2 (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.

I'm concerned about the up to 6 branches.
But this is just guessing, we need a test with a realistic workload.

Hello Mathew,

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Manfred Spraul

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

Quoted 6 lines

The radix tree uses a branching factor of 64 entries (6 bits) per level. The maximum ID is 31 bits (positive signed 32-bit integer). So the worst case for a single object is 6 pointer dereferences to find the object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12 cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2 (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.

I'm concerned about the up to 6 branches.
But this is just guessing, we need a test with a realistic workload.

Hello Mathew,

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Manfred Spraul

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

Quoted 6 lines

The radix tree uses a branching factor of 64 entries (6 bits) per level. The maximum ID is 31 bits (positive signed 32-bit integer). So the worst case for a single object is 6 pointer dereferences to find the object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12 cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2 (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.

I'm concerned about the up to 6 branches.
But this is just guessing, we need a test with a realistic workload.

Hello Mathew,

Quoted 6 lines · Matthew Wilcox

Quoted 5 lines · Manfred Spraul

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

Quoted 6 lines

The radix tree uses a branching factor of 64 entries (6 bits) per level. The maximum ID is 31 bits (positive signed 32-bit integer). So the worst case for a single object is 6 pointer dereferences to find the object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12 cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2 (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.

I'm concerned about the up to 6 branches.
But this is just guessing, we need a test with a realistic workload.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Is there a performance impact?
Right now, the idr tree is only large if there are lots of objects. What happens if we have only 1 object, with id=INT_MAX-1?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines · Eric W. Biederman

Quoted 9 lines

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid.  That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly.  New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

It means that all existing checkpoint/restore application will not work with a new kernel.
Everyone must first update the checkpoint/restore application, then update the kernel.

Is this acceptable?

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines · Eric W. Biederman

Quoted 9 lines

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid.  That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly.  New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

It means that all existing checkpoint/restore application will not work with a new kernel.
Everyone must first update the checkpoint/restore application, then update the kernel.

Is this acceptable?

Cc'ing mtk, Manfred and linux-api.

See below.

Quoted 14 lines · Waiman Long

Quoted 13 lines · Eric W. Biederman

Quoted 11 lines · Waiman Long

Quoted 10 lines · Eric W. Biederman

The define IPCMNI was originally the size of a statically sized array in the kernel and that has long since been removed. Therefore there is no fundamental reason for IPCMNI.

The only remaining use IPCMNI serves is as a convoluted way to format the ipc id to userspace.  It does not appear that anything except for the CHECKPOINT_RESTORE code even cares about this variety of assignment and the CHECKPOINT_RESTORE code only cares about this weirdness because it has to restore these peculiar ids.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Therefore make the assignment of ipc ids match the description in Advanced Programming in the Unix Environment and assign the next id until INT_MAX is hit then loop around to the lower ids.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

This can be implemented trivially with the current code using idr_alloc_cyclic.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

To make it possible to keep checkpoint/restore working I have renamed the sysctls from xxx_next_id to xxx_nextid.  That is enough change that a smart CRIU implementation can see that what is exported has changed, and act accordingly.  New kernels will be able to restore the old id's.

This code still needs some real world testing to verify my assumptions. And some work with the CRIU implementations to actually add the code that deals with the new for of id assignment.

ipc/namespace.c +0 -7

@@ -109,20 +109,13 @@ void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 {
 	struct kern_ipc_perm *perm;
 	int next_id;
-	int total, in_use;
 
 	down_write(&ids->rwsem);
-
-	in_use = ids->in_use;
-
-	for (total = 0, next_id = 0; total < in_use; next_id++) {
-		perm = idr_find(&ids->ipcs_idr, next_id);
-		if (perm == NULL)

ipc/util.c +3 -0

@@ -757,30 +725,20 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 					      loff_t *new_pos)
 {
 	struct kern_ipc_perm *ipc;
+	int id;
 
+	/* Out of range - return NULL to terminate iteration */
+	if (pos > INT_MAX)
 		return NULL;

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

Quoted 3 lines · Waiman Long

ipc/sem.c +3 -0

+	if (clamped)
+		pr_warn_once("sysctl: \"sem[3]\" was set out of range [%d, %d], clamped to %d.\n",
+			     0, IPCMNI, ns->sc_semmni);

Why warn if the kernel already does that? If we can avoid more code on drivers the better.

Luis

Add a range clamping test for the msgmni sysctl parameter to verify

Quoted 1 line · Waiman Long

Add a range clamping test for the msgmni sysctl parameter to verify

No! We don't want to test production values, please add a new test entry which is similar but does *not* modify system production values!

Luis