Date: Sun, 11 Oct 2009 10:38:57 GMT From: Edward Tomasz Napierala <trasz@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 169384 for review Message-ID: <200910111038.n9BAcvVu003301@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=169384 Change 169384 by trasz@trasz_victim on 2009/10/11 10:38:24 Get rid of per-group limits. The code implementing them was bad in the first place, and then got horribly broken after Brooks changed the way group ids are stored in the ucred. Some of the infrastructure - 'struct gidinfo', giwhatever() routines etc - are left alone; they are ok and will be reused when per-group limits support get reimplemented. Affected files ... .. //depot/projects/soc2009/trasz_limits/TODO#13 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/init_main.c#16 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_exec.c#9 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_exit.c#16 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_hrl.c#65 edit .. //depot/projects/soc2009/trasz_limits/sys/kern/kern_prot.c#24 edit .. //depot/projects/soc2009/trasz_limits/sys/sys/ucred.h#10 edit Differences ... ==== //depot/projects/soc2009/trasz_limits/TODO#13 (text+ko) ==== @@ -2,10 +2,9 @@ - Make the limits lists protected by the subjects lock (e.g. process lock) instead of hrl_lock. - - Make sure we have all the gidinfos we need in the 'struct ucred'. + - Bring back per-group limits. - Fix up (add/remove resource counters) when: - - Adding a group to a process, - Removing a group from a process, - Moving a process into a jail. - Changing [re]uid; ==== //depot/projects/soc2009/trasz_limits/sys/kern/init_main.c#16 (text+ko) ==== @@ -465,8 +465,6 @@ /* Create credentials. */ p->p_ucred = crget(); p->p_ucred->cr_ngroups = 1; /* group 0 */ - if (p->p_ucred->cr_gidinfos != NULL) - p->p_ucred->cr_gidinfos[0] = gifind(0); p->p_ucred->cr_uidinfo = uifind(0); p->p_ucred->cr_ruidinfo = uifind(0); p->p_ucred->cr_prison = &prison0; ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_exec.c#9 (text+ko) ==== @@ -325,7 +325,6 @@ struct nameidata nd; struct ucred *newcred = NULL, *oldcred; struct uidinfo *euip; - struct gidinfo *egip; register_t *stack_base; int error, len = 0, i; struct image_params image_params, *imgp; @@ -564,7 +563,6 @@ */ newcred = crget(); euip = uifind(attr.va_uid); - egip = gifind(attr.va_gid); i = imgp->args->begin_envv - imgp->args->begin_argv; /* Cache arguments if they fit inside our allowance */ if (ps_arg_cache_limit >= i + sizeof(struct pargs)) { @@ -693,7 +691,7 @@ if (attr.va_mode & S_ISUID) change_euid(newcred, euip); if (attr.va_mode & S_ISGID) - change_egid(newcred, egip); + change_egid(newcred, attr.va_gid); #ifdef MAC if (will_transition) { mac_vnode_execve_transition(oldcred, newcred, imgp->vp, @@ -822,7 +820,6 @@ * Free any resources malloc'd earlier that we didn't use. */ uifree(euip); - gifree(egip); if (newcred == NULL) crfree(oldcred); else ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_exit.c#16 (text+ko) ==== @@ -770,7 +770,7 @@ hrl_proc_exiting(p); /* - * Free credentials, arguments and sigacts. + * Free credentials, arguments, and sigacts. */ crfree(p->p_ucred); p->p_ucred = NULL; ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_hrl.c#65 (text+ko) ==== @@ -59,12 +59,6 @@ #define HRL_DEFAULT_BUFSIZE 4096 #define HRL_LOG_BUFSIZE 128 -int hrl_group_accounting = 0; - -TUNABLE_INT("kern.hrl_group_accounting", &hrl_group_accounting); -SYSCTL_INT(_kern, OID_AUTO, hrl_group_accounting, CTLFLAG_RD, - &hrl_group_accounting, 0, ""); - struct dict { const char *d_name; int d_value; @@ -127,7 +121,7 @@ static void hrl_assert_proc(const struct proc *p __unused) { - int i, resource; + int resource; struct ucred *cred; struct prison *pr; @@ -148,17 +142,6 @@ p->p_usage.hu_resources[resource], ("resource usage propagation meltdown")); } - if (hrl_group_accounting) { - for (i = 0; i < cred->cr_ngroups; i++) { - for (resource = 0; resource <= HRL_RESOURCE_MAX; resource++) { - KASSERT(cred->cr_gidinfos[i]->gi_usage.hu_resources[resource] >= 0, - ("resource usage propagation meltdown")); - KASSERT(cred->cr_gidinfos[i]->gi_usage.hu_resources[resource] >= - p->p_usage.hu_resources[resource], - ("resource usage propagation meltdown")); - } - } - } } #endif /* DIAGNOSTIC */ @@ -258,8 +241,8 @@ static int64_t hrl_available_resource(const struct proc *p, const struct hrl_rule *rule) { - int resource, i; - int64_t available = INT64_MAX, found = 0; + int resource; + int64_t available = INT64_MAX; struct ucred *cred = p->p_ucred; mtx_assert(&hrl_lock, MA_OWNED); @@ -274,20 +257,6 @@ available = rule->hr_amount - cred->cr_ruidinfo->ui_usage.hu_resources[resource]; break; - case HRL_SUBJECT_TYPE_GROUP: - if (hrl_group_accounting) { - for (i = 0; i < cred->cr_ngroups; i++) { - if (cred->cr_gidinfos[i] != - rule->hr_subject.hs_gip) - continue; - - found = 1; - available = rule->hr_amount - - cred->cr_gidinfos[i]->gi_usage.hu_resources[resource]; - } - KASSERT(found, ("hrl_available_resource: group not found")); - } - break; case HRL_SUBJECT_TYPE_LOGINCLASS: available = rule->hr_amount - cred->cr_loginclass->lc_usage.hu_resources[resource]; @@ -475,7 +444,7 @@ int hrl_alloc(struct proc *p, int resource, uint64_t amount) { - int i, j, error; + int error; struct ucred *cred; struct prison *pr; @@ -498,25 +467,6 @@ for (pr = cred->cr_prison; pr != NULL; pr = pr->pr_parent) pr->pr_usage.hu_resources[resource] += amount; cred->cr_loginclass->lc_usage.hu_resources[resource] += amount; - /* - * XXX: Slow. - */ - if (hrl_group_accounting) { - for (i = 0; i < cred->cr_ngroups; i++) { - /* - * Make sure we don't account a group more than once - * if it appears in cr_groups[] more than once. - */ - for (j = 0; j < i; j++) { - if (cred->cr_groups[i] == cred->cr_groups[j]) - goto skip_group; - } - cred->cr_gidinfos[i]->gi_usage.hu_resources[resource] += - amount; -skip_group: - continue; - } - } #ifdef DIAGNOSTIC hrl_assert_proc(p); #endif @@ -535,7 +485,7 @@ int hrl_allocated(struct proc *p, int resource, uint64_t amount) { - int i, j, error; + int error; int64_t diff; struct ucred *cred; struct prison *pr; @@ -562,25 +512,6 @@ for (pr = cred->cr_prison; pr != NULL; pr = pr->pr_parent) pr->pr_usage.hu_resources[resource] += diff; cred->cr_loginclass->lc_usage.hu_resources[resource] += diff; - /* - * XXX: Slow. - */ - if (hrl_group_accounting) { - for (i = 0; i < cred->cr_ngroups; i++) { - /* - * Make sure we don't account a group more than once - * if it appears in cr_groups[] more than once. - */ - for (j = 0; j < i; j++) { - if (cred->cr_groups[i] == cred->cr_groups[j]) - goto skip_group; - } - cred->cr_gidinfos[i]->gi_usage.hu_resources[resource] += - diff; -skip_group: - continue; - } - } #ifdef DIAGNOSTIC hrl_assert_proc(p); #endif @@ -595,7 +526,6 @@ void hrl_free(struct proc *p, int resource, uint64_t amount) { - int i, j; struct ucred *cred; struct prison *pr; @@ -617,25 +547,6 @@ for (pr = cred->cr_prison; pr != NULL; pr = pr->pr_parent) pr->pr_usage.hu_resources[resource] -= amount; cred->cr_loginclass->lc_usage.hu_resources[resource] -= amount; - /* - * XXX: Slow. - */ - if (hrl_group_accounting) { - for (i = 0; i < cred->cr_ngroups; i++) { - /* - * Make sure we don't account a group more than once - * if it appears in cr_groups[] more than once. - */ - for (j = 0; j < i; j++) { - if (cred->cr_groups[i] == cred->cr_groups[j]) - goto skip_group; - } - cred->cr_gidinfos[i]->gi_usage.hu_resources[resource] -= - amount; -skip_group: - continue; - } - } #ifdef DIAGNOSTIC hrl_assert_proc(p); #endif @@ -1149,7 +1060,6 @@ return (rule); } - /* * Link a rule with subjects to which it applies. */ @@ -1165,8 +1075,8 @@ KASSERT(hrl_rule_fully_specified(rule), ("rule not fully specified")); - if ((rule->hr_subject_type == HRL_SUBJECT_TYPE_GROUP || - rule->hr_per == HRL_SUBJECT_TYPE_GROUP) && !hrl_group_accounting) + if (rule->hr_subject_type == HRL_SUBJECT_TYPE_GROUP || + rule->hr_per == HRL_SUBJECT_TYPE_GROUP) return (EOPNOTSUPP); if (rule->hr_action == HRL_ACTION_DELAY) @@ -1734,7 +1644,7 @@ void hrl_proc_ucred_changing(struct proc *p, struct ucred *newcred) { - int error, i; + int error; struct hrl_limit *limit; struct uidinfo *olduip, *newuip; struct loginclass *oldlc, *newlc; @@ -1765,17 +1675,6 @@ error = hrl_limit_add_locked(&p->p_limits, limit->hl_rule); KASSERT(error == 0, ("XXX: better error handling needed")); } - if (hrl_group_accounting) { - for (i = 0; i < newcred->cr_ngroups; i++) { - LIST_FOREACH(limit, - &newcred->cr_gidinfos[i]->gi_limits, hl_next) { - error = hrl_limit_add_locked(&p->p_limits, - limit->hl_rule); - KASSERT(error == 0, - ("XXX: better error handling needed")); - } - } - } /* * Add rules for the current loginclass. @@ -1798,18 +1697,6 @@ } /* - * Fix up per-group resource consumption. - */ - if (hrl_group_accounting) { - for (i = 0; i < p->p_ucred->cr_ngroups; i++) - hrl_usage_subtract( - &p->p_ucred->cr_gidinfos[i]->gi_usage, &p->p_usage); - for (i = 0; i < newcred->cr_ngroups; i++) - hrl_usage_add( - &newcred->cr_gidinfos[i]->gi_usage, &p->p_usage); - } - - /* * Adjust loginclass resource usage information. */ newlc = newcred->cr_loginclass; ==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_prot.c#24 (text+ko) ==== @@ -79,8 +79,6 @@ #include <security/audit/audit.h> #include <security/mac/mac_framework.h> -extern int hrl_group_accounting; - static MALLOC_DEFINE(M_CRED, "cred", "credentials"); SYSCTL_NODE(_security, OID_AUTO, bsd, CTLFLAG_RW, 0, "BSD security policy"); @@ -655,13 +653,11 @@ struct proc *p = td->td_proc; struct ucred *newcred, *oldcred; gid_t gid; - struct gidinfo *gip; int error; gid = uap->gid; AUDIT_ARG_GID(gid); newcred = crget(); - gip = gifind(gid); PROC_LOCK(p); oldcred = crcopysafe(p, newcred); @@ -729,18 +725,16 @@ * Copy credentials so other references do not see our changes. */ if (oldcred->cr_groups[0] != gid) { - change_egid(newcred, gip); + change_egid(newcred, gid); setsugid(p); } change_cred(p, newcred); PROC_UNLOCK(p); - gifree(gip); crfree(oldcred); return (0); fail: PROC_UNLOCK(p); - gifree(gip); crfree(newcred); return (error); } @@ -757,13 +751,11 @@ struct proc *p = td->td_proc; struct ucred *newcred, *oldcred; gid_t egid; - struct gidinfo *egip; int error; egid = uap->egid; AUDIT_ARG_EGID(egid); newcred = crget(); - egip = gifind(egid); PROC_LOCK(p); oldcred = crcopysafe(p, newcred); @@ -779,18 +771,16 @@ goto fail; if (oldcred->cr_groups[0] != egid) { - change_egid(newcred, egip); + change_egid(newcred, egid); setsugid(p); } change_cred(p, newcred); PROC_UNLOCK(p); - gifree(egip); crfree(oldcred); return (0); fail: PROC_UNLOCK(p); - gifree(egip); crfree(newcred); return (error); } @@ -825,8 +815,6 @@ { struct proc *p = td->td_proc; struct ucred *newcred, *oldcred; - struct gidinfo **gidinfos = NULL, **oldgidinfos = NULL; - int i, oldngroups = 0; int error; if (ngrp > NGROUPS) @@ -834,12 +822,6 @@ AUDIT_ARG_GROUPSET(groups, ngrp); newcred = crget(); crextend(newcred, ngrp); - if (hrl_group_accounting) { - gidinfos = malloc(ngrp * sizeof(struct gidinfo *), M_CRED, - M_WAITOK | M_ZERO); - for (i = 0; i < ngrp; i++) - gidinfos[i] = gifind(groups[i]); - } PROC_LOCK(p); oldcred = crcopysafe(p, newcred); @@ -860,53 +842,19 @@ * have the egid in the groups[0]). We risk security holes * when running non-BSD software if we do not do the same. */ - if (hrl_group_accounting) { - oldngroups = newcred->cr_ngroups - 1; - for (i = 0; i < oldngroups; i++) - oldgidinfos[i] = newcred->cr_gidinfos[i + 1]; - } newcred->cr_ngroups = 1; } else { crsetgroups_locked(newcred, ngrp, groups); - if (hrl_group_accounting) { - oldngroups = newcred->cr_ngroups; - for (i = 0; i < oldngroups; i++) - oldgidinfos[i] = newcred->cr_gidinfos[i]; - newcred->cr_ngroups = ngrp; - for (i = 0; i < newcred->cr_ngroups; i++) - newcred->cr_gidinfos[i] = gidinfos[i]; - } } setsugid(p); change_cred(p, newcred); PROC_UNLOCK(p); - if (hrl_group_accounting) { - for (i = 0; i < oldngroups; i++) - gifree(oldgidinfos[i]); - } - /* Don't free gidinfos[]. */ crfree(oldcred); - if (hrl_group_accounting) { - for (i = 0; i < newcred->cr_ngroups; i++) - KASSERT(newcred->cr_gidinfos[i]->gi_gid == newcred->cr_groups[i], ("Whoops.")); - } return (0); fail: PROC_UNLOCK(p); - if (hrl_group_accounting) { - for (i = 0; i < oldngroups; i++) - gifree(oldgidinfos[i]); - free(oldgidinfos, M_CRED); - for (i = 0; i < ngrp; i++) - gifree(gidinfos[i]); - free(gidinfos, M_CRED); - } crfree(newcred); - if (hrl_group_accounting) { - for (i = 0; i < newcred->cr_ngroups; i++) - KASSERT(newcred->cr_gidinfos[i]->gi_gid == newcred->cr_groups[i], ("Whoops.")); - } return (error); } @@ -990,7 +938,6 @@ struct proc *p = td->td_proc; struct ucred *newcred, *oldcred; gid_t egid, rgid; - struct gidinfo *egip; int error; egid = uap->egid; @@ -998,7 +945,6 @@ AUDIT_ARG_EGID(egid); AUDIT_ARG_RGID(rgid); newcred = crget(); - egip = gifind(egid); PROC_LOCK(p); oldcred = crcopysafe(p, newcred); @@ -1016,7 +962,7 @@ goto fail; if (egid != (gid_t)-1 && oldcred->cr_groups[0] != egid) { - change_egid(newcred, egip); + change_egid(newcred, egid); setsugid(p); } if (rgid != (gid_t)-1 && oldcred->cr_rgid != rgid) { @@ -1030,13 +976,11 @@ } change_cred(p, newcred); PROC_UNLOCK(p); - gifree(egip); crfree(oldcred); return (0); fail: PROC_UNLOCK(p); - gifree(egip); crfree(newcred); return (error); } @@ -1138,7 +1082,6 @@ struct proc *p = td->td_proc; struct ucred *newcred, *oldcred; gid_t egid, rgid, sgid; - struct gidinfo *egip; int error; egid = uap->egid; @@ -1148,7 +1091,6 @@ AUDIT_ARG_RGID(rgid); AUDIT_ARG_SGID(sgid); newcred = crget(); - egip = gifind(egid); PROC_LOCK(p); oldcred = crcopysafe(p, newcred); @@ -1171,7 +1113,7 @@ goto fail; if (egid != (gid_t)-1 && oldcred->cr_groups[0] != egid) { - change_egid(newcred, egip); + change_egid(newcred, egid); setsugid(p); } if (rgid != (gid_t)-1 && oldcred->cr_rgid != rgid) { @@ -1184,13 +1126,11 @@ } change_cred(p, newcred); PROC_UNLOCK(p); - gifree(egip); crfree(oldcred); return (0); fail: PROC_UNLOCK(p); - gifree(egip); crfree(newcred); return (error); } @@ -1875,7 +1815,6 @@ void crfree(struct ucred *cr) { - int i; KASSERT(cr->cr_ref > 0, ("bad ucred refcount: %d", cr->cr_ref)); KASSERT(cr->cr_ref != 0xdeadc0de, ("dangling reference to ucred")); @@ -1889,10 +1828,6 @@ uifree(cr->cr_uidinfo); if (cr->cr_ruidinfo != NULL) uifree(cr->cr_ruidinfo); - if (hrl_group_accounting) { - for (i = 0; i < cr->cr_ngroups; i++) - gifree(cr->cr_gidinfos[i]); - } /* * Free a prison, if any. */ @@ -1926,7 +1861,6 @@ void crcopy(struct ucred *dest, struct ucred *src) { - int i; KASSERT(crshared(dest) == 0, ("crcopy of shared ucred")); bcopy(&src->cr_startcopy, &dest->cr_startcopy, @@ -1935,10 +1869,6 @@ crsetgroups(dest, src->cr_ngroups, src->cr_groups); uihold(dest->cr_uidinfo); uihold(dest->cr_ruidinfo); - if (hrl_group_accounting) { - for (i = 0; i < dest->cr_ngroups; i++) - gihold(dest->cr_gidinfos[i]); - } prison_hold(dest->cr_prison); loginclass_acquire(dest->cr_loginclass); #ifdef AUDIT @@ -2053,16 +1983,10 @@ cnt = roundup2(n, PAGE_SIZE / sizeof(gid_t)); /* Free the old array. */ - if (cr->cr_groups) { + if (cr->cr_groups) free(cr->cr_groups, M_CRED); - if (hrl_group_accounting) - free(cr->cr_gidinfos, M_CRED); - } cr->cr_groups = malloc(cnt * sizeof(gid_t), M_CRED, M_WAITOK | M_ZERO); - if (hrl_group_accounting) - cr->cr_gidinfos = malloc(cnt * sizeof(struct gidinfo *), M_CRED, - M_WAITOK | M_ZERO); cr->cr_agroups = cnt; } @@ -2222,15 +2146,10 @@ * duration of the call. */ void -change_egid(struct ucred *newcred, struct gidinfo *egip) +change_egid(struct ucred *newcred, gid_t egid) { - newcred->cr_groups[0] = egip->gi_gid; - if (hrl_group_accounting) { - gihold(egip); - gifree(newcred->cr_gidinfos[0]); - newcred->cr_gidinfos[0] = egip; - } + newcred->cr_groups[0] = egid; } /*- ==== //depot/projects/soc2009/trasz_limits/sys/sys/ucred.h#10 (text+ko) ==== @@ -63,7 +63,6 @@ struct label *cr_label; /* MAC label */ struct auditinfo_addr cr_audit; /* Audit properties. */ gid_t *cr_groups; /* groups */ - struct gidinfo **cr_gidinfos; /* per group resource consumption */ int cr_agroups; /* Available groups */ }; #define NOCRED ((struct ucred *)0) /* no credential available */ @@ -93,7 +92,7 @@ struct proc; void change_cred(struct proc *p, struct ucred *newcred); -void change_egid(struct ucred *newcred, struct gidinfo *egip); +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); void change_ruid(struct ucred *newcred, struct uidinfo *ruip);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200910111038.n9BAcvVu003301>