Michal Koutný <[email protected]> wrote:
> It was reported that v5.14 behaves differently when enforcing
> RLIMIT_NPROC limit, namely, it allows one more task than previously.
> This is consequence of the commit 21d1c5e386bc ("Reimplement
> RLIMIT_NPROC on top of ucounts") that missed the sharpness of
> equality in the forking path.
This can be fixed either by fixing the test or by moving the increment
to be before the test. Fix it my moving copy_creds which contains
the increment before is_ucounts_overlimit.
This is necessary so that in the case of CLONE_NEWUSER the new credential
with the new ucounts is available of is_ucounts_overlimit to test.
Both the test in fork and the test in set_user were semantically
changed when the code moved to ucounts. The change of the test in
fork was bad because it was before the increment. The test in
set_user was wrong and the change to ucounts fixed it. So this
fix is only restore the old behavior in one lcatio not two.
Link: https://lkml.kernel.org/r/[email protected]
Cc: [email protected]
Reported-by: Michal Koutný <[email protected]>
Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
kernel/fork.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index d75a528f7b21..17d8a8c85e3b 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2021,18 +2021,18 @@ static __latent_entropy struct task_struct *copy_process(
#ifdef CONFIG_PROVE_LOCKING
DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
#endif
+ retval = copy_creds(p, clone_flags);
+ if (retval < 0)
+ goto bad_fork_free;
+
retval = -EAGAIN;
if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) {
if (p->real_cred->user != INIT_USER &&
!capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN))
- goto bad_fork_free;
+ goto bad_fork_cleanup_count;
}
current->flags &= ~PF_NPROC_EXCEEDED;
- retval = copy_creds(p, clone_flags);
- if (retval < 0)
- goto bad_fork_free;
-
/*
* If multiple threads are within copy_process(), then this check
* triggers too late. This doesn't hurt, the check is only there
--
2.29.2
On Thu, Feb 10, 2022 at 08:13:17PM -0600, "Eric W. Biederman" <[email protected]> wrote:
> This can be fixed either by fixing the test or by moving the increment
> to be before the test. Fix it my moving copy_creds which contains
> the increment before is_ucounts_overlimit.
This is simpler than my approach and I find it correct too.
> Both the test in fork and the test in set_user were semantically
> changed when the code moved to ucounts. The change of the test in
> fork was bad because it was before the increment.
>
> The test in set_user was wrong and the change to ucounts fixed it. So
> this fix is only restore the old behavior in one lcatio not two.
Whom should be the task accounted to in the case of set*uid? (The change
to ucounts made the check against the pre-switch user's ucounts.)
> ---
> kernel/fork.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Michal Koutn? <[email protected]>
Michal Koutný <[email protected]> writes:
> On Thu, Feb 10, 2022 at 08:13:17PM -0600, "Eric W. Biederman" <[email protected]> wrote:
>> This can be fixed either by fixing the test or by moving the increment
>> to be before the test. Fix it my moving copy_creds which contains
>> the increment before is_ucounts_overlimit.
>
> This is simpler than my approach and I find it correct too.
>
>> Both the test in fork and the test in set_user were semantically
>> changed when the code moved to ucounts. The change of the test in
>> fork was bad because it was before the increment.
>>
>> The test in set_user was wrong and the change to ucounts fixed it. So
>> this fix is only restore the old behavior in one lcatio not two.
>
> Whom should be the task accounted to in the case of set*uid? (The change
> to ucounts made the check against the pre-switch user's ucounts.)
It needs to be post-switch in the case of set*id().
I have that fixed in the next version of my patchset.
>> ---
>> kernel/fork.c | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> Reviewed-by: Michal Koutný <[email protected]>