NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/41158: nfs_rename() locking against myself
On Mon, Apr 06, 2009 at 10:20:00AM +0000, Manuel Bouyer wrote:
> >Description:
> A netbsd 5.0_RC3 NFS server with a DIAGNOSTIC+LOCKDEBUG kernel
> will occasionally panic with
> Mutex error: lockdebug_wantlock: locking against myself
>
> lock address : 0x00000000ce95a02c type : sleep/adaptive
> initialized : 0x00000000c03af442
> shared holds : 0 exclusive: 1
> shares wanted: 0 exclusive: 7
> current cpu : 1 last held: 0
> current lwp : 0x00000000cea49060 last held: 0x00000000cea49060
> last locked : 0x00000000c03b94b4 unlocked : 0x00000000c025f3c0
> owner field : 0x00000000cea49060 wait/spin: 1/0
>
> Turnstile chain at 0xc071df60.
> => Turnstile at 0xcbf88068 (wrq=0xcbf88078, rdq=0xcbf88080).
> => 0 waiting readers:
> => 6 waiting writers: 0xcea59800 0xce4867a0 0xcea49ce0 0xcea17040 0xcea172c0 0
> xcea17a40
>
> panic: LOCKDEBUG
> fatal breakpoint trap in supervisor mode
> trap type 1 code 0 eip c03fb65c cs 8 eflags 246 cr2 cdc88000 ilevel 0
> Stopped in pid 208.1 (nfsd) at netbsd:breakpoint+0x4: popl %ebp
> db{1}> tr
> breakpoint(c0652a42,ce95d728,c2d08800,c03640cf,0,1,0,0,ce95d728,8) at
> netbsd:breakpoint+0x4
> panic(c0652a44,c064e7f4,c0523e0f,c064e7c3,5748,1a49060,0,cf69cc44,0,ce95a02c)
> at netbsd:panic+0x1b0
> lockdebug_abort1(c064e7c3,1,0,0,cbf6d508,cea49218,0,6,d35490ac,ce95db10) at
> netbsd:lockdebug_abort1+0xbb
> mutex_vector_enter(ce95a02c,8,ce95db6c,c025e587,ce95a004,0,cec93000,ce8df000,c3b6a100,ce95db58)
> at netbsd:mutex_vector_enter+0x464
> genfs_renamelock_enter(ce95a004,0,cec93000,ce8df000,c3b6a100,ce95db58,ce95db54,ce95db44,cea49060,0)
> at netbsd:genfs_renamelock_enter+0x14
> nfsrv_rename(cec18e10,ce8df000,cea49060,ce95dbd0,cd137b00,c0713d58,0,c06aed18,c0713d58,0)
> at netbsd:nfsrv_rename+0x4b7
There is a way to exit nfsrv_rename() without genfs_renamelock_exit(),
though the call to macro nfsm_reply(), or macro calling it (nfsm_srvmtofh()
is my suspect here).
My proposed fix (see attached patch) it to change nfsm_reply() to use
a 'error = 0; goto nfsmout' instead of return (0).
I most use it's equivalent because the function use nfsm_srvdone.
The place where it matters are:
nfsrv_create(): it could fix a bug here because we could exit the
function without vrele(dirp). Someone familiar with the VOP layer
should confirm that nfsreplyabort: is DTRT in this case (i.e. for all
calls to nfsm_reply()). There are some places here where a return(0)
is done without a vrele(dirp), I don't know if it's correct.
nfsrv_mknod(), nfsrv_symlink(), nfsrv_mkdir(): it's easier because there's
only 2 calls to nfsm_reply().
nfsrv_rename(): this is where it's interesting :) I think nfsmout: will
do it. Could there be a missing VOP_ABORTOP(tond)/vrele(tvp) in
the nfsmout: case ?
Then there are the macros using nfsm_reply(): nfsm_srvnamesiz is
always called at the top of the function; it's easy.
nfsm_srvmtofh() is a bit more difficult because of its use in
nfsrv_writegather(). nfsm_dissect() is called immediatly after, so using
the same nfsmout should be OK.
--
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
NetBSD: 26 ans d'experience feront toujours la difference
--
Index: nfs_serv.c
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfs_serv.c,v
retrieving revision 1.138
diff -u -r1.138 nfs_serv.c
--- nfs_serv.c 28 Mar 2008 05:02:08 -0000 1.138
+++ nfs_serv.c 8 Apr 2009 18:46:50 -0000
@@ -1383,7 +1383,7 @@
int32_t t1;
char *bpos;
int error = 0, cache = 0, len, tsize, dirfor_ret = 1, diraft_ret = 1;
- int rdev = 0;
+ int rdev = 0, abort = 0;
int v3 = (nfsd->nd_flag & ND_NFSV3), how, exclusive_flag = 0;
char *cp2;
struct mbuf *mb, *mreq;
@@ -1410,6 +1410,7 @@
vrele(dirp);
return (0);
}
+ abort = 1;
VATTR_NULL(&va);
if (v3) {
va.va_mode = 0;
@@ -1485,6 +1486,7 @@
KAUTH_SYSTEM_MKNOD, 0, NULL, NULL, NULL))) {
VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
vput(nd.ni_dvp);
+ abort = 0;
nfsm_reply(0);
return (error);
} else
@@ -1500,12 +1502,14 @@
vrele(nd.ni_dvp);
VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
error = EINVAL;
+ abort = 0;
nfsm_reply(0);
}
} else {
VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
vput(nd.ni_dvp);
error = ENXIO;
+ abort = 0;
}
vp = nd.ni_vp;
} else {
@@ -1515,6 +1519,7 @@
vrele(nd.ni_dvp);
else
vput(nd.ni_dvp);
+ abort = 0;
if (!error && va.va_size != -1) {
error = nfsrv_access(vp, VWRITE, cred,
(nd.ni_cnd.cn_flags & RDONLY), lwp, 0);
@@ -1552,7 +1557,9 @@
}
if (dirp) {
vrele(dirp);
+ dirp = NULL;
}
+ abort = 0;
nfsm_reply(NFSX_SRVFH(&nsfh, v3) + NFSX_FATTR(v3) + NFSX_WCCDATA(v3));
if (v3) {
if (!error) {
@@ -1569,13 +1576,15 @@
nfsmout:
if (dirp)
vrele(dirp);
- VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
- if (nd.ni_dvp == nd.ni_vp)
- vrele(nd.ni_dvp);
- else
- vput(nd.ni_dvp);
- if (nd.ni_vp)
- vput(nd.ni_vp);
+ if (abort) {
+ VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
+ if (nd.ni_dvp == nd.ni_vp)
+ vrele(nd.ni_dvp);
+ else
+ vput(nd.ni_dvp);
+ if (nd.ni_vp)
+ vput(nd.ni_vp);
+ }
return (error);
}
@@ -1599,6 +1608,7 @@
int32_t t1;
char *bpos;
int error = 0, cache = 0, len, dirfor_ret = 1, diraft_ret = 1;
+ int abort = 0;
u_int32_t major, minor;
enum vtype vtyp;
char *cp2;
@@ -1624,6 +1634,7 @@
vrele(dirp);
return (0);
}
+ abort = 1;
nfsm_dissect(tl, u_int32_t *, NFSX_UNSIGNED);
vtyp = nfsv3tov_type(*tl);
if (vtyp != VCHR && vtyp != VBLK && vtyp != VSOCK && vtyp != VFIFO) {
@@ -1695,7 +1706,9 @@
if (dirp) {
diraft_ret = VOP_GETATTR(dirp, &diraft, cred);
vrele(dirp);
+ dirp = NULL;
}
+ abort = 0;
nfsm_reply(NFSX_SRVFH(&nsfh, true) + NFSX_POSTOPATTR(1) +
NFSX_WCCDATA(1));
if (!error) {
@@ -1705,13 +1718,15 @@
nfsm_srvwcc_data(dirfor_ret, &dirfor, diraft_ret, &diraft);
return (0);
nfsmout:
- VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
- if (nd.ni_dvp == nd.ni_vp)
- vrele(nd.ni_dvp);
- else
- vput(nd.ni_dvp);
- if (nd.ni_vp)
- vput(nd.ni_vp);
+ if (abort) {
+ VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
+ if (nd.ni_dvp == nd.ni_vp)
+ vrele(nd.ni_dvp);
+ else
+ vput(nd.ni_dvp);
+ if (nd.ni_vp)
+ vput(nd.ni_vp);
+ }
if (dirp)
vrele(dirp);
return (error);
@@ -2009,21 +2024,26 @@
}
vrele(tond.ni_startdir);
PNBUF_PUT(tond.ni_cnd.cn_pnbuf);
+ tond.ni_cnd.cn_nameiop = 0;
out1:
if (fdirp) {
if (v3) {
fdiraft_ret = VOP_GETATTR(fdirp, &fdiraft, cred);
}
vrele(fdirp);
+ fdirp = NULL;
}
if (tdirp) {
if (v3) {
tdiraft_ret = VOP_GETATTR(tdirp, &tdiraft, cred);
}
vrele(tdirp);
+ tdirp = NULL;
}
vrele(fromnd.ni_startdir);
PNBUF_PUT(fromnd.ni_cnd.cn_pnbuf);
+ fromnd.ni_cnd.cn_nameiop = 0;
+ localfs = NULL;
nfsm_reply(2 * NFSX_WCCDATA(v3));
if (v3) {
nfsm_srvwcc_data(fdirfor_ret, &fdirfor, fdiraft_ret, &fdiraft);
@@ -2170,7 +2190,7 @@
char *bpos, *pathcp = NULL, *cp2;
struct uio io;
struct iovec iv;
- int error = 0, cache = 0, dirfor_ret = 1, diraft_ret = 1;
+ int error = 0, cache = 0, dirfor_ret = 1, diraft_ret = 1, abort = 0;
uint32_t len, len2;
int v3 = (nfsd->nd_flag & ND_NFSV3);
struct mbuf *mb, *mreq;
@@ -2191,6 +2211,7 @@
}
if (error)
goto out;
+ abort = 1;
VATTR_NULL(&va);
va.va_type = VLNK;
if (v3) {
@@ -2255,7 +2276,9 @@
diraft_ret = VOP_GETATTR(dirp, &diraft, cred);
}
vrele(dirp);
+ dirp = NULL;
}
+ abort = 0;
nfsm_reply(NFSX_SRVFH(&nsfh, v3) + NFSX_POSTOPATTR(v3) +
NFSX_WCCDATA(v3));
if (v3) {
@@ -2267,13 +2290,15 @@
}
return (0);
nfsmout:
- VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
- if (nd.ni_dvp == nd.ni_vp)
- vrele(nd.ni_dvp);
- else
- vput(nd.ni_dvp);
- if (nd.ni_vp)
- vrele(nd.ni_vp);
+ if (abort) {
+ VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
+ if (nd.ni_dvp == nd.ni_vp)
+ vrele(nd.ni_dvp);
+ else
+ vput(nd.ni_dvp);
+ if (nd.ni_vp)
+ vrele(nd.ni_vp);
+ }
if (dirp)
vrele(dirp);
if (pathcp)
@@ -2303,6 +2328,7 @@
int32_t t1;
char *bpos;
int error = 0, cache = 0, len, dirfor_ret = 1, diraft_ret = 1;
+ int abort = 0;
int v3 = (nfsd->nd_flag & ND_NFSV3);
char *cp2;
struct mbuf *mb, *mreq;
@@ -2327,6 +2353,7 @@
vrele(dirp);
return (0);
}
+ abort = 1;
VATTR_NULL(&va);
if (v3) {
va.va_mode = 0;
@@ -2362,7 +2389,9 @@
diraft_ret = VOP_GETATTR(dirp, &diraft, cred);
}
vrele(dirp);
+ dirp = NULL;
}
+ abort = 0;
nfsm_reply(NFSX_SRVFH(&nsfh, v3) + NFSX_POSTOPATTR(v3) +
NFSX_WCCDATA(v3));
if (v3) {
@@ -2378,13 +2407,15 @@
}
return (0);
nfsmout:
- VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
- if (nd.ni_dvp == nd.ni_vp)
- vrele(nd.ni_dvp);
- else
- vput(nd.ni_dvp);
- if (nd.ni_vp)
- vrele(nd.ni_vp);
+ if (abort) {
+ VOP_ABORTOP(nd.ni_dvp, &nd.ni_cnd);
+ if (nd.ni_dvp == nd.ni_vp)
+ vrele(nd.ni_dvp);
+ else
+ vput(nd.ni_dvp);
+ if (nd.ni_vp)
+ vrele(nd.ni_vp);
+ }
if (dirp)
vrele(dirp);
return (error);
@@ -3395,6 +3426,7 @@
u_quad_t frev;
nfsm_reply(0);
+nfsmout:
return (0);
}
@@ -3417,6 +3449,7 @@
else
error = EPROCUNAVAIL;
nfsm_reply(0);
+nfsmout:
return (0);
}
Index: nfsm_subs.h
===================================================================
RCS file: /cvsroot/src/sys/nfs/nfsm_subs.h,v
retrieving revision 1.50
diff -u -r1.50 nfsm_subs.h
--- nfsm_subs.h 4 Mar 2007 06:03:38 -0000 1.50
+++ nfsm_subs.h 8 Apr 2009 18:46:50 -0000
@@ -444,8 +444,10 @@
} \
mreq = *mrq; \
if (error && (!(nfsd->nd_flag & ND_NFSV3) || \
- error == EBADRPC)) \
- return(0); \
+ error == EBADRPC)) {\
+ error = 0; \
+ goto nfsmout; \
+ } \
}
#define nfsm_writereply(s, v3) \
Home |
Main Index |
Thread Index |
Old Index