Date: Mon, 17 Jun 2002 17:19:59 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Peter Jeremy <peter.jeremy@alcatel.com.au> Cc: freebsd-current@FreeBSD.ORG Subject: Re: proc-args (M_PARGS) leakage Message-ID: <20020617155522.X3493-100000@gamplex.bde.org> In-Reply-To: <20020617131502.O680@gsmx07.alcatel.com.au>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 17 Jun 2002, Peter Jeremy wrote:
> Having noticed that my system is paging far more than I would have
> expected, I went looking and found that the 'proc-args' pool was
> far larger than I expected. And is growing over time:
>
> gsmx07# vmstat -m|grep proc-args
> proc-args701802 70634K 70634K 1589264 16,32,64,128,256
> [about 10 minutes delay]
> gsmx07# vmstat -m|grep proc-args;vmstat -m|grep proc-args
> proc-args702048 70652K 70652K 1589557 16,32,64,128,256
> proc-args702047 70652K 70652K 1589558 16,32,64,128,256
> gsmx07#
I see a relatively slow growth on a fairly idle machine (4031K after
3 days of uptime).
> Whilst I'm fairly certain it's not my problem, sysctl_kern_proc_args()
> (1.136) looks dubious:
> ...
> PROC_LOCK(p);
> pa = p->p_args;
> pargs_hold(pa);
> PROC_UNLOCK(p);
> if (req->oldptr && pa != NULL) {
> error = SYSCTL_OUT(req, pa->ar_args, pa->ar_length);
> }
> if (req->newptr == NULL) {
> pargs_drop(pa);
> return (error);
> }
> To this point, it all looks correct: An additional reference has been
> added to p_args to allow the SYSCTL_OUT() to copy the arguments without
> them being freed. The relevant pargs entry will have a ref count of at
> least 2 (the original reference from 'p' and a new reference via
> pargs_hold()).
>
> PROC_LOCK(p);
> pa = p->p_args;
> p->p_args = NULL;
> PROC_UNLOCK(p);
> pargs_drop(pa);
>
> (And later code shows pa dead at this point). I don't follow this.
> pargs_drop(pa) deletes a single reference count - which matches the
> line "p->p_args = NULL;" - but I don't see anything to match the
> pargs_hold(pa) above.
Yes, this seems to be missing a pargs_drop() to drop the reference that
we have just gained. The corresponding code in RELENG_4 uses
--p->p_args->ar_ref to drop the main reference. This was sufficient since
RELENG_4 doesn't aquire another reference. I think the above should be
(after fixing some other bugs (see below) and some style bugs):
if (req->oldptr != NULL) {
if (pa != NULL)
error = SYSCTL_OUT(req, pa->ar_args, pa->ar_length);
else
dont_forget_to_set_output_parameters();
}
pargs_drop(pa);
if (error != 0 || req->newptr == NULL)
return (error);
#ifdef foot_shooting
/*
* This just loses if we can't read the new args. It doesn't even
* help in the non-error case, since setting p->p_args to NULL
* here doesn't keep it NULL after we release the proc lock.
*/
PROC_LOCK(p);
pa = p->p_args;
p->p_args = NULL;
PROC_UNLOCK(p);
pargs_drop(pa);
#endif
Other bugs:
if (req->oldptr && pa != NULL) {
error = SYSCTL_OUT(req, pa->ar_args, pa->ar_length);
}
This fails to set the output parameters in the req->oldptr != NULL case.
It also has some style bugs (only one pointer explicitly compared with NULL,
and verbose braces).
if (req->newptr == NULL) {
pargs_drop(pa);
return (error);
}
This should return if error != 0.
Otherwise OK until here, as above. We return if we are just reading
the proc table. I would have though that this was the usual case, but
on my fairly idle system it is 100% unusual: this sysctl is only used
by sendmail to write sometime.
PROC_LOCK(p);
pa = p->p_args;
p->p_args = NULL;
PROC_UNLOCK(p);
pargs_drop(pa);
This throws away p->p_args, so we should be sure that we have a new
p->p_args before doing it ...
if (req->newlen + sizeof(struct pargs) > ps_arg_cache_limit)
return (error);
... but here we don't even do this simple error check first. We also
return a garbage error code (normally 0).
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);
This leaks p->p_args in the unlikely event that p->p_args becomes non-NULL
after we have set it to NULL. The code under '#ifdef foot_shooting" should
be merged here to fix this.
} else
pargs_free(pa);
return (error);
The else clause is for the error case. Again, we have thown away
p->p_args too early.
Bruce
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?20020617155522.X3493-100000>