From 8899023f664718b49e85db30bfc35f79481fce32 Mon Sep 17 00:00:00 2001 From: Alfred Perlstein Date: Wed, 27 Mar 2002 21:36:18 +0000 Subject: [PATCH] Make the reference counting of 'struct pargs' SMP safe. There is still some locations where the PROC lock should be held in order to prevent inconsistent views from outside (like the proc->p_fd fix for kern/vfs_syscalls.c:checkdirs()) that can be fixed later. Submitted by: Jonathan Mini --- sys/compat/svr4/svr4_misc.c | 3 +-- sys/kern/kern_exec.c | 8 ++---- sys/kern/kern_exit.c | 3 +-- sys/kern/kern_fork.c | 3 +-- sys/kern/kern_proc.c | 12 ++++----- sys/sys/proc.h | 51 +++++++++++++++++++++++++++++++++++++ 6 files changed, 61 insertions(+), 19 deletions(-) diff --git a/sys/compat/svr4/svr4_misc.c b/sys/compat/svr4/svr4_misc.c index 33c679a7f87..3780fd046d6 100644 --- a/sys/compat/svr4/svr4_misc.c +++ b/sys/compat/svr4/svr4_misc.c @@ -1330,8 +1330,7 @@ loop: /* * Remove unused arguments */ - if (q->p_args && --q->p_args->ar_ref == 0) - FREE(q->p_args, M_PARGS); + pargs_drop(q->p_args); PROC_UNLOCK(q); /* diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c index 8e88de6084b..fe0be736b44 100644 --- a/sys/kern/kern_exec.c +++ b/sys/kern/kern_exec.c @@ -441,8 +441,7 @@ interpret: pa = p->p_args; p->p_args = NULL; PROC_UNLOCK(p); - if (pa != NULL && --pa->ar_ref == 0) - FREE(pa, M_PARGS); + pargs_drop(pa); /* Set values passed into the program in registers. */ setregs(td, imgp->entry_addr, (u_long)(uintptr_t)stack_base, @@ -451,10 +450,7 @@ interpret: /* Cache arguments if they fit inside our allowance */ i = imgp->endargs - imgp->stringbase; if (ps_arg_cache_limit >= i + sizeof(struct pargs)) { - MALLOC(pa, struct pargs *, sizeof(struct pargs) + i, - M_PARGS, M_WAITOK); - pa->ar_ref = 1; - pa->ar_length = i; + pa = pargs_alloc(i); bcopy(imgp->stringbase, pa->ar_args, i); PROC_LOCK(p); p->p_args = pa; diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c index 6e38fd0a857..172fd107c6c 100644 --- a/sys/kern/kern_exit.c +++ b/sys/kern/kern_exit.c @@ -637,8 +637,7 @@ loop: /* * Remove unused arguments */ - if (p->p_args && --p->p_args->ar_ref == 0) - FREE(p->p_args, M_PARGS); + pargs_drop(p->p_args); if (--p->p_procsig->ps_refcnt == 0) { if (p->p_sigacts != &p->p_uarea->u_sigacts) diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index db4a4590bae..ddd962a6637 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -482,8 +482,7 @@ again: p2->p_ucred = crhold(p1->p_ucred); td2->td_ucred = crhold(p2->p_ucred); /* XXXKSE */ - if (p2->p_args) - p2->p_args->ar_ref++; + pargs_hold(p2->p_args); if (flags & RFSIGSHARE) { p2->p_procsig = p1->p_procsig; diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c index 0e236cbaf1e..b81141bbc70 100644 --- a/sys/kern/kern_proc.c +++ b/sys/kern/kern_proc.c @@ -80,6 +80,7 @@ struct proclist zombproc; struct sx allproc_lock; struct sx proctree_lock; struct sx pgrpsess_lock; +struct mtx pargs_ref_lock; uma_zone_t proc_zone; uma_zone_t ithread_zone; @@ -94,6 +95,7 @@ procinit() sx_init(&allproc_lock, "allproc"); sx_init(&proctree_lock, "proctree"); sx_init(&pgrpsess_lock, "pgrpsess"); + mtx_init(&pargs_ref_lock, "struct pargs.ref", MTX_DEF); LIST_INIT(&allproc); LIST_INIT(&zombproc); pidhashtbl = hashinit(maxproc / 4, M_PROC, &pidhash); @@ -982,23 +984,19 @@ sysctl_kern_proc_args(SYSCTL_HANDLER_ARGS) pa = p->p_args; p->p_args = NULL; PROC_UNLOCK(p); - if (pa != NULL && --pa->ar_ref == 0) - FREE(pa, M_PARGS); + pargs_drop(pa); if (req->newlen + sizeof(struct pargs) > ps_arg_cache_limit) return (error); - MALLOC(pa, struct pargs *, sizeof(struct pargs) + req->newlen, - M_PARGS, M_WAITOK); - pa->ar_ref = 1; - pa->ar_length = req->newlen; + pa = pargs_alloc(req->newlen); error = SYSCTL_IN(req, pa->ar_args, req->newlen); if (!error) { PROC_LOCK(p); p->p_args = pa; PROC_UNLOCK(p); } else - FREE(pa, M_PARGS); + pargs_free(pa); return (error); } diff --git a/sys/sys/proc.h b/sys/sys/proc.h index e3e0cf8e996..d520d634acb 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -55,6 +55,8 @@ #else #include #endif +#include +#include #include #include /* Machine-dependent proc substruct. */ #include @@ -667,6 +669,10 @@ sigonstack(size_t sp) (--(p)->p_lock); \ } while (0) +/* Lock and unlock process arguments. */ +#define PARGS_LOCK(p) mtx_lock(&pargs_ref_lock) +#define PARGS_UNLOCK(p) mtx_unlock(&pargs_ref_lock) + #define PIDHASH(pid) (&pidhashtbl[(pid) & pidhash]) extern LIST_HEAD(pidhashhead, proc) *pidhashtbl; extern u_long pidhash; @@ -678,6 +684,7 @@ extern u_long pgrphash; extern struct sx allproc_lock; extern struct sx proctree_lock; extern struct sx pgrpsess_lock; +extern struct mtx pargs_ref_lock; extern struct proc proc0; /* Process slot for swapper. */ extern struct thread thread0; /* Primary thread in proc0 */ extern int hogticks; /* Limit on kernel cpu hogs. */ @@ -700,6 +707,50 @@ extern uma_zone_t proc_zone; extern int lastpid; +static __inline struct pargs * +pargs_alloc(int len) +{ + struct pargs *pa; + + MALLOC(pa, struct pargs *, sizeof(struct pargs) + len, M_PARGS, + M_WAITOK); + pa->ar_ref = 1; + pa->ar_length = len; + return (pa); +} + +static __inline void +pargs_free(struct pargs *pa) +{ + + FREE(pa, M_PARGS); +} + +static __inline void +pargs_hold(struct pargs *pa) +{ + + if (pa == NULL) + return; + PARGS_LOCK(pa); + pa->ar_ref++; + PARGS_UNLOCK(pa); +} + +static __inline void +pargs_drop(struct pargs *pa) +{ + + if (pa == NULL) + return; + PARGS_LOCK(pa); + if (--pa->ar_ref == 0) { + PARGS_UNLOCK(pa); + pargs_free(pa); + } else + PARGS_UNLOCK(pa); +} + /* * XXX macros for scheduler. Shouldn't be here, but currently needed for * bounding the dubious p_estcpu inheritance in wait1().