git: a5d1a0c9bfcc - main - kern: RACCT: Keep process credentials alive via references
Date: Thu, 06 Nov 2025 04:06:58 UTC
The branch main has been updated by olce:
URL: https://cgit.FreeBSD.org/src/commit/?id=a5d1a0c9bfcca38528b861c5afb51ea9b1696b65
commit a5d1a0c9bfcca38528b861c5afb51ea9b1696b65
Author: Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2025-11-03 18:21:08 +0000
Commit: Olivier Certner <olce@FreeBSD.org>
CommitDate: 2025-11-06 04:06:41 +0000
kern: RACCT: Keep process credentials alive via references
In system calls changing process credentials, on RACCT, calls to
racct_proc_ucred_changed() must be issued on the new credentials.
Currently, this is done after the new credentials have been installed on
the process via proc_set_cred() or proc_set_cred_enforce_proc_lim(),
which modifies 'p_ucred'. Only the process lock guarantees that the new
credentials pointed to by 'p_ucred' cannot themselves be concurrently
modified, which would cause their 'struct ucred' to potentially lose its
last reference from the process before the call to
racct_proc_ucred_changed(), which needs one.
For better code understandability and to avoid errors in future
modifications, stop relying on proc_set_cred*() storing the passed
'struct ucred' in the process 'p_ucred' and on the process lock to avoid
the reference taken by proc_set_cred*() to vanish. Instead, ensure that
a reference is held when racct_proc_ucred_changed() is called.
As racct_proc_ucred_changed() is actually passed explicit pointers to
the old and new credentials, there is in fact no need to call it after
proc_set_cred(). Instead, call it before proc_set_cred() and its taking
over the reference.
Since setcred() uses proc_set_cred_enforce_proc_lim(), which can fail,
instead of proc_set_cred(), we instead take an additional reference with
crhold(). Indeed, racct_proc_ucred_changed() should update resource
accounting only if proc_set_cred_enforce_proc_lim() succeeds (an
alternative would be to call it in advance and then in case of failure
of the latter to call it again in order to backpedal the updated
accounting, but we don't see a compelling reason to do that instead of
taking an additional reference).
While here, add to the documentation of proc_set_cred_enforce_proc_lim()
that it does not take over the credentials reference in case of failure.
While here, in racct_proc_ucred_changed()'s herald comment, add the
precise condition in which this function must be called.
No functional change intended.
Reviewed by: kib
MFC after: 3 days
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D53563
---
sys/kern/kern_jail.c | 9 ++++++--
sys/kern/kern_loginclass.c | 7 +++++-
sys/kern/kern_prot.c | 54 +++++++++++++++++++++++++++++++++-------------
sys/kern/kern_racct.c | 6 ++++--
4 files changed, 56 insertions(+), 20 deletions(-)
diff --git a/sys/kern/kern_jail.c b/sys/kern/kern_jail.c
index 523b7e314a10..1b9bd4cf62d5 100644
--- a/sys/kern/kern_jail.c
+++ b/sys/kern/kern_jail.c
@@ -3043,14 +3043,19 @@ do_jail_attach(struct thread *td, struct prison *pr, int drflags)
PROC_LOCK(p);
oldcred = crcopysafe(p, newcred);
newcred->cr_prison = pr;
- proc_set_cred(p, newcred);
- setsugid(p);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
#endif
#ifdef RCTL
crhold(newcred);
#endif
+ /*
+ * Takes over 'newcred''s reference, so 'newcred' must not be used
+ * besides this point except on RCTL where we took an additional
+ * reference above.
+ */
+ proc_set_cred(p, newcred);
+ setsugid(p);
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
diff --git a/sys/kern/kern_loginclass.c b/sys/kern/kern_loginclass.c
index 0c111c4f78d8..07d388f18f8d 100644
--- a/sys/kern/kern_loginclass.c
+++ b/sys/kern/kern_loginclass.c
@@ -222,13 +222,18 @@ sys_setloginclass(struct thread *td, struct setloginclass_args *uap)
PROC_LOCK(p);
oldcred = crcopysafe(p, newcred);
newcred->cr_loginclass = newlc;
- proc_set_cred(p, newcred);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
#endif
#ifdef RCTL
crhold(newcred);
#endif
+ /*
+ * Takes over 'newcred''s reference, so 'newcred' must not be used
+ * besides this point except on RCTL where we took an additional
+ * reference above.
+ */
+ proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 3c145851b683..f93cee6d7698 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -832,22 +832,31 @@ kern_setcred(struct thread *const td, const u_int flags,
if (error != 0)
goto unlock_finish;
+#ifdef RACCT
/*
- * Set the new credentials, noting that they have changed.
+ * Hold a reference to 'new_cred', as we need to call some functions on
+ * it after proc_set_cred_enforce_proc_lim().
*/
+ crhold(new_cred);
+#endif
+
+ /* Set the new credentials. */
cred_set = proc_set_cred_enforce_proc_lim(p, new_cred);
if (cred_set) {
setsugid(p);
- to_free_cred = old_cred;
#ifdef RACCT
+ /* Adjust RACCT counters. */
racct_proc_ucred_changed(p, old_cred, new_cred);
#endif
-#ifdef RCTL
- crhold(new_cred);
-#endif
+ to_free_cred = old_cred;
MPASS(error == 0);
- } else
+ } else {
+#ifdef RACCT
+ /* Matches the crhold() just before the containing 'if'. */
+ crfree(new_cred);
+#endif
error = EAGAIN;
+ }
unlock_finish:
PROC_UNLOCK(p);
@@ -857,10 +866,12 @@ unlock_finish:
* finishing operations.
*/
-#ifdef RCTL
+#ifdef RACCT
if (cred_set) {
+#ifdef RCTL
rctl_proc_ucred_changed(p, new_cred);
- /* Paired with the crhold() just above. */
+#endif
+ /* Paired with the crhold() above. */
crfree(new_cred);
}
#endif
@@ -991,16 +1002,19 @@ sys_setuid(struct thread *td, struct setuid_args *uap)
change_euid(newcred, uip);
setsugid(p);
}
- /*
- * This also transfers the proc count to the new user.
- */
- proc_set_cred(p, newcred);
+
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
#endif
#ifdef RCTL
crhold(newcred);
#endif
+ /*
+ * Takes over 'newcred''s reference, so 'newcred' must not be used
+ * besides this point except on RCTL where we took an additional
+ * reference above.
+ */
+ proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
@@ -1404,13 +1418,18 @@ sys_setreuid(struct thread *td, struct setreuid_args *uap)
change_svuid(newcred, newcred->cr_uid);
setsugid(p);
}
- proc_set_cred(p, newcred);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
#endif
#ifdef RCTL
crhold(newcred);
#endif
+ /*
+ * Takes over 'newcred''s reference, so 'newcred' must not be used
+ * besides this point except on RCTL where we took an additional
+ * reference above.
+ */
+ proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
@@ -1552,13 +1571,18 @@ sys_setresuid(struct thread *td, struct setresuid_args *uap)
change_svuid(newcred, suid);
setsugid(p);
}
- proc_set_cred(p, newcred);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
#endif
#ifdef RCTL
crhold(newcred);
#endif
+ /*
+ * Takes over 'newcred''s reference, so 'newcred' must not be used
+ * besides this point except on RCTL where we took an additional
+ * reference above.
+ */
+ proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
@@ -2783,7 +2807,7 @@ cru2xt(struct thread *td, struct xucred *xcr)
* 'enforce_proc_lim' being true and if no new process can be accounted to the
* new real UID because of the current limit (see the inner comment for more
* details) and the caller does not have privilege (PRIV_PROC_LIMIT) to override
- * that.
+ * that. In this case, the reference to 'newcred' is not taken over.
*/
static bool
_proc_set_cred(struct proc *p, struct ucred *newcred, bool enforce_proc_lim)
diff --git a/sys/kern/kern_racct.c b/sys/kern/kern_racct.c
index 17b64ad00bb5..d1324935bdc3 100644
--- a/sys/kern/kern_racct.c
+++ b/sys/kern/kern_racct.c
@@ -949,8 +949,10 @@ racct_proc_exit(struct proc *p)
}
/*
- * Called after credentials change, to move resource utilisation
- * between raccts.
+ * Called to signal credentials change, to move resource utilisation
+ * between raccts. Must be called with the proc lock held, in the same span as
+ * the credentials change itself (i.e., without the proc lock being unlocked
+ * between the two), but the order does not matter.
*/
void
racct_proc_ucred_changed(struct proc *p, struct ucred *oldcred,