[PATCH v5 0/3] ipc/sem: account dynamic sops arrays and validate SEMOPM

[PATCH v5 0/3] ipc/sem: account dynamic sops arrays and validate SEMOPM

[Yingjie Wang]

Shakeel, thanks for the suggestion.

Patch 1 now uses GFP_KERNEL_ACCOUNT for every dynamically allocated sops
array. The first 64 operations still use the existing stack buffer. The
fixed 1000-operation exemption and its conditional allocation flags have
been removed.

I compared this version with v4 in a 4-vCPU KVM guest. The benchmark process
was pinned to one vCPU. Each number is the median of 21 samples from three
fresh boots of each kernel. In a leaf memory cgroup, the changes were:

                                      65       500      1000
  successful semop()                   +3.2%     +3.2%     +2.3%
  allocation-focused invalid semid     +5.6%     +9.1%    +11.9%

The invalid-sem-id case allocates and copies the operation array before
returning EINVAL, so it emphasizes allocation overhead. The 64-operation
stack controls changed by 0.9% and 0.5%, respectively. The already-accounted
1001-operation controls changed by 1.0% and -0.2%. Successful calls in the
root cgroup changed by at most 2.3%, including the controls.

The sysctl handler parses writes into a temporary array while holding a
per-IPC-namespace mutex. The mutex covers the snapshot, validation, and
publication, so a failed write leaves the live settings unchanged and a
partial write cannot restore fields from an older snapshot. The snapshot and
publication use READ_ONCE() and WRITE_ONCE(), and reads continue through
proc_dointvec(). The unused sem_check_semmni() helper is also removed.

The UAPI warning about physical fragmentation is obsolete now that the
operation array uses kvmalloc(). The header describes SEMOPM as the
compiled-in default and retains the uniform <= INT_MAX range comment.

Patches 2 and 3 are unchanged from v4 and retain Manfred's Reviewed-by tags.
The series is based on 7be4bc52fd9e ("Merge tag 'driver-core-7.4-rc1' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core").

Validation completed on v5:

- a W=1 full x86-64 bzImage build;
- W=1 i386 builds of ipc/sem.o and ipc/ipc_sysctl.o;
- three paired boots of v4 and v5 for the root/leaf memcg benchmark, with
  seven samples for every path, cgroup, and operation count in each boot;
- initial and user-owned non-initial IPC namespaces rejected negative SEMOPM,
  invalid SEMMNI, and malformed writes without changing live settings;
- zero and positive SEMOPM values remained valid, including the 1000/1001
  operation boundary;
- concurrent partial, full, rejected, and successful sysctl writes completed
  without stale-snapshot overwrites;
- successful, EAGAIN, EFAULT, signal-interruption, and IPC_RMID paths released
  operation-array allocations;
- no BUG, WARNING, KASAN, UBSAN, or lock dependency report appeared;
- strict checkpatch with the intentional CONST_STRUCT diagnostic ignored,
  and patch application checks passed.

Changes since v4:

- account every dynamically allocated sops array with GFP_KERNEL_ACCOUNT, as
  Shakeel suggested;
- remove the separate 1000-operation accounting threshold;
- add root and leaf memory-cgroup benchmark results for 64, 65, 500, 1000,
  and 1001 operations.

V4:
https://lore.kernel.org/r/cover.1793207025.git.x-professor@outlook.com

Review of v4 1/3 by Shakeel:
https://lore.kernel.org/r/565d855cbd81e0f49018da21df00ff0308f646b1.1793207025.git.x-professor@outlook.com

Yingjie Wang (3):
  ipc/sem: account dynamically allocated sops arrays to memcg
  ipc/sem: reject negative SEMOPM values
  ipc/sem: remove obsolete SEMOPM fragmentation warning

 include/linux/ipc_namespace.h |  3 +++
 include/uapi/linux/sem.h      |  7 +++----
 ipc/ipc_sysctl.c              | 35 ++++++++++++++++++++++++++---------
 ipc/sem.c                     |  3 ++-
 ipc/util.h                    |  9 ---------
 5 files changed, 34 insertions(+), 23 deletions(-)


base-commit: 7be4bc52fd9e0e55dc9a2e0232b89427d2c4b614
-- 
2.43.0

[PATCH v5 1/3] ipc/sem: account dynamically allocated sops arrays to memcg

[Yingjie Wang]

