svn commit: r268087 - head/sys/kern

Konstantin Belousov kostikbel at gmail.com
Tue Jul 1 11:42:52 UTC 2014


On Tue, Jul 01, 2014 at 09:21:33AM +0000, Mateusz Guzik wrote:
> Author: mjg
> Date: Tue Jul  1 09:21:32 2014
> New Revision: 268087

> URL: http://svnweb.freebsd.org/changeset/base/268087
> 
> Log:
>   Don't call crcopysafe or uifind unnecessarily in execve.
>   
>   MFC after:	1 week
> 
> Modified:
>   head/sys/kern/kern_exec.c
> 
> Modified: head/sys/kern/kern_exec.c
> ==============================================================================
> --- head/sys/kern/kern_exec.c	Tue Jul  1 08:36:56 2014	(r268086)
> +++ head/sys/kern/kern_exec.c	Tue Jul  1 09:21:32 2014	(r268087)
> @@ -336,7 +336,7 @@ do_execve(td, args, mac_p)
>  	struct proc *p = td->td_proc;
>  	struct nameidata nd;
>  	struct ucred *newcred = NULL, *oldcred;
> -	struct uidinfo *euip;
> +	struct uidinfo *euip = NULL;
>  	register_t *stack_base;
>  	int error, i;
>  	struct image_params image_params, *imgp;
> @@ -601,8 +601,6 @@ interpret:
>  	/*
>  	 * Malloc things before we need locks.
>  	 */
> -	newcred = crget();
> -	euip = uifind(attr.va_uid);
>  	i = imgp->args->begin_envv - imgp->args->begin_argv;
>  	/* Cache arguments if they fit inside our allowance */
>  	if (ps_arg_cache_limit >= i + sizeof(struct pargs)) {
> @@ -631,7 +629,7 @@ interpret:
>  	PROC_LOCK(p);
>  	if (oldsigacts)
>  		p->p_sigacts = newsigacts;
> -	oldcred = crcopysafe(p, newcred);
> +	oldcred = p->p_ucred;
>  	/* Stop profiling */
>  	stopprofclock(p);
>  
> @@ -721,6 +719,8 @@ interpret:
>  		vn_lock(imgp->vp, LK_SHARED | LK_RETRY);
>  		if (error != 0)
>  			goto done1;
> +		newcred = crdup(oldcred);
> +		euip = uifind(attr.va_uid);
>  		PROC_LOCK(p);
>  		/*
>  		 * Set the new credentials.
> @@ -745,7 +745,6 @@ interpret:
>  		change_svuid(newcred, newcred->cr_uid);
>  		change_svgid(newcred, newcred->cr_gid);
>  		p->p_ucred = newcred;
> -		newcred = NULL;
>  	} else {
>  		if (oldcred->cr_uid == oldcred->cr_ruid &&
>  		    oldcred->cr_gid == oldcred->cr_rgid)
> @@ -764,10 +763,12 @@ interpret:
>  		 */
>  		if (oldcred->cr_svuid != oldcred->cr_uid ||
>  		    oldcred->cr_svgid != oldcred->cr_gid) {
> +			PROC_UNLOCK(p);
> +			newcred = crdup(oldcred);
> +			PROC_LOCK(p);
>  			change_svuid(newcred, newcred->cr_uid);
>  			change_svgid(newcred, newcred->cr_gid);
>  			p->p_ucred = newcred;
> -			newcred = NULL;
>  		}
>  	}
>  
> @@ -844,11 +845,10 @@ done1:
>  	/*
>  	 * Free any resources malloc'd earlier that we didn't use.
>  	 */
> -	uifree(euip);
> -	if (newcred == NULL)
> +	if (euip != NULL)
> +		uifree(euip);
> +	if (newcred != NULL)
>  		crfree(oldcred);
> -	else
> -		crfree(newcred);
>  	VOP_UNLOCK(imgp->vp, 0);
>  
>  	/*

Old code did the malloc(M_WAITOK) call in crget() before the text vnode
was locked.  After your change, the crdup() is called with the vnode locked.
Witness would not tell you that anything is wrong there, but the new
code is worse than the previous structure, even if malloc() was sometimes
done when not needed.

To satisfy the memory request from malloc(), pagedaemon or laundry may
need to lock the vnode, which creates a circular dependency.  Pagedaemon
locks vnodes with timeout, which just means that it would not be able
to clean pages while execve() is stuck in malloc(M_WAITOK), while
laundry takes the vnode lock without timeout, hanging until the malloc
request is satisfied.

The rule is, do not allocate memory while vnodes are locked.  It is not
always followed, but it makes no sense to change existing correct code
to broke the pattern.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20140701/b5734d49/attachment.sig>


More information about the svn-src-head mailing list