Date: Mon, 17 Jun 2002 00:51:31 -0700 (PDT) From: Julian Elischer <julian@elischer.org> To: Bruce Evans <bde@zeta.org.au> Cc: Peter Jeremy <peter.jeremy@alcatel.com.au>, freebsd-current@FreeBSD.ORG Subject: Re: proc-args (M_PARGS) leakage Message-ID: <Pine.BSF.4.21.0206170050590.11814-100000@InterJet.elischer.org> In-Reply-To: <20020617155522.X3493-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce, if you've worked all this out, could you commit it? On Mon, 17 Jun 2002, Bruce Evans wrote: > 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 > 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?Pine.BSF.4.21.0206170050590.11814-100000>