Commit 6a4746ba0619 ("ipc: remove memcg accounting for sops objects in
do_semtimedop()") removed accounting based on the assumption that sops
arrays would remain small, describing a maximum cost of two pages.

SEMOPM is a UAPI default that can be changed at build time and through
the per-namespace sysctl. Commit 50ec499b9a43 ("sysctl: allow change
system v ipc sysctls inside ipc namespace") made that limit tunable by
IPC namespace owners without capabilities in the initial user namespace.

do_semtimedop() keeps up to SEMOPM_FAST operations on the stack. Charge
every larger, dynamically allocated operation array to the caller's memory
cgroup. This avoids a second size threshold and prevents a configured
SEMOPM limit from expanding an unaccounted heap-allocation range.

In a 4-vCPU KVM guest, with the benchmark pinned to one vCPU in a leaf
memory cgroup, accounting raised the median time per successful semop()
call by 2.3% to 3.2% for 65, 500 and 1000 operations, compared with
unaccounted allocation. An invalid semid, which allocates and copies the
array before returning EINVAL, rose by 5.6% to 11.9%. The 64-operation
stack path changed by at most 0.9%, and successful calls in the root
cgroup by at most 2.3%. Each result used 21 samples from three boots.

Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
Link: https://lore.kernel.org/r/20261016152609.2528805-1-x-professor@outlook.com
Fixes: 6a4746ba0619 ("ipc: remove memcg accounting for sops objects in do_semtimedop()")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 ipc/sem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 5ec41de7e85b2..ea95cca5bb0ec 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -2234,7 +2234,7 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
 		return -EINVAL;
 
 	if (nsops > SEMOPM_FAST) {
-		sops = kvmalloc_objs(*sops, nsops);
+		sops = kvmalloc_objs(*sops, nsops, GFP_KERNEL_ACCOUNT);
 		if (sops == NULL)
 			return -ENOMEM;
 	}
-- 
2.43.0

[PATCH v5 2/3] ipc/sem: reject negative SEMOPM values

[Yingjie Wang]

The SEMOPM field has been stored as an int while semop() receives an
unsigned operation count since the beginning of Git history. A negative
SEMOPM accepted by proc_dointvec() is converted to a large unsigned value
in the comparison, defeating the intended operation-count limit.

Commit 50ec499b9a43 ("sysctl: allow change system v ipc sysctls inside ipc
namespace") made kernel.sem writable by IPC namespace owners without
capabilities in the initial user namespace.

Parse writes against a temporary copy and validate SEMOPM and SEMMNI
before changing the live settings. Hold a per-namespace mutex from the
snapshot through publication so concurrent sysctl writers cannot publish
stale values for fields they did not supply. Access the live fields with
READ_ONCE() and WRITE_ONCE(), preserving the per-field access semantics of
proc_dointvec(). Read requests continue to use proc_dointvec() directly.

Failed writes leave the live settings unchanged. Successful partial writes
change only the supplied fields relative to the latest locked snapshot.
The writer mutex retains the existing per-field visibility for readers.

Keep zero and positive SEMOPM values valid. Zero continues to reject every
non-empty semop() request.

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Link: https://lore.kernel.org/r/20261016155905.2a659b7bb7d61f33467a3263@linux-foundation.org
Link: https://lore.kernel.org/r/51c93c19-f1dd-455f-8698-6b1ee30eb630@colorfullife.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Reviewed-by: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 include/linux/ipc_namespace.h |  3 +++
 ipc/ipc_sysctl.c              | 35 ++++++++++++++++++++++++++---------
 ipc/sem.c                     |  1 +
 ipc/util.h                    |  9 ---------
 4 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index 12faca29bbb9c..c06568ccfca88 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -4,6 +4,7 @@
 
 #include <linux/err.h>
 #include <linux/idr.h>
+#include <linux/mutex.h>
 #include <linux/rwsem.h>
 #include <linux/notifier.h>
 #include <linux/nsproxy.h>
@@ -31,6 +32,8 @@ struct ipc_ids {
 struct ipc_namespace {
 	struct ipc_ids	ids[3];
 
+	/* Serialize kernel.sem writers. */
+	struct mutex	sem_ctls_mutex;
 	int		sem_ctls[4];
 	int		used_sems;
 
diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index d038d944257f6..0e3608258fe29 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -51,21 +51,38 @@ static int proc_ipc_auto_msgmni(const struct ctl_table *table, int write,
 static int proc_ipc_sem_dointvec(const struct ctl_table *table, int write,
 	void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct ctl_table ipc_table;
 	struct ipc_namespace *ns =
 		container_of(table->data, struct ipc_namespace, sem_ctls);
-	int ret, semmni;
+	int sem_ctls[ARRAY_SIZE(ns->sem_ctls)];
+	size_t i;
+	int ret;
+
+	if (!write)
+		return proc_dointvec(table, write, buffer, lenp, ppos);
 
-	semmni = ns->sem_ctls[3];
-	ret = proc_dointvec(table, write, buffer, lenp, ppos);
+	mutex_lock(&ns->sem_ctls_mutex);
+	for (i = 0; i < ARRAY_SIZE(sem_ctls); i++)
+		sem_ctls[i] = READ_ONCE(ns->sem_ctls[i]);
 
-	if (!ret)
-		ret = sem_check_semmni(ns);
+	ipc_table = *table;
+	ipc_table.data = sem_ctls;
 
-	/*
-	 * Reset the semmni value if an error happens.
-	 */
+	ret = proc_dointvec(&ipc_table, write, buffer, lenp, ppos);
 	if (ret)
-		ns->sem_ctls[3] = semmni;
+		goto out_unlock;
+
+	if (sem_ctls[2] < 0 ||
+	    sem_ctls[3] < 0 || sem_ctls[3] > ipc_mni) {
+		ret = -ERANGE;
+		goto out_unlock;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(sem_ctls); i++)
+		WRITE_ONCE(ns->sem_ctls[i], sem_ctls[i]);
+
+out_unlock:
+	mutex_unlock(&ns->sem_ctls_mutex);
 	return ret;
 }
 
diff --git a/ipc/sem.c b/ipc/sem.c
index ea95cca5bb0ec..098e483ae8d3c 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -248,6 +248,7 @@ static int sysvipc_sem_proc_show(struct seq_file *s, void *it);
 
 void sem_init_ns(struct ipc_namespace *ns)
 {
+	mutex_init(&ns->sem_ctls_mutex);
 	ns->sc_semmsl = SEMMSL;
 	ns->sc_semmns = SEMMNS;
 	ns->sc_semopm = SEMOPM;
diff --git a/ipc/util.h b/ipc/util.h
index a55d6cebe6d3b..51b4efa3d6db0 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -245,15 +245,6 @@ 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 *));
 
-static inline int sem_check_semmni(struct ipc_namespace *ns) {
-	/*
-	 * Check semmni range [0, ipc_mni]
-	 * semmni is the last element of sem_ctls[4] array
-	 */
-	return ((ns->sem_ctls[3] < 0) || (ns->sem_ctls[3] > ipc_mni))
-		? -ERANGE : 0;
-}
-
 #ifdef CONFIG_COMPAT
 #include <linux/compat.h>
 struct compat_ipc_perm {
-- 
2.43.0

[PATCH v5 3/3] ipc/sem: remove obsolete SEMOPM fragmentation warning

[Yingjie Wang]

The UAPI comment advises keeping SEMOPM at or below 1000 because a larger
operation array may fail due to physical memory fragmentation.

Commit e4243b8062c1 ("ipc/sem: play nicer with large nsops allocations")
changed the operation array to kvmalloc() specifically to support larger
configured limits, with vmalloc fallback when a physically contiguous
allocation is unavailable.

Replace the obsolete warning with a description of the compiled-in
default and the runtime sysctl.

Reviewed-by: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 include/uapi/linux/sem.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/sem.h b/include/uapi/linux/sem.h
index 75aa3b273cd99..d9f961a1592c7 100644
--- a/include/uapi/linux/sem.h
+++ b/include/uapi/linux/sem.h
@@ -71,16 +71,15 @@ struct  seminfo {
  * The values has been chosen to be larger than necessary for any
  * known configuration.
  *
- * SEMOPM should not be increased beyond 1000, otherwise there is the
- * risk that semop()/semtimedop() fails due to kernel memory fragmentation when
- * allocating the sop array.
+ * SEMOPM is the default maximum number of operations per semop() call. The
+ * runtime limit can be changed through /proc/sys/kernel/sem.
  */
 
 
 #define SEMMNI  32000           /* <= IPCMNI  max # of semaphore identifiers */
 #define SEMMSL  32000           /* <= INT_MAX max num of semaphores per id */
 #define SEMMNS  (SEMMNI*SEMMSL) /* <= INT_MAX max # of semaphores in system */
-#define SEMOPM  500	        /* <= 1 000 max num of ops per semop call */
+#define SEMOPM  500	        /* <= INT_MAX max num of ops per semop call */
 #define SEMVMX  32767           /* <= 32767 semaphore maximum value */
 #define SEMAEM  SEMVMX          /* adjust on exit max value */
 
-- 
2.43.0