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>