Austin Donnelly wrote:
> Well, since I can reproduce this bug on demand, I'd be interested in
> testing any patches you feel like coming up with.
Hi Austin,
I've reworked the dentry locking for the nfsd create routine, and
hopefully this will take care of the problems you reported. The code now
locks the parent directory before checking for existence, and keeps the
parent locked through the call to the low-level create op.
The attached patch has been lightly tested and seems to behave
correctly, but let me know how it works out.
Regards,
Bill
--------------D1B5288D7D9167AE2471753A
Content-Type: text/plain; charset=us-ascii; name="nfsd_103-patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="nfsd_103-patch"
--- linux-2.1.103/fs/nfsd/vfs.c.old Sun May 17 12:21:02 1998
+++ linux-2.1.103/fs/nfsd/vfs.c Sat May 30 21:29:03 1998
@@ -548,6 +548,8 @@
/*
* Create a file (regular, directory, device, fifo).
* UNIX sockets not yet implemented.
+ * If the response fh has been verified, the parent directory should
+ * already be locked. The parent directory is left locked.
* N.B. Every call to nfsd_create needs an fh_put for _both_ fhp and resfhp
*/
int
@@ -590,7 +592,8 @@
}
/*
- * The response filehandle may have been setup already ...
+ * Check whether the response filehandle has been verified yet.
+ * If it has, the parent directory should already be locked.
*/
if (!resfhp->fh_dverified) {
dchild = lookup_dentry(fname, dget(dentry), 0);
@@ -598,18 +601,25 @@
if(IS_ERR(dchild))
goto out_nfserr;
fh_compose(resfhp, fhp->fh_export, dchild);
- } else
+ /* Looks good, lock the directory. */
+ fh_lock(fhp);
+ } else {
dchild = resfhp->fh_dentry;
+ if (!fhp->fh_locked)
+ printk("nfsd_create: parent %s/%s not locked!\n",
+ dentry->d_parent->d_name.name,
+ dentry->d_name.name);
+ }
/*
* Make sure the child dentry is still negative ...
*/
if (dchild->d_inode) {
printk("nfsd_create: dentry %s/%s not negative!\n",
- dentry->d_parent->d_name.name, dentry->d_name.name);
+ dentry->d_name.name, dchild->d_name.name);
+ err = nfserr_exist;
+ goto out;
}
- /* Looks good, lock the directory. */
- fh_lock(fhp);
DQUOT_INIT(dirp);
switch (type) {
case S_IFREG:
@@ -625,10 +635,9 @@
break;
}
DQUOT_DROP(dirp);
- fh_unlock(fhp);
-
if (err < 0)
goto out_nfserr;
+
if (EX_ISSYNC(fhp->fh_export))
write_inode_now(dirp);
--- linux-2.1.103/fs/nfsd/nfsproc.c.old Tue Jan 27 09:36:34 1998
+++ linux-2.1.103/fs/nfsd/nfsproc.c Sat May 30 21:54:02 1998
@@ -193,15 +193,14 @@
svc_fh *dirfhp = &argp->fh;
svc_fh *newfhp = &resp->fh;
struct iattr *attr = &argp->attrs;
- struct inode *inode = NULL;
- int nfserr, type, mode;
- int rdonly = 0, exists;
+ struct inode *inode;
+ int nfserr, type, mode, rdonly = 0;
dev_t rdev = NODEV;
dprintk("nfsd: CREATE %d/%ld %s\n",
SVCFH_DEV(dirfhp), SVCFH_INO(dirfhp), argp->name);
- /* Get the directory inode */
+ /* First verify the parent filehandle */
nfserr = fh_verify(rqstp, dirfhp, S_IFDIR, MAY_EXEC);
if (nfserr)
goto done; /* must fh_put dirfhp even on error */
@@ -214,18 +213,32 @@
} else if (nfserr)
goto done;
- /* First, check if the file already exists. */
- exists = !nfsd_lookup(rqstp, dirfhp, argp->name, argp->len, newfhp);
+ /*
+ * Now do a lookup to verify the new filehandle ...
+ */
+ nfserr = nfsd_lookup(rqstp, dirfhp, argp->name, argp->len, newfhp);
+ if (nfserr && nfserr != nfserr_noent)
+ goto done;
+ if (!newfhp->fh_dverified) {
+ printk(KERN_ERR "nfsd_proc_create: new fh not verified??\n");
+ nfserr = nfserr_perm;
+ goto done;
+ }
- if (newfhp->fh_dverified)
- inode = newfhp->fh_dentry->d_inode;
+ /*
+ * ... then lock the parent and check for existence.
+ * N.B. this should follow the VFS lock_parent model.
+ */
+ fh_lock(dirfhp);
- /* Get rid of this soon... */
- if (exists && !inode) {
- printk("nfsd_proc_create: Wheee... exists but d_inode==NULL\n");
- nfserr = nfserr_rofs;
- goto done;
- }
+ /* Make sure the parent->child relationship hasn't changed. */
+ if (newfhp->fh_dentry->d_parent != dirfhp->fh_dentry) {
+ printk(KERN_WARNING
+ "nfsd_proc_create: parent dir changed\n");
+ nfserr = nfserr_noent;
+ goto out_unlock;
+ }
+ inode = newfhp->fh_dentry->d_inode;
/* Unfudge the mode bits */
if (attr->ia_valid & ATTR_MODE) {
@@ -233,7 +246,7 @@
mode = attr->ia_mode & ~S_IFMT;
if (!type) /* HP weirdness */
type = S_IFREG;
- } else if (exists) {
+ } else if (inode) {
type = inode->i_mode & S_IFMT;
mode = inode->i_mode & ~S_IFMT;
} else {
@@ -243,8 +256,8 @@
/* This is for "echo > /dev/null" a la SunOS. Argh. */
nfserr = nfserr_rofs;
- if (rdonly && (!exists || type == S_IFREG))
- goto done;
+ if (rdonly && (!inode || type == S_IFREG))
+ goto out_unlock;
attr->ia_valid |= ATTR_MODE;
attr->ia_mode = type | mode;
@@ -252,7 +265,6 @@
/* Special treatment for non-regular files according to the
* gospel of sun micro
*/
- nfserr = 0;
if (type != S_IFREG) {
int is_borc = 0;
u32 size = attr->ia_size;
@@ -267,21 +279,21 @@
} else if (size != rdev) {
/* dev got truncated because of 16bit Linux dev_t */
nfserr = nfserr_io; /* or nfserr_inval? */
- goto done;
+ goto out_unlock;
} else {
/* Okay, char or block special */
is_borc = 1;
}
/* Make sure the type and device matches */
- if (exists && (type != (inode->i_mode & S_IFMT)
- || (is_borc && inode->i_rdev != rdev))) {
- nfserr = nfserr_exist;
- goto done;
- }
+ nfserr = nfserr_exist;
+ if (inode && (type != (inode->i_mode & S_IFMT) ||
+ (is_borc && inode->i_rdev != rdev)))
+ goto out_unlock;
}
- if (!exists) {
+ nfserr = 0;
+ if (!inode) {
/* File doesn't exist. Create it and set attrs */
nfserr = nfsd_create(rqstp, dirfhp, argp->name, argp->len,
attr, type, rdev, newfhp);
@@ -297,6 +309,10 @@
nfserr = nfsd_setattr(rqstp, newfhp, attr);
}
+out_unlock:
+ /* We don't really need to unlock, as fh_put does it. */
+ fh_unlock(dirfhp);
+
done:
fh_put(dirfhp);
RETURN(nfserr);
@@ -386,12 +402,13 @@
{
int nfserr;
- dprintk("nfsd: MKDIR %p %s\n",
- SVCFH_DENTRY(&argp->fh),
- argp->name);
+ dprintk("nfsd: MKDIR %p %s\n", SVCFH_DENTRY(&argp->fh), argp->name);
+
+ if (resp->fh.fh_dverified) {
+ printk(KERN_WARNING
+ "nfsd_proc_mkdir: response already verified??\n");
+ }
- /* N.B. what about the dentry count?? */
- resp->fh.fh_dverified = 0; /* paranoia */
nfserr = nfsd_create(rqstp, &argp->fh, argp->name, argp->len,
&argp->attrs, S_IFDIR, 0, &resp->fh);
fh_put(&argp->fh);
--------------D1B5288D7D9167AE2471753A--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu