Skip to contentSkip to conversation

ipc: Clamp *mni to the real IPCMNI limit & increase that limit

PATCH Superseded Waiman Long 2 participants 20 messages Mar 16, 2018 – Mar 29, 2018

Subsystems (6)

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore
#0 lore raw

v5 cover ipc: Clamp *mni to the real IPCMNI limit & increase that limit

v4->v5:
- Revert the flags back to 16-bit so that there will be no change to the size of ctl_table.
- Enhance the sysctl_check_flags() as requested by Luis to perform more checks to spot incorrect ctl_table entries.
- Change the sysctl selftest to use dummy sysctls instead of production ones & enhance it to do more checks.
- Add one more sysctl selftest for registration failure.
- Add 2 ipc patches to add an extended mode to increase IPCMNI from 32k to 2M.
- Miscellaneous change to incorporate feedback comments from reviewers.

v3->v4:
- Remove v3 patches 1 & 2 as they have been merged into the mm tree.
- Change flags from uint16_t to unsigned int.
- Remove CTL_FLAGS_OOR_WARNED and use pr_warn_ratelimited() instead.
- Simplify the warning message code.
- Add a new patch to fail the ctl_table registration with invalid flag.
- Add a test case for range clamping in sysctl selftest.

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
v3 patch: https://lkml.org/lkml/2018/3/1/716
v4 patch: https://lkml.org/lkml/2018/3/12/867

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, may cause regressions if existing user setup scripts set those parameters above 32k as those scripts will now fail in this case.

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.

sysctl: "shmmni" was set out of range [0, 32768], clamped to 32768.

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

New sysctl selftests are added to exercise new code added by this patchset.

There are users out there requesting increase in the IPCMNI value. The last 2 patches attempt to do that by using a boot kernel parameter "ipcmni_extend" to increase the IPCMNI limit from 32k to 2M.

Eric Biederman had posted an RFC patch to just scrap the IPCMNI limit and open up the whole positive integer space for IPC IDs. A major issue that I have with this approach is that SysV IPC had been in use for over 20 years. We just don't know if there are user applications that have dependency on the way that the IDs are built. So drastic change like this may have the potential of breaking some applications.

I prefer a more conservative approach where users will observe no change in behavior unless they explictly opt in to enable the extended mode. I could open up the whole positive integer space in this case like what Eric did, but that will make the code more complex. So I just extend IPCMNI to 2M in this case and keep similar ID generation logic.

Waiman Long (9):
sysctl: Add flags to support min/max range clamping proc/sysctl: Provide additional ctl_table.flags checks
sysctl: Warn when a clamped sysctl parameter is set out of range
ipc: Clamp msgmni and shmmni to the real IPCMNI limit
ipc: Clamp semmni to the real IPCMNI limit
test_sysctl: Add range clamping test
test_sysctl: Add ctl_table registration failure test
ipc: Allow boot time extension of IPCMNI from 32k to 2M
ipc: Conserve sequence numbers in extended IPCMNI mode

 Documentation/admin-guide/kernel-parameters.txt |  3 +
 fs/proc/proc_sysctl.c                           | 62 ++++++++++++++++++++
 include/linux/ipc.h                             | 11 +++-
 include/linux/ipc_namespace.h                   |  1 +
 include/linux/sysctl.h                          | 32 +++++++++++
 ipc/ipc_sysctl.c                                | 33 ++++++++++-
 ipc/sem.c                                       | 25 ++++++++
 ipc/util.c                                      | 41 ++++++++-----
 ipc/util.h                                      | 23 +++++---
 kernel/sysctl.c                                 | 76 ++++++++++++++++++++++---
 lib/test_sysctl.c                               | 70 +++++++++++++++++++++++
 tools/testing/selftests/sysctl/sysctl.sh        | 67 ++++++++++++++++++++++
 12 files changed, 411 insertions(+), 33 deletions(-)
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v5 1/9 sysctl: Add flags to support min/max range clamping

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

The clamped value is either the maximum or minimum value that is closest to the input value provided by the user.

---
 include/linux/sysctl.h | 20 ++++++++++++++++++++
 kernel/sysctl.c        | 48 +++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 59 insertions(+), 9 deletions(-)
include/linux/sysctl.h +20 -0
index b769ecf..e446e1f 100644
@@ -116,6 +116,7 @@ struct ctl_table
 	void *data;
 	int maxlen;
 	umode_t mode;
+	uint16_t flags;
 	struct ctl_table *child;	/* Deprecated */
 	proc_handler *proc_handler;	/* Callback for text formatting */
 	struct ctl_table_poll *poll;
@@ -123,6 +124,25 @@ 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 the provided min/max value in case the user
+ *	provided a value outside of the given range. The clamped value is
+ *	either the provided minimum or maximum value that is closest to
+ *	the input value. No lower bound or upper bound checking will be
+ *	done if the corresponding minimum or maximum value isn't provided.
+ *
+ * At most 16 different flags are allowed.
+ */
+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

112 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#2 lore raw

Re: [PATCH v5 1/9] sysctl: Add flags to support min/max range clamping

Quoted 4 lines · Waiman Long

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.

I don't get it. Why define a generic range flag when we can be mores specific and you do that in your next patch. What's the point of this flag then?

Luis

#3 lore raw

Re: [PATCH v5 1/9] sysctl: Add flags to support min/max range clamping

Quoted 9 lines · Luis R. Rodriguez
Quoted 4 lines · Waiman Long

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.

I don't get it. Why define a generic range flag when we can be mores specific and you do that in your next patch. What's the point of this flag then?

Luis

I was thinking about using the signed/unsigned bits as just annotations for ranges for future extension. For the purpose of this patchset alone, I can merge the three bits into just two.

Cheers,
Longman

#4 lore raw

Re: [PATCH v5 1/9] sysctl: Add flags to support min/max range clamping

Quoted 6 lines · Waiman Long
Quoted 5 lines · Luis R. Rodriguez
Quoted 4 lines · Waiman Long

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.

Quoted 8 lines
Quoted 4 lines

I don't get it. Why define a generic range flag when we can be mores specific and you do that in your next patch. What's the point of this flag then?

Luis

I was thinking about using the signed/unsigned bits as just annotations for ranges for future extension. For the purpose of this patchset alone, I can merge the three bits into just two.

Only introduce flags which you will actually use in the same patch series.

Luis

#5 lore raw

Re: [PATCH v5 1/9] sysctl: Add flags to support min/max range clamping

Quoted 10 lines · Luis R. Rodriguez
Quoted 9 lines · Waiman Long
Quoted 8 lines · Luis R. Rodriguez
Quoted 4 lines · Waiman Long

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.

I don't get it. Why define a generic range flag when we can be mores specific and you do that in your next patch. What's the point of this flag then?

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Luis

I was thinking about using the signed/unsigned bits as just annotations for ranges for future extension. For the purpose of this patchset alone, I can merge the three bits into just two.

Only introduce flags which you will actually use in the same patch series.

Luis

Yes, will do. Since the merge window is coming, should I wait until it is over to send out the new patch?

Cheers,
Longman

#6 lore raw

Re: [PATCH v5 1/9] sysctl: Add flags to support min/max range clamping

Quoted 15 lines · Waiman Long
Quoted 14 lines · Luis R. Rodriguez
Quoted 13 lines · Waiman Long
Quoted 9 lines · Luis R. Rodriguez
Quoted 4 lines · Waiman Long

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.

I don't get it. Why define a generic range flag when we can be mores specific and you do that in your next patch. What's the point of this flag then?

Luis

I was thinking about using the signed/unsigned bits as just annotations for ranges for future extension. For the purpose of this patchset alone, I can merge the three bits into just two.

Quoted 6 lines
Quoted 3 lines

Only introduce flags which you will actually use in the same patch series.

Luis

Yes, will do. Since the merge window is coming, should I wait until it is over to send out the new patch?

Probably best. May be too tight for review now if Linus spins out a release this weekend.

Luis

#7 lore raw

PATCH v5 2/9 proc/sysctl: Provide additional ctl_table.flags checks

In reply to Waiman Long

Checking code is added to provide the following additional ctl_table.flags checks:

1) No unknown flag is allowed.
2) Minimum of a range cannot be larger than the maximum value.
3) The signed and unsigned flags are mutually exclusive.
4) The proc_handler should be consistent with the signed or unsigned flags.

Two new flags are added to indicate if the min/max values are signed or unsigned - CTL_FLAGS_SIGNED_RANGE & CTL_FLAGS_UNSIGNED_RANGE. These 2 flags can be optionally enabled for range checking purpose. But either one of them must be set with CTL_FLAGS_CLAMP_RANGE.

---
 fs/proc/proc_sysctl.c  | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysctl.h | 16 +++++++++++--
 2 files changed, 76 insertions(+), 2 deletions(-)
fs/proc/proc_sysctl.c +62 -0
index 493c975..2863ea1 100644
@@ -1092,6 +1092,66 @@ 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;
+	uint16_t sign_flags = CTL_FLAGS_SIGNED_RANGE|CTL_FLAGS_UNSIGNED_RANGE;
+
+	if ((table->flags & ~CTL_TABLE_FLAGS_ALL) ||
+	   ((table->flags & sign_flags) == sign_flags))
+		err = sysctl_err(path, table, "invalid flags");
+
+	if (table->flags & (CTL_FLAGS_CLAMP_RANGE | sign_flags)) {
+		int range_err = 0;
+		bool is_int = (table->maxlen == sizeof(int));
+
+		if (!is_int && (table->maxlen != sizeof(long))) {
+			range_err++;
+		} else if (!table->extra1 || !table->extra2) {
+			/* No min > max checking needed */
+		} else if (table->flags & CTL_FLAGS_UNSIGNED_RANGE) {
+			unsigned long min, max;
+
+			min = is_int ? *(unsigned int *)table->extra1
+				     : *(unsigned long *)table->extra1;
+			max = is_int ? *(unsigned int *)table->extra2
+				     : *(unsigned long *)table->extra2;
+			range_err += (min > max);
+		} else if (table->flags & CTL_FLAGS_SIGNED_RANGE) {
+
+			long min, max;
+
+			min = is_int ? *(int *)table->extra1
+				     : *(long *)table->extra1;
+			max = is_int ? *(int *)table->extra2
+				     : *(long *)table->extra2;
+			range_err += (min > max);
+		} else {
+			/*
+			 * Either CTL_FLAGS_UNSIGNED_RANGE or
+			 * CTL_FLAGS_SIGNED_RANGE should be set.
+			 */
+			range_err++;
+		}
+
+		/*
+		 * proc_handler and flag consistency check.
+		 */
+		if (((table->proc_handler == proc_douintvec_minmax)   ||
+		     (table->proc_handler == proc_doulongvec_minmax)) &&
+		    !(table->flags & CTL_FLAGS_UNSIGNED_RANGE))
+			range_err++;
+
+		if ((table->proc_handler == proc_dointvec_minmax) &&
+		   !(table->flags & CTL_FLAGS_SIGNED_RANGE))
+			range_err++;
+
+		if (range_err)
+			err |= sysctl_err(path, table, "Invalid range");
+	}
+	return err;
+}
+
 static int sysctl_check_table(const char *path, struct ctl_table *table)
 {
 	int err = 0;
@@ -1111,6 +1171,8 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
 		    (table->proc_handler == proc_doulongvec_ms_jiffies_minmax)) {
 			if (!table->data)
 				err |= sysctl_err(path, table, "No data");
+			if (table->flags)
+				err |= sysctl_check_flags(path, table);
 			if (!table->maxlen)
 				err |= sysctl_err(path, table, "No maxlen");
 			else
include/linux/sysctl.h +14 -2
index e446e1f..088f032 100644
@@ -134,14 +134,26 @@ struct ctl_table
  *	the input value. No lower bound or upper bound checking will be
  *	done if the corresponding minimum or maximum value isn't provided.
  *
+ * @CTL_FLAGS_SIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum signed values of
+ *	an allowable range.
+ *
+ * @CTL_FLAGS_UNSIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum unsigned values of
+ *	an allowable range.
+ *
  * At most 16 different flags are allowed.
  */
 enum ctl_table_flags {
 	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
-	__CTL_FLAGS_MAX			= BIT(1),
+	CTL_FLAGS_SIGNED_RANGE		= BIT(1),
+	CTL_FLAGS_UNSIGNED_RANGE	= BIT(2),
+	__CTL_FLAGS_MAX			= BIT(3),
 };
 
-#define CTL_TABLE_FLAGS_ALL	(__CTL_FLAGS_MAX - 1)
+#define CTL_TABLE_FLAGS_ALL		(__CTL_FLAGS_MAX - 1)
+#define CTL_FLAGS_CLAMP_RANGE_SIGNED	(CTL_FLAGS_CLAMP_RANGE|CTL_FLAGS_SIGNED_RANGE)
+#define CTL_FLAGS_CLAMP_RANGE_UNSIGNED	(CTL_FLAGS_CLAMP_RANGE|CTL_FLAGS_UNSIGNED_RANGE)
 
 struct ctl_node {
 	struct rb_node node;
Signature · 1 line
1.8.3.1
#8 lore raw

Re: [PATCH v5 2/9] proc/sysctl: Provide additional ctl_table.flags checks

Quoted 32 lines · Waiman Long

Checking code is added to provide the following additional ctl_table.flags checks:

1) No unknown flag is allowed.
2) Minimum of a range cannot be larger than the maximum value.
3) The signed and unsigned flags are mutually exclusive.
4) The proc_handler should be consistent with the signed or unsigned flags.

Two new flags are added to indicate if the min/max values are signed or unsigned - CTL_FLAGS_SIGNED_RANGE & CTL_FLAGS_UNSIGNED_RANGE. These 2 flags can be optionally enabled for range checking purpose. But either one of them must be set with CTL_FLAGS_CLAMP_RANGE.

---

include/linux/sysctl.h +7 -0
index e446e1f..088f032 100644
@@ -134,14 +134,26 @@ struct ctl_table
  *	the input value. No lower bound or upper bound checking will be
  *	done if the corresponding minimum or maximum value isn't provided.
  *
+ * @CTL_FLAGS_SIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum signed values of
+ *	an allowable range.
+ *
+ * @CTL_FLAGS_UNSIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum unsigned values of
+ *	an allowable range.

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

Quoted 10 lines
include/linux/sysctl.h +4 -1
+ *
  * At most 16 different flags are allowed.
  */
 enum ctl_table_flags {
 	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
-	__CTL_FLAGS_MAX			= BIT(1),
+	CTL_FLAGS_SIGNED_RANGE		= BIT(1),
+	CTL_FLAGS_UNSIGNED_RANGE	= BIT(2),
+	__CTL_FLAGS_MAX			= BIT(3),
 };

You are adding new flags which the user can set, and yet these are used internally.

It would be best if internal flags are just that, not flags that a user can set.

This patch should be folded with the first one.

I'm starting to loose hope on these patch sets.

Luis

#9 lore raw

Re: [PATCH v5 2/9] proc/sysctl: Provide additional ctl_table.flags checks

Quoted 45 lines · Luis R. Rodriguez
Quoted 31 lines · Waiman Long

Checking code is added to provide the following additional ctl_table.flags checks:

1) No unknown flag is allowed.
2) Minimum of a range cannot be larger than the maximum value.
3) The signed and unsigned flags are mutually exclusive.
4) The proc_handler should be consistent with the signed or unsigned flags.

Two new flags are added to indicate if the min/max values are signed or unsigned - CTL_FLAGS_SIGNED_RANGE & CTL_FLAGS_UNSIGNED_RANGE. These 2 flags can be optionally enabled for range checking purpose. But either one of them must be set with CTL_FLAGS_CLAMP_RANGE.

---

include/linux/sysctl.h +7 -0
index e446e1f..088f032 100644
@@ -134,14 +134,26 @@ struct ctl_table
  *	the input value. No lower bound or upper bound checking will be
  *	done if the corresponding minimum or maximum value isn't provided.
  *
+ * @CTL_FLAGS_SIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum signed values of
+ *	an allowable range.
+ *
+ * @CTL_FLAGS_UNSIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum unsigned values of
+ *	an allowable range.

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

Quoted 10 lines
include/linux/sysctl.h +4 -1
+ *
  * At most 16 different flags are allowed.
  */
 enum ctl_table_flags {
 	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
-	__CTL_FLAGS_MAX			= BIT(1),
+	CTL_FLAGS_SIGNED_RANGE		= BIT(1),
+	CTL_FLAGS_UNSIGNED_RANGE	= BIT(2),
+	__CTL_FLAGS_MAX			= BIT(3),
 };

