Date: Sun, 16 Jan 2011 14:45:07 GMT From: Edward Tomasz Napierala <trasz@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 187860 for review Message-ID: <201101161445.p0GEj7H6048739@skunkworks.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://p4web.freebsd.org/@@187860?ac=10 Change 187860 by trasz@trasz_victim on 2011/01/16 14:44:46 When credential changes, the per-process list of applicable RCTL rules needs to be updated, which requires memory allocation. We can't return error there, so M_NOWAIT cannot be used. Rework this piece of code so that we use M_WAITOK. It cannot be called with proc lock held, so it cannot be called from change_cred(). Thus, retire change_cred() altogether. This was probably the last large known problem with rctl. Affected files ... .. //depot/projects/soc2009/trasz_limits/sys/fs/unionfs/union_subr.c#12 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_container.c#53 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_exec.c#23 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_jail.c#31 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_loginclass.c#26 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_prot.c#32 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_rctl.c#9 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/container.h#21 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/rctl.h#3 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/ucred.h#11 edit Differences ... ==== //depot/projects/soc2009/trasz_limits/sys/fs/unionfs/union_subr.c#12 (text+ko) ==== @@ -775,6 +775,11 @@ /* Authority change to root */ rootinfo = uifind((uid_t)0); cred = crdup(cnp->cn_cred); + /* + * The calls to chgproccnt() are needed to compensate for change_ruid() + * calling chgproccnt(). + */ + chgproccnt(cred->cr_ruidinfo, 1, 0); change_euid(cred, rootinfo); change_ruid(cred, rootinfo); change_svuid(cred, (uid_t)0); @@ -824,6 +829,7 @@ unionfs_mkshadowdir_abort: cnp->cn_cred = credbk; + chgproccnt(cred->cr_ruidinfo, -1, 0); crfree(cred); return (error); ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_container.c#53 (text+ko) ==== @@ -623,24 +623,25 @@ } /* - * Called before credentials change, to move resource utilisation + * Called after credentials change, to move resource utilisation * between containers. */ void -container_proc_ucred_changing(struct proc *p, struct ucred *newcred) +container_proc_ucred_changed(struct proc *p, struct ucred *oldcred, + struct ucred *newcred) { struct uidinfo *olduip, *newuip; struct loginclass *oldlc, *newlc; struct prison *oldpr, *newpr, *pr; - PROC_LOCK_ASSERT(p, MA_OWNED); + PROC_LOCK_ASSERT(p, MA_NOTOWNED); newuip = newcred->cr_ruidinfo; - olduip = p->p_ucred->cr_ruidinfo; + olduip = oldcred->cr_ruidinfo; newlc = newcred->cr_loginclass; - oldlc = p->p_ucred->cr_loginclass; + oldlc = oldcred->cr_loginclass; newpr = newcred->cr_prison; - oldpr = p->p_ucred->cr_prison; + oldpr = oldcred->cr_prison; mtx_lock(&container_lock); if (newuip != olduip) { @@ -660,7 +661,7 @@ mtx_unlock(&container_lock); #ifdef RCTL - rctl_proc_ucred_changing(p, newcred); + rctl_proc_ucred_changed(p, newcred); #endif } ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_exec.c#23 (text+ko) ==== @@ -698,7 +698,7 @@ */ change_svuid(newcred, newcred->cr_uid); change_svgid(newcred, newcred->cr_gid); - change_cred(p, newcred); + p->p_ucred = newcred; newcred = NULL; } else { if (oldcred->cr_uid == oldcred->cr_ruid && @@ -720,7 +720,7 @@ oldcred->cr_svgid != oldcred->cr_gid) { change_svuid(newcred, newcred->cr_uid); change_svgid(newcred, newcred->cr_gid); - change_cred(p, newcred); + p->p_ucred = newcred; newcred = NULL; } } ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_jail.c#31 (text+ko) ==== @@ -2294,8 +2294,11 @@ setsugid(p); crcopy(newcred, oldcred); newcred->cr_prison = pr; - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); +#ifdef CONTAINERS + container_proc_ucred_changed(p, oldcred, newcred); +#endif crfree(oldcred); prison_deref(ppr, PD_DEREF | PD_DEUREF); return (0); ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_loginclass.c#26 (text+ko) ==== @@ -202,8 +202,11 @@ PROC_LOCK(p); oldcred = crcopysafe(p, newcred); newcred->cr_loginclass = newlc; - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); +#ifdef CONTAINERS + container_proc_ucred_changed(p, oldcred, newcred); +#endif loginclass_release(oldcred->cr_loginclass); crfree(oldcred); ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_prot.c#32 (text+ko) ==== @@ -578,8 +578,11 @@ change_euid(newcred, uip); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); +#ifdef CONTAINERS + container_proc_ucred_changed(p, oldcred, newcred); +#endif uifree(uip); crfree(oldcred); return (0); @@ -634,7 +637,7 @@ change_euid(newcred, euip); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); uifree(euip); crfree(oldcred); @@ -734,7 +737,7 @@ change_egid(newcred, gid); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); crfree(oldcred); return (0); @@ -780,7 +783,7 @@ change_egid(newcred, egid); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); crfree(oldcred); return (0); @@ -853,7 +856,7 @@ crsetgroups_locked(newcred, ngrp, groups); } setsugid(p); - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); crfree(oldcred); return (0); @@ -916,8 +919,11 @@ change_svuid(newcred, newcred->cr_uid); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); +#ifdef CONTAINERS + container_proc_ucred_changed(p, oldcred, newcred); +#endif uifree(ruip); uifree(euip); crfree(oldcred); @@ -980,7 +986,7 @@ change_svgid(newcred, newcred->cr_groups[0]); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); crfree(oldcred); return (0); @@ -1054,8 +1060,11 @@ change_svuid(newcred, suid); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); +#ifdef CONTAINERS + container_proc_ucred_changed(p, oldcred, newcred); +#endif uifree(ruip); uifree(euip); crfree(oldcred); @@ -1130,7 +1139,7 @@ change_svgid(newcred, sgid); setsugid(p); } - change_cred(p, newcred); + p->p_ucred = newcred; PROC_UNLOCK(p); crfree(oldcred); return (0); @@ -2116,26 +2125,6 @@ p->p_stops = 0; } -/* - * Assign new credential to the process, fixing up RCTL accounting - * as neccessary. - */ -void -change_cred(struct proc *p, struct ucred *newcred) -{ - PROC_LOCK_ASSERT(p, MA_OWNED); - - if (p->p_ucred->cr_ruidinfo != newcred->cr_ruidinfo) { - chgproccnt(p->p_ucred->cr_ruidinfo, -1, 0); - chgproccnt(newcred->cr_ruidinfo, 1, 0); - } - -#ifdef CONTAINERS - container_proc_ucred_changing(p, newcred); -#endif - p->p_ucred = newcred; -} - /*- * Change a process's effective uid. * Side effects: newcred->cr_uid and newcred->cr_uidinfo will be modified. @@ -2177,10 +2166,12 @@ change_ruid(struct ucred *newcred, struct uidinfo *ruip) { + (void)chgproccnt(newcred->cr_ruidinfo, -1, 0); newcred->cr_ruid = ruip->ui_uid; uihold(ruip); uifree(newcred->cr_ruidinfo); newcred->cr_ruidinfo = ruip; + (void)chgproccnt(newcred->cr_ruidinfo, 1, 0); } /*- ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_rctl.c#9 (text+ko) ==== @@ -1414,82 +1414,145 @@ return (error); } +/* + * Update RCTL rule list after credential change. + */ void -rctl_proc_ucred_changing(struct proc *p, struct ucred *newcred) +rctl_proc_ucred_changed(struct proc *p, struct ucred *newcred) { - int error; - struct rctl_rule_link *link; - struct uidinfo *olduip, *newuip; - struct loginclass *oldlc, *newlc; - struct prison *oldpr, *newpr; + int rulecnt, i; + struct rctl_rule_link *link, *newlink; + struct uidinfo *newuip; + struct loginclass *newlc; + struct prison *newpr; + LIST_HEAD(, rctl_rule_link) newrules; - PROC_LOCK_ASSERT(p, MA_OWNED); - newuip = newcred->cr_ruidinfo; - olduip = p->p_ucred->cr_ruidinfo; newlc = newcred->cr_loginclass; - oldlc = p->p_ucred->cr_loginclass; newpr = newcred->cr_prison; - oldpr = p->p_ucred->cr_prison; + + LIST_INIT(&newrules); +again: /* - * Remove rules that are no longer applicable with the new ucred. + * First, count the rules that apply to the process with new + * credentials. */ - rw_wlock(&rctl_lock); + rulecnt = 0; + rw_rlock(&rctl_lock); LIST_FOREACH(link, &p->p_container.c_rule_links, rctl_next) { - switch (link->rctl_rule->hr_subject_type) { - case RCTL_SUBJECT_TYPE_PROCESS: - continue; - case RCTL_SUBJECT_TYPE_USER: - if (newuip == olduip) - continue; - break; - case RCTL_SUBJECT_TYPE_LOGINCLASS: - if (newlc == oldlc) - continue; - break; - case RCTL_SUBJECT_TYPE_JAIL: - if (newpr == oldpr) - continue; - break; - default: - panic("rctl_proc_ucred_changing: unknown subject %d", - link->rctl_rule->hr_subject_type); - } + if (link->rctl_rule->hr_subject_type == + RCTL_SUBJECT_TYPE_PROCESS) + rulecnt++; + } + LIST_FOREACH(link, &newuip->ui_container.c_rule_links, rctl_next) + rulecnt++; + LIST_FOREACH(link, &newlc->lc_container.c_rule_links, rctl_next) + rulecnt++; + LIST_FOREACH(link, &newpr->pr_container.c_rule_links, rctl_next) + rulecnt++; + rw_runlock(&rctl_lock); - LIST_REMOVE(link, rctl_next); - rctl_rule_release(link->rctl_rule); - uma_zfree(rctl_rule_link_zone, link); + /* + * Create temporary list. We've dropped the rctl_lock in order + * to use M_WAITOK. + */ + for (i = 0; i < rulecnt; i++) { + newlink = uma_zalloc(rctl_rule_link_zone, M_WAITOK); + newlink->rctl_rule = NULL; + LIST_INSERT_HEAD(&newrules, newlink, rctl_next); } - rw_wunlock(&rctl_lock); - + + newlink = LIST_FIRST(&newrules); + /* - * Add rules for the new ucred and move between containers where applicable. + * Assign rules to the newly allocated list entries. */ - if (newuip != olduip) { - rw_wlock(&rctl_lock); - LIST_FOREACH(link, &newuip->ui_container.c_rule_links, rctl_next) { - error = rctl_container_add_rule_locked(&p->p_container, link->rctl_rule); - KASSERT(error == 0, ("XXX: better error handling needed")); + rw_wlock(&rctl_lock); + LIST_FOREACH(link, &p->p_container.c_rule_links, rctl_next) { + if (link->rctl_rule->hr_subject_type == + RCTL_SUBJECT_TYPE_PROCESS) { + if (newlink == NULL) + goto goaround; + rctl_rule_acquire(link->rctl_rule); + newlink->rctl_rule = link->rctl_rule; + newlink = LIST_NEXT(newlink, rctl_next); + rulecnt--; } - rw_wunlock(&rctl_lock); + } + + LIST_FOREACH(link, &newuip->ui_container.c_rule_links, rctl_next) { + if (newlink == NULL) + goto goaround; + rctl_rule_acquire(link->rctl_rule); + newlink->rctl_rule = link->rctl_rule; + newlink = LIST_NEXT(newlink, rctl_next); + rulecnt--; + } + + LIST_FOREACH(link, &newlc->lc_container.c_rule_links, rctl_next) { + if (newlink == NULL) + goto goaround; + rctl_rule_acquire(link->rctl_rule); + newlink->rctl_rule = link->rctl_rule; + newlink = LIST_NEXT(newlink, rctl_next); + rulecnt--; + } + + LIST_FOREACH(link, &newpr->pr_container.c_rule_links, rctl_next) { + if (newlink == NULL) + goto goaround; + rctl_rule_acquire(link->rctl_rule); + newlink->rctl_rule = link->rctl_rule; + newlink = LIST_NEXT(newlink, rctl_next); + rulecnt--; } - if (newlc != oldlc) { - rw_wlock(&rctl_lock); - LIST_FOREACH(link, &newlc->lc_container.c_rule_links, rctl_next) { - error = rctl_container_add_rule_locked(&p->p_container, link->rctl_rule); - KASSERT(error == 0, ("XXX: better error handling needed")); + + if (rulecnt == 0) { + /* + * Free the old rule list. + */ + while (!LIST_EMPTY(&p->p_container.c_rule_links)) { + link = LIST_FIRST(&p->p_container.c_rule_links); + LIST_REMOVE(link, rctl_next); + rctl_rule_release(link->rctl_rule); + uma_zfree(rctl_rule_link_zone, link); + } + + /* + * Replace lists and we're done. + * + * XXX: Is there any way to switch list heads instead + * of iterating here? + */ + while (!LIST_EMPTY(&newrules)) { + newlink = LIST_FIRST(&newrules); + LIST_REMOVE(newlink, rctl_next); + LIST_INSERT_HEAD(&p->p_container.c_rule_links, + newlink, rctl_next); } + rw_wunlock(&rctl_lock); + + return; } - if (newpr != oldpr) { - rw_wlock(&rctl_lock); - LIST_FOREACH(link, &newpr->pr_container.c_rule_links, rctl_next) { - error = rctl_container_add_rule_locked(&p->p_container, link->rctl_rule); - KASSERT(error == 0, ("XXX: better error handling needed")); - } - rw_wunlock(&rctl_lock); + +goaround: + rw_wunlock(&rctl_lock); + + /* + * Rule list changed while we were not holding the rctl_lock. + * Free the new list and try again. + */ + while (!LIST_EMPTY(&newrules)) { + newlink = LIST_FIRST(&newrules); + LIST_REMOVE(newlink, rctl_next); + if (newlink->rctl_rule != NULL) + rctl_rule_release(newlink->rctl_rule); + uma_zfree(rctl_rule_link_zone, newlink); } + + goto again; } /* ==== //depot/projects/soc2009/trasz_limits/sys/sys/container.h#21 (text+ko) ==== @@ -109,6 +109,7 @@ int container_proc_fork(struct proc *parent, struct proc *child); void container_proc_exit(struct proc *p); -void container_proc_ucred_changing(struct proc *p, struct ucred *newcred); +void container_proc_ucred_changed(struct proc *p, struct ucred *oldcred, + struct ucred *newcred); #endif /* !_CONTAINER_H_ */ ==== //depot/projects/soc2009/trasz_limits/sys/sys/rctl.h#3 (text+ko) ==== @@ -110,7 +110,7 @@ #ifdef _KERNEL -void rctl_proc_ucred_changing(struct proc *p, struct ucred *newcred); +void rctl_proc_ucred_changed(struct proc *p, struct ucred *newcred); struct rctl_rule *rctl_rule_alloc(int flags); struct rctl_rule *rctl_rule_duplicate(const struct rctl_rule *rule, int flags); ==== //depot/projects/soc2009/trasz_limits/sys/sys/ucred.h#11 (text+ko) ==== @@ -91,7 +91,6 @@ struct thread; struct proc; -void change_cred(struct proc *p, struct ucred *newcred); void change_egid(struct ucred *newcred, gid_t egid); void change_euid(struct ucred *newcred, struct uidinfo *euip); void change_rgid(struct ucred *newcred, gid_t rgid);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201101161445.p0GEj7H6048739>