d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Close race conditions between fork() and [sg]etpriority()'s

Browse Source 
PRIO_USER case, possibly also other places that deferences
p_ucred.

In the past, we insert a new process into the allproc list right
after PID allocation, and release the allproc_lock sx.  Because
most content in new proc's structure is not yet initialized,
this could lead to undefined result if we do not handle PRS_NEW
with care.

The problem with PRS_NEW state is that it does not provide fine
grained information about how much initialization is done for a
new process.  By defination, after PRIO_USER setpriority(), all
processes that belongs to given user should have their nice value
set to the specified value.  Therefore, if p_{start,end}copy
section was done for a PRS_NEW process, we can not safely ignore
it because p_nice is in this area.  On the other hand, we should
be careful on PRS_NEW processes because we do not allow non-root
users to lower their nice values, and without a successful copy
of the copy section, we can get stale values that is inherted
from the uninitialized area of the process structure.

This commit tries to close the race condition by grabbing proc
mutex *before* we release allproc_lock xlock, and do copy as
well as zero immediately after the allproc_lock xunlock.  This
guarantees that the new process would have its p_copy and p_zero
sections, as well as user credential informaion initialized.  In
getpriority() case, instead of grabbing PROC_LOCK for a PRS_NEW
process, we just skip the process in question, because it does
not affect the final result of the call, as the p_nice value
would be copied from its parent, and we will see it during
allproc traverse.

Other potential solutions are still under evaluation.

Discussed with:	davidxu, jhb, rwatson
PR:		kern/108071
MFC after:	2 weeks
This commit is contained in:
Xin LI 2007-02-26 03:38:09 +00:00
parent 4813511138
commit 1ad9ee8603
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=167007
2 changed files with 17 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
sys/kern/kern_fork.c
View File

@ -423,8 +423,22 @@ fork1(td, flags, pages, procp)
AUDIT_ARG(pid, p2->p_pid);
LIST_INSERT_HEAD(&allproc, p2, p_list);
LIST_INSERT_HEAD(PIDHASH(p2->p_pid), p2, p_hash);
PROC_LOCK(p2);
PROC_LOCK(p1);
sx_xunlock(&allproc_lock);
bcopy(&p1->p_startcopy, &p2->p_startcopy,
__rangeof(struct proc, p_startcopy, p_endcopy));
PROC_UNLOCK(p1);
bzero(&p2->p_startzero,
__rangeof(struct proc, p_startzero, p_endzero));
p2->p_ucred = crhold(td->td_ucred);
PROC_UNLOCK(p2);
/*
* Malloc things while we don't hold any locks.
*/
@ -482,13 +496,9 @@ fork1(td, flags, pages, procp)
PROC_LOCK(p2);
PROC_LOCK(p1);
bzero(&p2->p_startzero,
__rangeof(struct proc, p_startzero, p_endzero));
bzero(&td2->td_startzero,
__rangeof(struct thread, td_startzero, td_endzero));
bcopy(&p1->p_startcopy, &p2->p_startcopy,
__rangeof(struct proc, p_startcopy, p_endcopy));
bcopy(&td->td_startcopy, &td2->td_startcopy,
__rangeof(struct thread, td_startcopy, td_endcopy));
@ -511,7 +521,6 @@ fork1(td, flags, pages, procp)
sched_fork(td, td2);
mtx_unlock_spin(&sched_lock);
p2->p_ucred = crhold(td->td_ucred);
td2->td_ucred = crhold(p2->p_ucred);
#ifdef AUDIT
audit_proc_fork(p1, p2);

3
sys/kern/kern_resource.c
View File

@ -143,6 +143,9 @@ getpriority(td, uap)
uap->who = td->td_ucred->cr_uid;
sx_slock(&allproc_lock);
FOREACH_PROC_IN_SYSTEM(p) {
/* Do not bother to check PRS_NEW processes */
if (p->p_state == PRS_NEW)
continue;
PROC_LOCK(p);
if (!p_cansee(td, p) &&
p->p_ucred->cr_uid == uap->who) {