PERFORCE change 187860 for review
Edward Tomasz Napierala
trasz at FreeBSD.org
Sun Jan 16 14:45:08 UTC 2011
http://p4web.freebsd.org/@@187860?ac=10
Change 187860 by trasz at 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);
More information about the p4-projects
mailing list