call racct_proc_ucred_changed() under the proc lock

The lock is required to ensure that the switch to the new credentials
and the transfer of the process's accounting data from the old
credentials to the new ones is done atomically.  Otherwise, some updates
may be applied to the new credentials and then additionally transferred
from the old credentials if the updates happen after proc_set_cred() and
before racct_proc_ucred_changed().

The problem is especially pronounced for RACCT_RSS because
- there is a strict accounting for this resource (it's reclaimable)
- it's updated asynchronously by the vm daemon
- it's updated by setting an absolute value instead of applying a delta

I had to remove a call to rctl_proc_ucred_changed() from
racct_proc_ucred_changed() and make all callers of latter call the
former as well.  The reason is that rctl_proc_ucred_changed, as it is
implemented now, cannot be called while holding the proc lock, so the
lock is dropped after calling racct_proc_ucred_changed.  Additionally,
I've added calls to crhold / crfree around the rctl call, because
without the proc lock there is no gurantee that the new credentials,
owned by the process, will stay stable.  That does not eliminate a
possibility that the credentials passed to the rctl will get stale.
Ideally, rctl_proc_ucred_changed should be able to work under the proc
lock.

Many thanks to kib for pointing out the above problems.

PR:		222027
Discussed with:	kib
No comment:	trasz
MFC after:	2 weeks
Differential Revision: https://reviews.freebsd.org/D15048
This commit is contained in:
Andriy Gapon 2018-04-20 13:08:04 +00:00
parent cb0d9834d4
commit f87beb93e8
5 changed files with 42 additions and 13 deletions

View File

@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$");
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/racct.h>
#include <sys/rctl.h>
#include <sys/refcount.h>
#include <sys/sx.h>
#include <sys/sysent.h>
@ -2401,9 +2402,14 @@ do_jail_attach(struct thread *td, struct prison *pr)
newcred->cr_prison = pr;
proc_set_cred(p, newcred);
setsugid(p);
PROC_UNLOCK(p);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
crhold(newcred);
#endif
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
crfree(newcred);
#endif
prison_deref(oldcred->cr_prison, PD_DEREF | PD_DEUREF);
crfree(oldcred);
@ -3960,6 +3966,7 @@ prison_racct_modify(struct prison *pr)
*/
racct_move(pr->pr_prison_racct->prr_racct, oldprr->prr_racct);
#ifdef RCTL
/*
* Force rctl to reattach rules to processes.
*/
@ -3967,9 +3974,10 @@ prison_racct_modify(struct prison *pr)
PROC_LOCK(p);
cred = crhold(p->p_ucred);
PROC_UNLOCK(p);
racct_proc_ucred_changed(p, cred, cred);
rctl_proc_ucred_changed(p, cred);
crfree(cred);
}
#endif
sx_sunlock(&allproc_lock);
prison_racct_free_locked(oldprr);

View File

@ -58,6 +58,7 @@ __FBSDID("$FreeBSD$");
#include <sys/proc.h>
#include <sys/queue.h>
#include <sys/racct.h>
#include <sys/rctl.h>
#include <sys/refcount.h>
#include <sys/rwlock.h>
#include <sys/sysproto.h>
@ -230,9 +231,14 @@ sys_setloginclass(struct thread *td, struct setloginclass_args *uap)
oldcred = crcopysafe(p, newcred);
newcred->cr_loginclass = newlc;
proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
crhold(newcred);
#endif
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
crfree(newcred);
#endif
loginclass_free(oldcred->cr_loginclass);
crfree(oldcred);

View File

@ -66,6 +66,7 @@ __FBSDID("$FreeBSD$");
#include <sys/jail.h>
#include <sys/pioctl.h>
#include <sys/racct.h>
#include <sys/rctl.h>
#include <sys/resourcevar.h>
#include <sys/socket.h>
#include <sys/socketvar.h>
@ -575,9 +576,14 @@ sys_setuid(struct thread *td, struct setuid_args *uap)
setsugid(p);
}
proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
crhold(newcred);
#endif
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
crfree(newcred);
#endif
uifree(uip);
crfree(oldcred);
@ -923,9 +929,14 @@ sys_setreuid(struct thread *td, struct setreuid_args *uap)
setsugid(p);
}
proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
crhold(newcred);
#endif
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
crfree(newcred);
#endif
uifree(ruip);
uifree(euip);
@ -1064,9 +1075,14 @@ sys_setresuid(struct thread *td, struct setresuid_args *uap)
setsugid(p);
}
proc_set_cred(p, newcred);
PROC_UNLOCK(p);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
crhold(newcred);
#endif
PROC_UNLOCK(p);
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
crfree(newcred);
#endif
uifree(ruip);
uifree(euip);

View File

@ -1046,7 +1046,7 @@ racct_proc_ucred_changed(struct proc *p, struct ucred *oldcred,
if (!racct_enable)
return;
PROC_LOCK_ASSERT(p, MA_NOTOWNED);
PROC_LOCK_ASSERT(p, MA_OWNED);
newuip = newcred->cr_ruidinfo;
olduip = oldcred->cr_ruidinfo;
@ -1073,10 +1073,6 @@ racct_proc_ucred_changed(struct proc *p, struct ucred *oldcred,
p->p_racct);
}
RACCT_UNLOCK();
#ifdef RCTL
rctl_proc_ucred_changed(p, newcred);
#endif
}
void

View File

@ -1956,12 +1956,15 @@ rctl_proc_ucred_changed(struct proc *p, struct ucred *newcred)
struct prison_racct *newprr;
int rulecnt, i;
ASSERT_RACCT_ENABLED();
if (!racct_enable)
return;
PROC_LOCK_ASSERT(p, MA_NOTOWNED);
newuip = newcred->cr_ruidinfo;
newlc = newcred->cr_loginclass;
newprr = newcred->cr_prison->pr_prison_racct;
LIST_INIT(&newrules);
again: