Dear friends,

this is just a rebase/small rework of original Christian Brauner's series
from:
https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=pid_max_namespacing

Christian kindly allowed me to take these patches and resend after small modifications.

Current tree:
https://github.com/mihalicyn/linux/commits/pid_max_namespacing

Changelog for version 2:
- rebased from 6.7 to 6.12

Changelog (from original Christian's patches):
 - rebased from 5.14-rc1
 - a few fixes (missing ns_free_inum on error path, missing initialization, etc)
 - permission check changes in pid_table_root_permissions
 - unsigned int pid_max -> int pid_max (keep pid_max type as it was)
 - add READ_ONCE in alloc_pid() as suggested by Christian
[ it is here https://lore.kernel.org/lkml/20240222160915.315255-1-aleksandr.mikhalitsyn@canonical.com/#t ]

Original description from Christian:

The pid_max sysctl is a global value. For a long time the default value
has been 65535 and during the pidfd dicussions Linus proposed to bump
pid_max by default (cf. [1]). Based on this discussion systemd started
bumping pid_max to 2^22. So all new systems now run with a very high
pid_max limit with some distros having also backported that change.
The decision to bump pid_max is obviously correct. It just doesn't make
a lot of sense nowadays to enforce such a low pid number. There's
sufficient tooling to make selecting specific processes without typing
really large pid numbers available.

In any case, there are workloads that have expections about how large
pid numbers they accept. Either for historical reasons or architectural
reasons. One concreate example is the 32-bit version of Android's bionic
libc which requires pid numbers less than 65536. There are workloads
where it is run in a 32-bit container on a 64-bit kernel. If the host
has a pid_max value greater than 65535 the libc will abort thread
creation because of size assumptions of pthread_mutex_t.

That's a fairly specific use-case however, in general specific workloads
that are moved into containers running on a host with a new kernel and a
new systemd can run into issues with large pid_max values. Obviously
making assumptions about the size of the allocated pid is suboptimal but
we have userspace that does it.

Of course, giving containers the ability to restrict the number of
processes in their respective pid namespace indepent of the global limit
through pid_max is something desirable in itself and comes in handy in
general.

Independent of motivating use-cases the existence of pid namespaces
makes this also a good semantical extension and there have been prior
proposals pushing in a similar direction.
The trick here is to minimize the risk of regressions which I think is
doable. The fact that pid namespaces are hierarchical will help us here.

What we mostly care about is that when the host sets a low pid_max
limit, say (crazy number) 100 that no descendant pid namespace can
allocate a higher pid number in its namespace. Since pid allocation is
hierarchial this can be ensured by checking each pid allocation against
the pid namespace's pid_max limit. This means if the allocation in the
descendant pid namespace succeeds, the ancestor pid namespace can reject
it. If the ancestor pid namespace has a higher limit than the descendant
pid namespace the descendant pid namespace will reject the pid
allocation. The ancestor pid namespace will obviously not care about
this.
All in all this means pid_max continues to enforce a system wide limit
on the number of processes but allows pid namespaces sufficient leeway
in handling workloads with assumptions about pid values and allows
containers to restrict the number of processes in a pid namespace
through the pid_max interface.

Signed-off-by: Christian Brauner <brauner@kernel.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>

Christian Brauner (2):
  pid: allow pid_max to be set per pid namespace
  tests/pid_namespace: add pid_max tests

 include/linux/pid.h                           |   3 -
 include/linux/pid_namespace.h                 |  10 +-
 kernel/pid.c                                  | 125 +++++-
 kernel/pid_namespace.c                        |  43 ++-
 kernel/sysctl.c                               |   9 -
 kernel/trace/pid_list.c                       |   2 +-
 kernel/trace/trace.h                          |   2 -
 kernel/trace/trace_sched_switch.c             |   2 +-
 .../selftests/pid_namespace/.gitignore        |   1 +
 .../testing/selftests/pid_namespace/Makefile  |   2 +-
 .../testing/selftests/pid_namespace/pid_max.c | 358 ++++++++++++++++++
 11 files changed, 521 insertions(+), 36 deletions(-)
 create mode 100644 tools/testing/selftests/pid_namespace/pid_max.c

-- 
2.43.0

Hello.

On Fri, Nov 22, 2024 at 02:24:57PM +0100, Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote:
>
(Sorry for responding only now as I missed this until I read v6.14 news.)

> The pid_max sysctl is a global value. For a long time the default value
> has been 65535 and during the pidfd dicussions Linus proposed to bump
> pid_max by default (cf. [1]). Based on this discussion systemd started
> bumping pid_max to 2^22. So all new systems now run with a very high
> pid_max limit with some distros having also backported that change.

Yes, multiple [1] people [2] proposed even lifting the legacy limit in
kernel directly.

> Of course, giving containers the ability to restrict the number of
> processes in their respective pid namespace indepent of the global limit
> through pid_max is something desirable in itself and comes in handy in
> general.

Yes, this is what pids.max of a cgroup (already) does.

(It is already difficult for users to troubleshoot which of multiple pid
limits restricts their workload. I'm afraid making pid_max
per-(hierarchical-)NS will contribute to confusion.)
Also, the implementation copies the limit upon creation from
parent, this pattern showed cumbersome with some attributes in legacy
cgroup controllers e.g.  it's subject to race condition between parent's
limit modification and children creation.

> Independent of motivating use-cases the existence of pid namespaces
> makes this also a good semantical extension and there have been prior
> proposals pushing in a similar direction.
> The trick here is to minimize the risk of regressions which I think is
> doable. The fact that pid namespaces are hierarchical will help us here.

I understand it is tempting to make pid_max part of a pid namespace but
was the overlap with pids controller considered?

I'd consider the alternative of relying of virtualized PID numbers in
pid namespaces with appropriate pids.max limit and numbers allocation
strategy that would keep PID values below the limit (i.e. taking the
first free pid number in given NS, actually I thought it is already the
case but it doesn't work like that (when I try now [3])).
WDYT?

TL;DR instead of getting rid of the legacy limit, it was further
extended to pid namespaces because of legacy workloads and it (almost)
duplicates existing mechanism. Can this be rethought please?

Thanks,
Michal

[1] https://lore.kernel.org/all/20240408145819.8787-1-mkoutny@suse.com/
[2] https://lore.kernel.org/linux-api/CAHk-=wiZ40LVjnXSi9iHLE_-ZBsWFGCgdmNiYZUXn1-V5YBg2g@mail.gmail.com/

On Thu, Jan 30, 2025 at 6:45 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Hello.

Dear Michal,

(responding in this thread too, as a part of discussion around [1] revert)

[1] https://lore.kernel.org/all/20250221170249.890014-1-mkoutny@suse.com/

>
> On Fri, Nov 22, 2024 at 02:24:57PM +0100, Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote:
> >
> (Sorry for responding only now as I missed this until I read v6.14 news.)
>
> > The pid_max sysctl is a global value. For a long time the default value
> > has been 65535 and during the pidfd dicussions Linus proposed to bump
> > pid_max by default (cf. [1]). Based on this discussion systemd started
> > bumping pid_max to 2^22. So all new systems now run with a very high
> > pid_max limit with some distros having also backported that change.
>
> Yes, multiple [1] people [2] proposed even lifting the legacy limit in
> kernel directly.
>
> > Of course, giving containers the ability to restrict the number of
> > processes in their respective pid namespace indepent of the global limit
> > through pid_max is something desirable in itself and comes in handy in
> > general.
>
> Yes, this is what pids.max of a cgroup (already) does.

Not precisely, as it only limits the number of tasks in the cgroup,
while we talk
about pid *value* limit.

>
> (It is already difficult for users to troubleshoot which of multiple pid
> limits restricts their workload. I'm afraid making pid_max
> per-(hierarchical-)NS will contribute to confusion.)
> Also, the implementation copies the limit upon creation from
> parent, this pattern showed cumbersome with some attributes in legacy
> cgroup controllers e.g.  it's subject to race condition between parent's
> limit modification and children creation.

yeah, but it was intentional not to make this kernel change too big
and complex (and probably slow down things too).
Let's be honest that this pid_max setting is that kind of thing that
is rarely changed
and people use cgroups nowadays for that kind of stuff (and it is good!).

>
> > Independent of motivating use-cases the existence of pid namespaces
> > makes this also a good semantical extension and there have been prior
> > proposals pushing in a similar direction.
> > The trick here is to minimize the risk of regressions which I think is
> > doable. The fact that pid namespaces are hierarchical will help us here.
>
> I understand it is tempting to make pid_max part of a pid namespace but
> was the overlap with pids controller considered?

Of course, but as it was pointed out in the cover-letter, this patch
is not aimed to be
a replacement or suggested alternative to the pids controller.
Obviously, a cgroup way is the best
way to limit and control resources. This is about making an existing
pid_max limit to be namespace
aware to make user space happy. In the context of system containers
(LXC) it's a usual thing to do.
We see some kernel global limit or setting and consider if it's safe
to be namespaced in some way
and if it is safe and if it makes sense then we do it.

Second reason for having this is that we have a real use case scenario
with 32-bit Android Bionic libc
where we need to set a limit for PID *value*. And here, unfortunately,
pids controller does not help either.

>
> I'd consider the alternative of relying of virtualized PID numbers in
> pid namespaces with appropriate pids.max limit and numbers allocation
> strategy that would keep PID values below the limit (i.e. taking the
> first free pid number in given NS, actually I thought it is already the
> case but it doesn't work like that (when I try now [3])).
> WDYT?
>
> TL;DR instead of getting rid of the legacy limit, it was further
> extended to pid namespaces because of legacy workloads and it (almost)
> duplicates existing mechanism. Can this be rethought please?

I hope I explained above why I believe that this does not duplicate an
existing mechanism.

>
> Thanks,
> Michal

Kind regards,
Alex

>
> [1] https://lore.kernel.org/all/20240408145819.8787-1-mkoutny@suse.com/
> [2] https://lore.kernel.org/linux-api/CAHk-=wiZ40LVjnXSi9iHLE_-ZBsWFGCgdmNiYZUXn1-V5YBg2g@mail.gmail.com/

Hello Aleksandr.

On Tue, Feb 25, 2025 at 07:01:21PM +0100, Aleksandr Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> wrote:
> We see some kernel global limit or setting and consider if it's safe
> to be namespaced in some way
> and if it is safe and if it makes sense then we do it.

I know there are ucounts for various per-userns limits (NB RLIMIT_NPROC
among them).
Do you have any other precedents in mind?

In my thinking (biased towards raw resources, not ucounts) it's composed
like one global limit + cgroup limits for non-root groups, hence the
surprise with pid_max granularity.

> Second reason for having this is that we have a real use case scenario
> with 32-bit Android Bionic libc
> where we need to set a limit for PID *value*. And here, unfortunately,
> pids controller does not help either.

(I think if there were no pids controller, namespaced pid_max would be
very good approach how to implement this. But it sounds a little bit
redundant after pids controller was conceived.)

pid namespaces are definitely good place to tackle this since they do
pid numbers virtualization afterall. The challenge is how to limit the
number (amount) and number (pid) of tasks.
Note that besides the pids controller, pid_max and RLIMIT_NPROC, there's
also threads-max limit. Namespacing pid_max makes configuration space
even more complex :-/ In contrast with pids.max, there's no external
visibility of the namespace's pid_max (you must nsenter it) and pid_max
failures are more difficult to troubleshoot (mere failed fork(2)).

Admiteddly, I'm slightly hesitant to pursue the pids controller based
approach due to ns_last_pid. (Also how is that with starting those 32b
apps?  Do they themselves adjust the limits inside the pidns or is this
done by some launcher (who may need privileges to set pids.max)?)


One more idea I have, would be to rebase my original pid_max default
value elimination [1] on top of the namespaced pid_max and not to copy
from parent but start unlimited in the ns too. (Or keep global default
value and unlimit only descednants so that's similar semantics to
ucounts.)


> I hope I explained above why I believe that this does not duplicate an
> existing mechanism.

The 32b scenario is certainly a sensible thing to resolve. But I'm still
worried people would start adjusting both of those and (presumably
different) people would run into unexpected fork failures.

Thanks,
Michal

[1] https://lore.kernel.org/all/20240408145819.8787-1-mkoutny@suse.com/

On Fri, Feb 28, 2025 at 02:46:11PM +0100, Michal Koutný <mkoutny@suse.com> wrote:
> One more idea I have, would be to rebase my original pid_max default
> value elimination [1] on top of the namespaced pid_max and not to copy
> from parent but start unlimited in the ns too. (Or keep global default
> value and unlimit only descednants so that's similar semantics to
> ucounts.)

This seems like a satisfactory conservative correction
https://lore.kernel.org/r/20250305145849.55491-1-mkoutny@suse.com

Michal

On Fri, 22 Nov 2024 14:24:57 +0100, Alexander Mikhalitsyn wrote:
> this is just a rebase/small rework of original Christian Brauner's series
> from:
> https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=pid_max_namespacing
> 
> Christian kindly allowed me to take these patches and resend after small modifications.
> 
> Current tree:
> https://github.com/mihalicyn/linux/commits/pid_max_namespacing
> 
> [...]

Applied to the kernel.pid branch of the vfs/vfs.git tree.
Patches in the kernel.pid branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: kernel.pid

[1/2] pid: allow pid_max to be set per pid namespace
      https://git.kernel.org/vfs/vfs/c/b7e4772ef1dc
[2/2] tests/pid_namespace: add pid_max tests
      https://git.kernel.org/vfs/vfs/c/bef328352883