Re: [PATCH] ipc/sem: account enlarged sops arrays to memcg

[Andrew Morton <akpm@linux-foundation.org>]

(Alexey, Eric, others: please weigh in)

On Fri, 16 Oct 2026 15:26:11 +0000 Wang Yingjie <x-professor@outlook.com> wrote:

> Commit 50ec499b9a43 ("sysctl: allow change system v ipc sysctls
> inside ipc namespace") lets an IPC namespace owner change kernel.sem.
> The SEMOPM field defaults to 500 but has no semantic range check.
> 
> do_semtimedop() allocates nsops * sizeof(struct sembuf) with
> GFP_KERNEL. A blocking operation retains that buffer, so a user can
> raise SEMOPM and hold large attacker-selected amounts of host kernel
> memory without charging the calling task memcg.
> 
> On the current mainline base, one 10,000,000-operation call retained
> 60,936,448 bytes while a 32 MiB cgroup peaked at 909,312 bytes and
> reported no limit event. Sixteen 1,000,000-operation calls retained
> 100,860,416 bytes while the cgroup peaked at 1,310,720 bytes.
> 
> Commit 6a4746ba0619 ("ipc: remove memcg accounting for sops objects in
> do_semtimedop()") removed accounting on the premise that the buffer was
> at most two pages. Preserve that default-sized path, but account arrays
> larger than the default SEMOPM value.

OK, I think. But...

> With this change, both test configurations reach the 32 MiB cgroup
> limit and the caller is terminated by the memcg OOM killer before it
> can retain the enlarged arrays.

Only if memcg is enabled and the task's memory cgroup has an effective
limit.  Otherwise we can end up handing out idiotic amounts of kernel
memory.  Memcg accounting isn't a substitute for limiting the
allocation size.

> The sysctl also accepts negative SEMOPM values. Since nsops is unsigned,
> SEMOPM=-1 bypasses the intended operation-count check.

Let's fix this first. nsops is unsigned int, whereas ns->sc_semopm is
int. In the comparison nsops > ns->sc_semopm, a negative SEMOPM is
converted to unsigned, so SEMOPM=-1 effectively sets the limit to
UINT_MAX.

I'm thinking:

- Permit init_ipc_ns.sc_semopm to be set to any value from 1 to INT_MAX.
  This preserves the existing ability of host root to configure larger
  limits.

- For all other IPC namespaces, permit values from 1 to 1000. The
  documentation has long recommended 1000 as an upper limit, although
  it hasn't been enforced. This would be a new restriction, so the
  changelog should explain why we're introducing it.

- Validate the new values before installing any of them.  The current
  proc_ipc_sem_dointvec() updates the live array directly, and
  rejecting an invalid value afterward would leave a window in which
  other tasks could observe it.

- Update the comment in include/uapi/linux/sem.h. The warning about
  physical memory fragmentation is outdated now that the allocation
  uses kvmalloc(). Document the new 1000 limit for non-initial IPC
  namespaces instead.

Ideally, a new IPC namespace would inherit its parent's configured
limit, but currently every new namespace starts at SEMOPM=500. I don't
think we need to change that as part of this fix.

Then please redo your memcg-accounting patch as a separate followup,
with suitably adjusted changelogging. That would provide additional
protection for legitimately enlarged allocations in the initial IPC
namespace and for allocations between 501 and 1000 operations elsewhere.

Thanks.