You are adding new flags which the user can set, and yet these are used internally.

Quoted 7 lines

It would be best if internal flags are just that, not flags that a user can set.

This patch should be folded with the first one.

I'm starting to loose hope on these patch sets.

Luis

In order to do the correct min > max check, I need to know if the quantity is signed or not. Just looking at the proc_handler alone is not a reliable indicator if it is signed or unsigned.

Yes, I can put the signed bit into the previous patch.

-Longman

#10 lore raw

Re: [PATCH v5 2/9] proc/sysctl: Provide additional ctl_table.flags checks

Quoted 52 lines · Waiman Long
Quoted 51 lines · Luis R. Rodriguez
Quoted 31 lines · Waiman Long

Checking code is added to provide the following additional ctl_table.flags checks:

1) No unknown flag is allowed.
2) Minimum of a range cannot be larger than the maximum value.
3) The signed and unsigned flags are mutually exclusive.
4) The proc_handler should be consistent with the signed or unsigned flags.

Two new flags are added to indicate if the min/max values are signed or unsigned - CTL_FLAGS_SIGNED_RANGE & CTL_FLAGS_UNSIGNED_RANGE. These 2 flags can be optionally enabled for range checking purpose. But either one of them must be set with CTL_FLAGS_CLAMP_RANGE.

---

include/linux/sysctl.h +7 -0
index e446e1f..088f032 100644
@@ -134,14 +134,26 @@ struct ctl_table
  *	the input value. No lower bound or upper bound checking will be
  *	done if the corresponding minimum or maximum value isn't provided.
  *
+ * @CTL_FLAGS_SIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum signed values of
+ *	an allowable range.
+ *
+ * @CTL_FLAGS_UNSIGNED_RANGE: Set to indicate that the extra1 and extra2
+ *	fields are pointers to minimum and maximum unsigned values of
+ *	an allowable range.

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

Quoted 10 lines
include/linux/sysctl.h +4 -1
+ *
  * At most 16 different flags are allowed.
  */
 enum ctl_table_flags {
 	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
-	__CTL_FLAGS_MAX			= BIT(1),
+	CTL_FLAGS_SIGNED_RANGE		= BIT(1),
+	CTL_FLAGS_UNSIGNED_RANGE	= BIT(2),
+	__CTL_FLAGS_MAX			= BIT(3),
 };

You are adding new flags which the user can set, and yet these are used internally.

It would be best if internal flags are just that, not flags that a user can set.

This patch should be folded with the first one.

I'm starting to loose hope on these patch sets.

Quoted 7 lines
Quoted 1 line

Luis

In order to do the correct min > max check, I need to know if the quantity is signed or not. Just looking at the proc_handler alone is not a reliable indicator if it is signed or unsigned.

Yes, I can put the signed bit into the previous patch.

Do that and also remove the unused flags. It is confusing as a reviewer why a flag was added and then you use another flag later. Seriously, please take a bit more time to review your own patches prior to submission. Each change should make sense and have use in the patch series.

Luis

#11 lore raw

PATCH v5 3/9 sysctl: Warn when a clamped sysctl parameter is set out of range

In reply to 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.

---
 kernel/sysctl.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)
kernel/sysctl.c +36 -8
index af351ed..a9e3ed4 100644
@@ -17,6 +17,7 @@
  * The list_for_each() macro wasn't appropriate for the sysctl loop.
  *  Removed it and replaced it with older style, 03/23/00, Bill Wendling
  */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/module.h>
 #include <linux/aio.h>
@@ -2505,6 +2506,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,6 +2516,7 @@ struct do_proc_dointvec_minmax_conv_param {
 	int *min;
 	int *max;
 	uint16_t *flags;
+	const char *name;
 };
 
 static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
@@ -2521,24 +2524,35 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 					int write, void *data)
 {
 	struct do_proc_dointvec_minmax_conv_param *param = data;
+
 	if (write) {
 		int val = *negp ? -*lvalp : *lvalp;
+		bool clamped = false;
 		bool clamp = param->flags &&
 			   (*param->flags & CTL_FLAGS_CLAMP_RANGE);
 
 		if (param->min && *param->min > val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->min;
-			else
+				clamped = true;
+			} else {
 				return -EINVAL;
+			}
 		}
 		if (param->max && *param->max < val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->max;
-			else
+				clamped = true;
+			} else {
 				return -EINVAL;
+			}
 		}
 		*valp = val;
