Date: Tue, 1 Jul 2014 13:56:12 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@FreeBSD.org> Subject: Re: svn commit: r268087 - head/sys/kern Message-ID: <20140701115612.GA26696@dft-labs.eu> In-Reply-To: <20140701114245.GO93733@kib.kiev.ua> References: <201407010921.s619LXHL063077@svn.freebsd.org> <20140701114245.GO93733@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Jul 01, 2014 at 02:42:45PM +0300, Konstantin Belousov wrote:
> 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.
Right, my bad. This was intended to be a minor cleanup, I'm happy to
revert if you want.
Note that current code relocks the vnode already, so there should be no
harm doing the same in 'else' case. (Although LK_RETRY looks somewhat
fishy in here.)
That said I propose the following:
diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index cce687b..9b3a99d 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
@@ -716,11 +716,11 @@ interpret:
VOP_UNLOCK(imgp->vp, 0);
setugidsafety(td);
error = fdcheckstd(td);
- vn_lock(imgp->vp, LK_SHARED | LK_RETRY);
if (error != 0)
goto done1;
newcred = crdup(oldcred);
euip = uifind(attr.va_uid);
+ vn_lock(imgp->vp, LK_SHARED | LK_RETRY);
PROC_LOCK(p);
/*
* Set the new credentials.
@@ -764,7 +764,9 @@ interpret:
if (oldcred->cr_svuid != oldcred->cr_uid ||
oldcred->cr_svgid != oldcred->cr_gid) {
PROC_UNLOCK(p);
+ VOP_UNLOCK(imgp->vp, 0);
newcred = crdup(oldcred);
+ vn_lock(imgp->vp, LK_SHARED | LK_RETRY);
PROC_LOCK(p);
change_svuid(newcred, newcred->cr_uid);
change_svgid(newcred, newcred->cr_gid);
@@ -841,6 +843,7 @@ interpret:
SDT_PROBE(proc, kernel, , exec__success, args->fname, 0, 0, 0, 0);
+ VOP_UNLOCK(imgp->vp, 0);
done1:
/*
* Free any resources malloc'd earlier that we didn't use.
@@ -849,7 +852,6 @@ done1:
uifree(euip);
if (newcred != NULL)
crfree(oldcred);
- VOP_UNLOCK(imgp->vp, 0);
/*
* Handle deferred decrement of ref counts.
--
Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140701115612.GA26696>