Date: Tue, 18 Jun 2002 22:54:17 -0700 (PDT) From: Don Lewis <dl-freebsd@catspoiler.org> To: bright@mu.org Cc: nate@root.org, current@FreeBSD.ORG Subject: Re: Suggested fixes for uidinfo "would sleep" messages Message-ID: <200206190554.g5J5sGM1064829@gw.catspoiler.org> In-Reply-To: <20020618194828.GC12139@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 18 Jun, Alfred Perlstein wrote:
> * Nate Lawson <nate@root.org> [020618 12:17] wrote:
>> As with others on the list, I've been getting a lot of witness complaints:
>>
>> ../../../vm/uma_core.c:1327: could sleep with "process lock" locked from
>> ../../../kern/kern_prot.c:511
>> ../../../vm/uma_core.c:1327: could sleep with "process lock" locked from
>> ../../../kern/kern_prot.c:613
>>
>> Basically, every time cron runs, it does a setuid() or seteuid(), which
>> calls change_ruid or change_euid which call uifree and uifind (which does
>> the malloc with M_WAITOK while holding PROC_LOCK).
> ...
>> Is anyone else working on a fix?
>
> The code should not be holding proc locks over ui*() calls.
I finally got tired of seeing these messages. Since I haven't seen
anybody post a patch, I bit the bullet and cranked one out. It could
use some examination to make sure that the reference counts are handled
properly and there aren't any leaks. I'm not terribly happy about
having to unhide the uidinfo stuff and expose it to the users of
change_{r,e}uid(), and I don't like allocating memory before checking
permissions, but it looks like the alternatives are worse.
Index: sys/alpha/osf1/osf1_misc.c
===================================================================
RCS file: /home/ncvs/src/sys/alpha/osf1/osf1_misc.c,v
retrieving revision 1.30
diff -u -r1.30 osf1_misc.c
--- sys/alpha/osf1/osf1_misc.c 13 Apr 2002 23:11:22 -0000 1.30
+++ sys/alpha/osf1/osf1_misc.c 18 Jun 2002 19:11:50 -0000
@@ -1056,17 +1056,20 @@
struct proc *p;
int error;
uid_t uid;
+ struct uidinfo *uip;
struct ucred *newcred, *oldcred;
p = td->td_proc;
uid = SCARG(uap, uid);
newcred = crget();
+ uip = uifind(uid);
PROC_LOCK(p);
oldcred = p->p_ucred;
if ((error = suser_cred(p->p_ucred, PRISON_ROOT)) != 0 &&
uid != oldcred->cr_ruid && uid != oldcred->cr_svuid) {
PROC_UNLOCK(p);
+ uifree(uip);
crfree(newcred);
return (error);
}
@@ -1074,7 +1077,7 @@
crcopy(newcred, oldcred);
if (error == 0) {
if (uid != oldcred->cr_ruid) {
- change_ruid(newcred, uid);
+ change_ruid(newcred, uip);
setsugid(p);
}
if (oldcred->cr_svuid != uid) {
@@ -1083,11 +1086,12 @@
}
}
if (newcred->cr_uid != uid) {
- change_euid(newcred, uid);
+ change_euid(newcred, uip);
setsugid(p);
}
p->p_ucred = newcred;
PROC_UNLOCK(p);
+ uifree(uip);
crfree(oldcred);
return (0);
}
Index: sys/kern/kern_exec.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_exec.c,v
retrieving revision 1.164
diff -u -r1.164 kern_exec.c
--- sys/kern/kern_exec.c 7 Jun 2002 05:41:27 -0000 1.164
+++ sys/kern/kern_exec.c 18 Jun 2002 19:09:06 -0000
@@ -128,6 +128,7 @@
struct proc *p = td->td_proc;
struct nameidata nd, *ndp;
struct ucred *newcred = NULL, *oldcred;
+ struct uidinfo *euip;
register_t *stack_base;
int error, len, i;
struct image_params image_params, *imgp;
@@ -303,6 +304,7 @@
* Malloc things before we need locks.
*/
newcred = crget();
+ euip = uifind(attr.va_uid);
i = imgp->endargs - imgp->stringbase;
if (ps_arg_cache_limit >= i + sizeof(struct pargs))
newargs = pargs_alloc(i);
@@ -390,7 +392,7 @@
*/
crcopy(newcred, oldcred);
if (attr.va_mode & VSUID)
- change_euid(newcred, attr.va_uid);
+ change_euid(newcred, euip);
if (attr.va_mode & VSGID)
change_egid(newcred, attr.va_gid);
setugidsafety(td);
@@ -472,6 +474,7 @@
/*
* Free any resources malloc'd earlier that we didn't use.
*/
+ uifree(euip);
if (newcred == NULL)
crfree(oldcred);
else
Index: sys/kern/kern_prot.c
===================================================================
RCS file: /home/ncvs/src/sys/kern/kern_prot.c,v
retrieving revision 1.156
diff -u -r1.156 kern_prot.c
--- sys/kern/kern_prot.c 19 May 2002 00:14:48 -0000 1.156
+++ sys/kern/kern_prot.c 18 Jun 2002 18:50:01 -0000
@@ -503,11 +503,13 @@
struct proc *p = td->td_proc;
struct ucred *newcred, *oldcred;
uid_t uid;
+ struct uidinfo *uip;
int error;
mtx_lock(&Giant);
uid = uap->uid;
newcred = crget();
+ uip = uifind(uid);
PROC_LOCK(p);
oldcred = p->p_ucred;
@@ -537,11 +539,15 @@
#endif
(error = suser_cred(oldcred, PRISON_ROOT)) != 0) {
PROC_UNLOCK(p);
+ uifree(uip);
crfree(newcred);
mtx_unlock(&Giant);
return (error);
}
+ /*
+ * Copy credentials so other references do not see our changes.
+ */
crcopy(newcred, oldcred);
#ifdef _POSIX_SAVED_IDS
/*
@@ -559,7 +565,7 @@
* Set the real uid and transfer proc count to new user.
*/
if (uid != oldcred->cr_ruid) {
- change_ruid(newcred, uid);
+ change_ruid(newcred, uip);
setsugid(p);
}
/*
@@ -577,14 +583,14 @@
/*
* In all permitted cases, we are changing the euid.
- * Copy credentials so other references do not see our changes.
*/
if (uid != oldcred->cr_uid) {
- change_euid(newcred, uid);
+ change_euid(newcred, uip);
setsugid(p);
}
p->p_ucred = newcred;
PROC_UNLOCK(p);
+ uifree(uip);
crfree(oldcred);
mtx_unlock(&Giant);
return (0);
@@ -605,17 +611,20 @@
struct proc *p = td->td_proc;
struct ucred *newcred, *oldcred;
uid_t euid;
+ struct uidinfo *euip;
int error;
euid = uap->euid;
mtx_lock(&Giant);
newcred = crget();
+ euip = uifind(euid);
PROC_LOCK(p);
oldcred = p->p_ucred;
if (euid != oldcred->cr_ruid && /* allow seteuid(getuid()) */
euid != oldcred->cr_svuid && /* allow seteuid(saved uid) */
(error = suser_cred(oldcred, PRISON_ROOT)) != 0) {
PROC_UNLOCK(p);
+ uifree(euip);
crfree(newcred);
mtx_unlock(&Giant);
return (error);
@@ -626,11 +635,12 @@
*/
crcopy(newcred, oldcred);
if (oldcred->cr_uid != euid) {
- change_euid(newcred, euid);
+ change_euid(newcred, euip);
setsugid(p);
}
p->p_ucred = newcred;
PROC_UNLOCK(p);
+ uifree(euip);
crfree(oldcred);
mtx_unlock(&Giant);
return (0);
@@ -858,12 +868,15 @@
struct proc *p = td->td_proc;
struct ucred *newcred, *oldcred;
uid_t euid, ruid;
+ struct uidinfo *euip, *ruip;
int error;
euid = uap->euid;
ruid = uap->ruid;
mtx_lock(&Giant);
newcred = crget();
+ euip = uifind(euid);
+ ruip = uifind(ruid);
PROC_LOCK(p);
oldcred = p->p_ucred;
if (((ruid != (uid_t)-1 && ruid != oldcred->cr_ruid &&
@@ -872,17 +885,19 @@
euid != oldcred->cr_ruid && euid != oldcred->cr_svuid)) &&
(error = suser_cred(oldcred, PRISON_ROOT)) != 0) {
PROC_UNLOCK(p);
+ uifree(ruip);
+ uifree(euip);
crfree(newcred);
mtx_unlock(&Giant);
return (error);
}
crcopy(newcred, oldcred);
if (euid != (uid_t)-1 && oldcred->cr_uid != euid) {
- change_euid(newcred, euid);
+ change_euid(newcred, euip);
setsugid(p);
}
if (ruid != (uid_t)-1 && oldcred->cr_ruid != ruid) {
- change_ruid(newcred, ruid);
+ change_ruid(newcred, ruip);
setsugid(p);
}
if ((ruid != (uid_t)-1 || newcred->cr_uid != newcred->cr_ruid) &&
@@ -892,6 +907,8 @@
}
p->p_ucred = newcred;
PROC_UNLOCK(p);
+ uifree(ruip);
+ uifree(euip);
crfree(oldcred);
mtx_unlock(&Giant);
return (0);
@@ -975,6 +992,7 @@
struct proc *p = td->td_proc;
struct ucred *newcred, *oldcred;
uid_t euid, ruid, suid;
+ struct uidinfo *euip, *ruip;
int error;
euid = uap->euid;
@@ -982,6 +1000,8 @@
suid = uap->suid;
mtx_lock(&Giant);
newcred = crget();
+ euip = uifind(euid);
+ ruip = uifind(ruid);
PROC_LOCK(p);
oldcred = p->p_ucred;
if (((ruid != (uid_t)-1 && ruid != oldcred->cr_ruid &&
@@ -995,6 +1015,8 @@
suid != oldcred->cr_uid)) &&
(error = suser_cred(oldcred, PRISON_ROOT)) != 0) {
PROC_UNLOCK(p);
+ uifree(ruip);
+ uifree(euip);
crfree(newcred);
mtx_unlock(&Giant);
return (error);
@@ -1002,11 +1024,11 @@
crcopy(newcred, oldcred);
if (euid != (uid_t)-1 && oldcred->cr_uid != euid) {
- change_euid(newcred, euid);
+ change_euid(newcred, euip);
setsugid(p);
}
if (ruid != (uid_t)-1 && oldcred->cr_ruid != ruid) {
- change_ruid(newcred, ruid);
+ change_ruid(newcred, ruip);
setsugid(p);
}
if (suid != (uid_t)-1 && oldcred->cr_svuid != suid) {
@@ -1015,6 +1037,8 @@
}
p->p_ucred = newcred;
PROC_UNLOCK(p);
+ uifree(ruip);
+ uifree(euip);
crfree(oldcred);
mtx_unlock(&Giant);
return (0);
@@ -1874,12 +1898,13 @@
* duration of the call.
*/
void
-change_euid(struct ucred *newcred, uid_t euid)
+change_euid(struct ucred *newcred, struct uidinfo *euip)
{
- newcred->cr_uid = euid;
+ newcred->cr_uid = euip->ui_uid;
+ uihold(euip);
uifree(newcred->cr_uidinfo);
- newcred->cr_uidinfo = uifind(euid);
+ newcred->cr_uidinfo = euip;
}
/*-
@@ -1904,13 +1929,14 @@
* duration of the call.
*/
void
-change_ruid(struct ucred *newcred, uid_t ruid)
+change_ruid(struct ucred *newcred, struct uidinfo *ruip)
{
(void)chgproccnt(newcred->cr_ruidinfo, -1, 0);
- newcred->cr_ruid = ruid;
+ newcred->cr_ruid = ruip->ui_uid;
+ uihold(ruip);
uifree(newcred->cr_ruidinfo);
- newcred->cr_ruidinfo = uifind(ruid);
+ newcred->cr_ruidinfo = ruip;
(void)chgproccnt(newcred->cr_ruidinfo, 1, 0);
}
Index: sys/sys/ucred.h
===================================================================
RCS file: /home/ncvs/src/sys/sys/ucred.h,v
retrieving revision 1.34
diff -u -r1.34 ucred.h
--- sys/sys/ucred.h 7 Apr 2002 03:59:31 -0000 1.34
+++ sys/sys/ucred.h 18 Jun 2002 18:50:31 -0000
@@ -85,9 +85,9 @@
#endif
void cred_update_thread(struct thread *td);
void change_egid(struct ucred *newcred, gid_t egid);
-void change_euid(struct ucred *newcred, uid_t euid);
+void change_euid(struct ucred *newcred, struct uidinfo *euip);
void change_rgid(struct ucred *newcred, gid_t rgid);
-void change_ruid(struct ucred *newcred, uid_t ruid);
+void change_ruid(struct ucred *newcred, struct uidinfo *ruip);
void change_svgid(struct ucred *newcred, gid_t svgid);
void change_svuid(struct ucred *newcred, uid_t svuid);
void crcopy(struct ucred *dest, struct ucred *src);
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200206190554.g5J5sGM1064829>