+		if (clamped && param->name)
+			pr_warn_ratelimited("\"%s\" was set out of range [%d, %d], clamped to %d.\n",
+				param->name,
+				param->min ? *param->min : -INT_MAX,
+				param->max ? *param->max :  INT_MAX, val);
 	} else {
 		int val = *valp;
 		if (val < 0) {
@@ -2576,6 +2590,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
 		.min = (int *) table->extra1,
 		.max = (int *) table->extra2,
 		.flags = &table->flags,
+		.name  = table->procname,
 	};
 	return do_proc_dointvec(table, write, buffer, lenp, ppos,
 				do_proc_dointvec_minmax_conv, &param);
@@ -2586,6 +2601,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write,
  * @min: pointer to minimum allowable value
  * @max: pointer to maximum allowable value
  * @flags: pointer to flags
+ * @name: sysctl parameter name
  *
  * The do_proc_douintvec_minmax_conv_param structure provides the
  * minimum and maximum values for doing range checking for those sysctl
@@ -2595,6 +2611,7 @@ struct do_proc_douintvec_minmax_conv_param {
 	unsigned int *min;
 	unsigned int *max;
 	uint16_t *flags;
+	const char *name;
 };
 
 static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
@@ -2605,6 +2622,7 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 
 	if (write) {
 		unsigned int val = *lvalp;
+		bool clamped = false;
 		bool clamp = param->flags &&
 			   (*param->flags & CTL_FLAGS_CLAMP_RANGE);
 
@@ -2612,18 +2630,27 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 			return -EINVAL;
 
 		if (param->min && *param->min > val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->min;
-			else
+				clamped = true;
+			} else {
 				return -ERANGE;
+			}
 		}
 		if (param->max && *param->max < val) {
-			if (clamp)
+			if (clamp) {
 				val = *param->max;
-			else
+				clamped = true;
+			} else {
 				return -ERANGE;
+			}
 		}
 		*valp = val;
+		if (clamped && param->name)
+			pr_warn_ratelimited("\"%s\" was set out of range [%u, %u], clamped to %u.\n",
+				param->name,
+				param->min ? *param->min : 0,
+				param->max ? *param->max : UINT_MAX, val);
 	} else {
 		unsigned int val = *valp;
 		*lvalp = (unsigned long) val;
@@ -2659,6 +2686,7 @@ int proc_douintvec_minmax(struct ctl_table *table, int write,
 		.min = (unsigned int *) table->extra1,
 		.max = (unsigned int *) table->extra2,
 		.flags = &table->flags,
+		.name  = table->procname,
 	};
 	return do_proc_douintvec(table, write, buffer, lenp, ppos,
 				 do_proc_douintvec_minmax_conv, &param);
Signature · 1 line
1.8.3.1
#12 lore raw

PATCH v5 4/9 ipc: Clamp msgmni and shmmni to the real IPCMNI limit

In reply to Waiman Long

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

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

---
 ipc/ipc_sysctl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
ipc/ipc_sysctl.c +7 -2
index 8ad93c2..088721e 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_SIGNED,
 	},
 	{
 		.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_SIGNED,
 	},
 	{
 		.procname	= "auto_msgmni",
Signature · 1 line
1.8.3.1
#13 lore raw

PATCH v5 5/9 ipc: Clamp semmni to the real IPCMNI limit

In reply to Waiman Long

For SysV semaphores, the semmni value is the last part of the 4-element sem number array. To make semmni behave in a similar way to msgmni and shmmni, we can't directly use the _minmax handler. Instead, a special sem specific handler is added to check the last argument to make sure that it is clamped to the [0, IPCMNI] range and prints a warning message once when an out-of-range value is being written. This does require duplicating some of the code in the _minmax handlers.

---
 ipc/ipc_sysctl.c | 12 +++++++++++-
 ipc/sem.c        | 25 +++++++++++++++++++++++++
 ipc/util.h       |  4 ++++
 3 files changed, 40 insertions(+), 1 deletion(-)
ipc/ipc_sysctl.c +11 -1
index 088721e..0ad7088 100644
@@ -88,12 +88,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;
@@ -177,7 +187,7 @@ 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,
 	},
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	{
ipc/sem.c +25 -0
index a4af049..faf2caa 100644
@@ -2337,3 +2337,28 @@ 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;
+
+	/*
+	 * Clamp semmni to the range [0, IPCMNI].
+	 */
+	if (ns->sc_semmni < 0) {
+		ns->sc_semmni = 0;
+		clamped = true;
+	}
+	if (ns->sc_semmni > IPCMNI) {
+		ns->sc_semmni = IPCMNI;
+		clamped = true;
+	}
+	if (clamped)
+		pr_warn_ratelimited("sysctl: \"sem[3]\" was set out of range [%d, %d], clamped to %d.\n",
+				     0, IPCMNI, ns->sc_semmni);
+}
+#endif
ipc/util.h +4 -0
index 89b8ec1..af57394 100644
@@ -206,6 +206,10 @@ int ipcget(struct ipc_namespace *ns, struct ipc_ids *ids,
 void free_ipcs(struct ipc_namespace *ns, struct ipc_ids *ids,
 		void (*free)(struct ipc_namespace *, struct kern_ipc_perm *));
 
+#ifdef CONFIG_PROC_SYSCTL
+extern void sem_check_semmni(struct ctl_table *table, struct ipc_namespace *ns);
+#endif
+
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 struct compat_ipc_perm {
Signature · 1 line
1.8.3.1
#14 lore raw

PATCH v5 6/9 test_sysctl: Add range clamping test

In reply to Waiman Long

Add a range clamping test to verify that the input value will be clamped if it exceeds the builtin maximum or minimum value.

Below is the expected test run result:

Running test: sysctl_test_0006 - run #0
Checking range minimum clamping ... ok
Checking range maximum clamping ... ok
Checking range minimum clamping ... ok
Checking range maximum clamping ... ok

---
 lib/test_sysctl.c                        | 29 ++++++++++++++++++
 tools/testing/selftests/sysctl/sysctl.sh | 52 ++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)
lib/test_sysctl.c +29 -0
index 3dd801c..7bb4cf7 100644
@@ -38,12 +38,18 @@
 
 static int i_zero;
 static int i_one_hundred = 100;
+static int signed_min = -10;
+static int signed_max = 10;
+static unsigned int unsigned_min = 10;
+static unsigned int unsigned_max = 30;
 
 struct test_sysctl_data {
 	int int_0001;
 	int int_0002;
 	int int_0003[4];
+	int range_0001;
 
+	unsigned int urange_0001;
 	unsigned int uint_0001;
 
 	char string_0001[65];
@@ -58,6 +64,9 @@ struct test_sysctl_data {
 	.int_0003[2] = 2,
 	.int_0003[3] = 3,
 
+	.range_0001 = 0,
+	.urange_0001 = 20,
+
 	.uint_0001 = 314,
 
 	.string_0001 = "(none)",
@@ -102,6 +111,26 @@ struct test_sysctl_data {
 		.mode		= 0644,
 		.proc_handler	= proc_dostring,
 	},
+	{
+		.procname	= "range_0001",
+		.data		= &test_data.range_0001,
+		.maxlen		= sizeof(test_data.range_0001),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.flags		= CTL_FLAGS_CLAMP_RANGE_SIGNED,
+		.extra1		= &signed_min,
+		.extra2		= &signed_max,
+	},
+	{
+		.procname	= "urange_0001",
+		.data		= &test_data.urange_0001,
+		.maxlen		= sizeof(test_data.urange_0001),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec_minmax,
+		.flags		= CTL_FLAGS_CLAMP_RANGE_UNSIGNED,
+		.extra1		= &unsigned_min,
+		.extra2		= &unsigned_max,
+	},
 	{ }
 };
 
tools/testing/selftests/sysctl/sysctl.sh +52 -0

70 lines not on this page · raw message

Signature · 1 line
1.8.3.1
#15 lore raw

PATCH v5 7/9 test_sysctl: Add ctl_table registration failure test

In reply to Waiman Long

Incorrect sysctl tables are constructed and fed to the register_sysctl_table() function in the test_sysctl kernel module. The function is supposed to fail the registration of those tables or an error will be printed if no failure is returned.

The registration failures will cause other warning and error messages to be printed into the dmesg log, though.

A new test is also added to the sysctl.sh to look for those failure messages in the dmesg log to see if anything unexpeced happens.

---
 lib/test_sysctl.c                        | 41 ++++++++++++++++++++++++++++++++
 tools/testing/selftests/sysctl/sysctl.sh | 15 ++++++++++++
 2 files changed, 56 insertions(+)
lib/test_sysctl.c +41 -0
index 7bb4cf7..14853d5 100644
@@ -154,13 +154,54 @@ struct test_sysctl_data {
 	{ }
 };
 
+static struct ctl_table fail_sysctl_table0[] = {
+	{
+		.procname	= "failed_sysctl0",
+		.data		= &test_data.range_0001,
+		.maxlen		= sizeof(test_data.range_0001),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.flags		= CTL_FLAGS_CLAMP_RANGE_SIGNED,
+		.extra1		= &signed_max,
+		.extra2		= &signed_min,
+	},
+	{ }
+};
+
+static struct ctl_table fail_sysctl_root_table[] = {
+	{
+		.procname	= "debug",
+		.maxlen		= 0,
+		.mode		= 0555,
+	},
+	{ }
+};
+
+static struct ctl_table *fail_tables[] = {
+	fail_sysctl_table0, NULL,
+};
+
 static struct ctl_table_header *test_sysctl_header;
 
 static int __init test_sysctl_init(void)
 {
+	struct ctl_table_header *fail_sysctl_header;
+	int i;
+
 	test_sysctl_header = register_sysctl_table(test_sysctl_root_table);
 	if (!test_sysctl_header)
 		return -ENOMEM;
+
+	for (i = 0; fail_tables[i]; i++) {
+		fail_sysctl_root_table[0].child = fail_tables[i];
+		fail_sysctl_header = register_sysctl_table(fail_sysctl_root_table);
+		if (fail_sysctl_header) {
+			pr_err("fail_tables[%d] registration check failed!\n", i);
+			unregister_sysctl_table(fail_sysctl_header);
+			break;
+		}
+	}
+
 	return 0;
 }
 late_initcall(test_sysctl_init);
tools/testing/selftests/sysctl/sysctl.sh +15 -0
index 1aa1bba..23acdee 100755
@@ -35,6 +35,7 @@ ALL_TESTS="$ALL_TESTS 0003:1:1"
 ALL_TESTS="$ALL_TESTS 0004:1:1"
 ALL_TESTS="$ALL_TESTS 0005:3:1"
 ALL_TESTS="$ALL_TESTS 0006:1:1"
+ALL_TESTS="$ALL_TESTS 0007:1:1"
 
 test_modprobe()
 {
@@ -652,6 +653,20 @@ sysctl_test_0006()
 	set_orig
 }
 
+sysctl_test_0007()
+{
+	echo "Checking test_sysctl module registration failure test ..."
+	dmesg | grep "sysctl.*fail_tables.*failed"
+	if [[ $? -eq 0 ]]; then
+		echo "FAIL" >&2
+		rc=1
+	else
+		echo "ok"
+	fi
+
+	test_rc
+}
+
 list_tests()
 {
 	echo "Test ID list:"
Signature · 1 line
1.8.3.1
#16 lore raw

PATCH v5 8/9 ipc: Allow boot time extension of IPCMNI from 32k to 2M

In reply to Waiman Long

The maximum number of unique System V IPC identifiers was limited to 32k. That limit should be big enough for most use cases.

However, there are some users out there requesting for more. To satisfy the need of those users, a new boot time kernel option "ipcmni_extend" is added to extend the IPCMNI value to 2M. This is a 64X increase which hopefully is big enough for them.

This new option does have the side effect of reducing the maximum number of unique sequence numbers from 64k down to 1k. So it is a trade-off.

---
 Documentation/admin-guide/kernel-parameters.txt |  3 +++
 include/linux/ipc.h                             | 11 ++++++++++-
 ipc/ipc_sysctl.c                                | 12 +++++++++++-
 ipc/util.c                                      | 12 ++++++------
 ipc/util.h                                      | 18 +++++++++++-------
 5 files changed, 41 insertions(+), 15 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +3 -0
index 1d1d53f..2be35a4 100644
@@ -1733,6 +1733,9 @@
 	ip=		[IP_PNP]
 			See Documentation/filesystems/nfs/nfsroot.txt.
 
+	ipcmni_extend	[KNL] Extend the maximum number of unique System V
+			IPC identifiers from 32768 to 2097152.
+
 	irqaffinity=	[SMP] Set the default irq affinity mask
 			The argument is a cpu list, as described above.
 
include/linux/ipc.h +10 -1
index 821b2f2..3ecd869 100644
@@ -8,7 +8,16 @@
 #include <uapi/linux/ipc.h>
 #include <linux/refcount.h>
 
-#define IPCMNI 32768  /* <= MAX_INT limit for ipc arrays (including sysctl changes) */
+/*
+ * By default, the ipc arrays can have up to 32k (15 bits) entries.
+ * When IPCMNI extension mode is turned on, the ipc arrays can have up
+ * to 2M (21 bits) entries. However, the space for sequence number will
+ * be shrunk from 16 bits to 10 bits.
+ */
+#define IPCMNI_SHIFT		15
+#define IPCMNI_EXTEND_SHIFT	21
+#define IPCMNI			(1 << IPCMNI_SHIFT)
+#define IPCMNI_EXTEND		(1 << IPCMNI_EXTEND_SHIFT)
 
 /* used by in-kernel data structures */
 struct kern_ipc_perm {
ipc/ipc_sysctl.c +11 -1
index 0ad7088..5f7cfae 100644
@@ -109,7 +109,8 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 static int zero;
 static int one = 1;
 static int int_max = INT_MAX;
-static int ipc_mni = IPCMNI;
+int ipc_mni __read_mostly = IPCMNI;
+int ipc_mni_shift __read_mostly = IPCMNI_SHIFT;
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -237,3 +238,12 @@ static int __init ipc_sysctl_init(void)
 }
 
 device_initcall(ipc_sysctl_init);
+
+static int __init ipc_mni_extend(char *str)
+{
+	ipc_mni = IPCMNI_EXTEND;
+	ipc_mni_shift = IPCMNI_EXTEND_SHIFT;
+	pr_info("IPCMNI extended to %d.\n", ipc_mni);
+	return 0;
+}
+early_param("ipcmni_extend", ipc_mni_extend);
ipc/util.c +6 -6
index 4ed5a17..daee305 100644
@@ -112,7 +112,7 @@ static int __init ipc_init(void)
  * @ids: ipc identifier set
  *
  * Set up the sequence range to use for the ipc identifier range (limited
- * below IPCMNI) then initialise the keys hashtable and ids idr.
+ * below ipc_mni) then initialise the keys hashtable and ids idr.
  */
 int ipc_init_ids(struct ipc_ids *ids)
 {
@@ -213,7 +213,7 @@ static inline int ipc_buildid(int id, struct ipc_ids *ids,
 		ids->next_id = -1;
 	}
 
-	return SEQ_MULTIPLIER * new->seq + id;
+	return (new->seq << SEQ_SHIFT) + id;
 }
 
 #else
@@ -227,7 +227,7 @@ static inline int ipc_buildid(int id, struct ipc_ids *ids,
 	if (ids->seq > IPCID_SEQ_MAX)
 		ids->seq = 0;
 
-	return SEQ_MULTIPLIER * new->seq + id;
+	return (new->seq << SEQ_SHIFT) + id;
 }
 
 #endif /* CONFIG_CHECKPOINT_RESTORE */
@@ -251,8 +251,8 @@ 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 (limit > ipc_mni)
+		limit = ipc_mni;
 
 	if (!ids->tables_initialized || ids->in_use >= limit)
 		return -ENOSPC;
@@ -769,7 +769,7 @@ static struct kern_ipc_perm *sysvipc_find_ipc(struct ipc_ids *ids, loff_t pos,
 	if (total >= ids->in_use)
 		return NULL;
 
-	for (; pos < IPCMNI; pos++) {
+	for (; pos < ipc_mni; pos++) {
 		ipc = idr_find(&ids->ipcs_idr, pos);
 		if (ipc != NULL) {
 			*new_pos = pos + 1;
ipc/util.h +11 -7
index af57394..6871ca9 100644
@@ -15,7 +15,11 @@
 #include <linux/err.h>
 #include <linux/ipc_namespace.h>
 
-#define SEQ_MULTIPLIER	(IPCMNI)
+extern int ipc_mni;
+extern int ipc_mni_shift;
+
+#define SEQ_SHIFT	ipc_mni_shift
+#define SEQ_MASK	((1 << ipc_mni_shift) - 1)
 
 int sem_init(void);
 int msg_init(void);
@@ -93,9 +97,9 @@ 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)
+#define ipcid_to_idx(id)  ((id) & SEQ_MASK)
+#define ipcid_to_seqx(id) ((id) >> SEQ_SHIFT)
+#define IPCID_SEQ_MAX	  (INT_MAX >> SEQ_SHIFT)
 
 /* must be called with ids->rwsem acquired for writing */
 int ipc_addid(struct ipc_ids *, struct kern_ipc_perm *, int);
@@ -120,8 +124,8 @@ 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;
+	if (ids->in_use == ipc_mni)
+		return ipc_mni - 1;
 
 	return ids->max_id;
 }
@@ -163,7 +167,7 @@ struct kern_ipc_perm *ipcctl_pre_down_nolock(struct ipc_namespace *ns,
 
 static inline int ipc_checkid(struct kern_ipc_perm *ipcp, int uid)
 {
-	return uid / SEQ_MULTIPLIER != ipcp->seq;
+	return (uid >> SEQ_SHIFT) != ipcp->seq;
 }
 
 static inline void ipc_lock_object(struct kern_ipc_perm *perm)
Signature · 1 line
1.8.3.1
#17 lore raw

PATCH v5 9/9 ipc: Conserve sequence numbers in extended IPCMNI mode

In reply to Waiman Long

The mixing in of a sequence number into the IPC IDs is probably to avoid ID reuse in userspace as much as possible. With extended IPCMNI mode, the number of usable sequecne numbers is greatly reduced leading to higher chance of ID reuse.

To address this issue, we need to conserve the sequence number space as much as possible. Right now, the sequence number is incremented for every new ID created. In reality, we only need to increment the sequence number when one or more IDs have been removed previously to make sure that those IDs will not be reused when a new one is built. This is being done in the extended IPCMNI mode,

---
 include/linux/ipc_namespace.h |  1 +
 ipc/ipc_sysctl.c              |  2 ++
 ipc/util.c                    | 29 ++++++++++++++++++++++-------
 ipc/util.h                    |  1 +
 4 files changed, 26 insertions(+), 7 deletions(-)
include/linux/ipc_namespace.h +1 -0
index b5630c8..9c86fd9 100644
@@ -16,6 +16,7 @@
 struct ipc_ids {
 	int in_use;
 	unsigned short seq;
+	unsigned short deleted;
 	bool tables_initialized;
 	struct rw_semaphore rwsem;
 	struct idr ipcs_idr;
ipc/ipc_sysctl.c +2 -0
index 5f7cfae..61a832d 100644
@@ -111,6 +111,7 @@ static int proc_ipc_sem_dointvec(struct ctl_table *table, int write,
 static int int_max = INT_MAX;
 int ipc_mni __read_mostly = IPCMNI;
 int ipc_mni_shift __read_mostly = IPCMNI_SHIFT;
+bool ipc_mni_extended __read_mostly;
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -243,6 +244,7 @@ static int __init ipc_mni_extend(char *str)
 {
 	ipc_mni = IPCMNI_EXTEND;
 	ipc_mni_shift = IPCMNI_EXTEND_SHIFT;
+	ipc_mni_extended = true;
 	pr_info("IPCMNI extended to %d.\n", ipc_mni);
 	return 0;
 }
ipc/util.c +22 -7
index daee305..8b38a6f 100644
@@ -118,7 +118,8 @@ int ipc_init_ids(struct ipc_ids *ids)
 {
 	int err;
 	ids->in_use = 0;
-	ids->seq = 0;
+	ids->deleted = false;
+	ids->seq = ipc_mni_extended ? 0 : -1; /* seq # is pre-incremented */
 	init_rwsem(&ids->rwsem);
 	err = rhashtable_init(&ids->key_ht, &ipc_kht_params);
 	if (err)
@@ -192,6 +193,11 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
 	return NULL;
 }
 
+/*
+ * To conserve sequence number space with extended ipc_mni when new ID
+ * is built, the sequence number is incremented only when one or more
+ * IDs have been removed previously.
+ */
 #ifdef CONFIG_CHECKPOINT_RESTORE
 /*
  * Specify desired id for next allocated IPC object.
@@ -205,9 +211,13 @@ static inline int ipc_buildid(int id, 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;
+		if (!ipc_mni_extended || ids->deleted) {
+			ids->seq++;
+			if (ids->seq > IPCID_SEQ_MAX)
+				ids->seq = 0;
+			ids->deleted = false;
+		}
+		new->seq = ids->seq;
 	} else {
 		new->seq = ipcid_to_seqx(ids->next_id);
 		ids->next_id = -1;
@@ -223,9 +233,13 @@ static inline int ipc_buildid(int id, struct ipc_ids *ids,
 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;
+	if (!ipc_mni_extended || ids->deleted) {
+		ids->seq++;
+		if (ids->seq > IPCID_SEQ_MAX)
+			ids->seq = 0;
+		ids->deleted = false;
+	}
+	new->seq = ids->seq;
 
 	return (new->seq << SEQ_SHIFT) + id;
 }
@@ -435,6 +449,7 @@ void ipc_rmid(struct ipc_ids *ids, struct kern_ipc_perm *ipcp)
 	idr_remove(&ids->ipcs_idr, lid);
 	ipc_kht_remove(ids, ipcp);
 	ids->in_use--;
+	ids->deleted = true;
 	ipcp->deleted = true;
 
 	if (unlikely(lid == ids->max_id)) {
ipc/util.h +1 -0
index 6871ca9..e6c2055 100644
@@ -17,6 +17,7 @@
 
 extern int ipc_mni;
 extern int ipc_mni_shift;
+extern bool ipc_mni_extended;
 
 #define SEQ_SHIFT	ipc_mni_shift
 #define SEQ_MASK	((1 << ipc_mni_shift) - 1)
Signature · 1 line
1.8.3.1
#18 lore raw
In reply to Waiman Long

Andrew,

please drop these patches until further notice. I would recommend we avoid merging these patches until we get proper Acked-by for the entire set. Waiman has a bit more work to do and even after the 5th iteration this is not right yet.

Luis

#19 lore raw
Quoted 5 lines · Luis R. Rodriguez

Andrew,

please drop these patches until further notice. I would recommend we avoid merging these patches until we get proper Acked-by for the entire set. Waiman has a bit more work to do and even after the 5th iteration this is not right yet.

Sorry or the noise, I see they are not merged.

Luis