I've done some further work to update the preliminary nfsd patch, and
have corrected some additional problems with directory locking. The
basic problem was that in a number of places an existence test was being
done before locking the parent directory, contrary to the VFS
convention.
The patch adds a fh_lock_parent() routine to provide VFS semantics, and
then tests the return code before proceeding with a create operation. I
also found one place where an svc_fh structure was being used without
being initialized, which could have caused problems for certain error
exit paths. These combined changes should clear up the problems
associated with the "dentry not negative" messages.
Everything seems to be working well in testing here, but I'd appreciate
further testing by anyone using knfsd.
Regards,
Bill
--------------D03821EEA133BB2EB455297A
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/include/linux/nfsd/nfsd.h.old Thu May 21 13:44:20 1998
+++ linux-2.1.103/include/linux/nfsd/nfsd.h Sun May 31 13:18:00 1998
@@ -60,6 +60,7 @@
};
typedef int (*encode_dent_fn)(struct readdir_cd *, const char *,
int, off_t, ino_t);
+typedef int (*nfsd_dirop_t)(struct inode *, struct dentry *, int, int);
/*
* Procedure table for NFSv2
@@ -72,6 +73,8 @@
*/
int nfsd_svc(unsigned short port, int nrservs);
+/* nfsd/vfs.c */
+int fh_lock_parent(struct svc_fh *, struct dentry *);
void nfsd_racache_init(void);
int nfsd_lookup(struct svc_rqst *, struct svc_fh *,
const char *, int, struct svc_fh *);
--- linux-2.1.103/fs/nfsd/vfs.c.old Sun May 17 12:21:02 1998
+++ linux-2.1.103/fs/nfsd/vfs.c Sun May 31 13:18:56 1998
@@ -31,14 +31,13 @@
#include <linux/sunrpc/svc.h>
#include <linux/nfsd/nfsd.h>
+#include <linux/nfsd/nfsfh.h>
#include <linux/quotaops.h>
#if LINUX_VERSION_CODE >= 0x020100
#include <asm/uaccess.h>
#endif
-extern void fh_update(struct svc_fh*);
-
#define NFSDDBG_FACILITY NFSDDBG_FILEOP
/* Open mode for nfsd_open */
@@ -77,6 +76,41 @@
static struct raparms * raparm_cache = 0;
/*
+ * Lock a parent directory following the VFS locking protocol.
+ */
+int
+fh_lock_parent(struct svc_fh *parent_fh, struct dentry *dchild)
+{
+ int nfserr = 0;
+
+ fh_lock(parent_fh);
+ /*
+ * Make sure the parent->child relationship still holds,
+ * and that the child is still hashed.
+ */
+ if (dchild->d_parent != parent_fh->fh_dentry)
+ goto out_not_parent;
+ if (list_empty(&dchild->d_hash))
+ goto out_not_hashed;
+out:
+ return nfserr;
+
+out_not_parent:
+ printk(KERN_WARNING
+ "fh_lock_parent: %s/%s parent changed\n",
+ dchild->d_parent->d_name.name, dchild->d_name.name);
+ goto out_unlock;
+out_not_hashed:
+ printk(KERN_WARNING
+ "fh_lock_parent: %s/%s unhashed\n",
+ dchild->d_parent->d_name.name, dchild->d_name.name);
+out_unlock:
+ nfserr = nfserr_noent;
+ fh_unlock(parent_fh);
+ goto out;
+}
+
+/*
* Deny access to certain file systems
*/
static inline int
@@ -546,8 +580,11 @@
}
/*
- * Create a file (regular, directory, device, fifo).
- * UNIX sockets not yet implemented.
+ * 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. Note that the parent directory is left locked.
+ *
* N.B. Every call to nfsd_create needs an fh_put for _both_ fhp and resfhp
*/
int
@@ -557,13 +594,12 @@
{
struct dentry *dentry, *dchild;
struct inode *dirp;
+ nfsd_dirop_t opfunc = NULL;
int err;
err = nfserr_perm;
if (!flen)
goto out;
- if (!(iap->ia_valid & ATTR_MODE))
- iap->ia_mode = 0;
err = fh_verify(rqstp, fhp, S_IFDIR, MAY_CREATE);
if (err)
goto out;
@@ -571,64 +607,74 @@
dentry = fhp->fh_dentry;
dirp = dentry->d_inode;
- /* Get all the sanity checks out of the way before we lock the parent. */
err = nfserr_notdir;
if(!dirp->i_op || !dirp->i_op->lookup)
goto out;
- err = nfserr_perm;
- if (type == S_IFREG) {
- if(!dirp->i_op->create)
- goto out;
- } else if(type == S_IFDIR) {
- if(!dirp->i_op->mkdir)
- goto out;
- } else if((type == S_IFCHR) || (type == S_IFBLK) || (type == S_IFIFO)) {
- if(!dirp->i_op->mknod)
- goto out;
- } else {
- goto out;
- }
-
/*
- * 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);
err = PTR_ERR(dchild);
- if(IS_ERR(dchild))
+ if (IS_ERR(dchild))
goto out_nfserr;
fh_compose(resfhp, fhp->fh_export, dchild);
- } else
+ /* Lock the parent and check for errors ... */
+ err = fh_lock_parent(fhp, dchild);
+ if (err)
+ goto out;
+ } else {
dchild = resfhp->fh_dentry;
+ if (!fhp->fh_locked)
+ printk(KERN_ERR
+ "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 ...
*/
+ err = nfserr_exist;
if (dchild->d_inode) {
- printk("nfsd_create: dentry %s/%s not negative!\n",
- dentry->d_parent->d_name.name, dentry->d_name.name);
+ printk(KERN_WARNING
+ "nfsd_create: dentry %s/%s not negative!\n",
+ dentry->d_name.name, dchild->d_name.name);
+ goto out;
}
- /* Looks good, lock the directory. */
- fh_lock(fhp);
- DQUOT_INIT(dirp);
+ /*
+ * Get the dir op function pointer.
+ */
+ err = nfserr_perm;
switch (type) {
case S_IFREG:
- err = dirp->i_op->create(dirp, dchild, iap->ia_mode);
+ opfunc = (nfsd_dirop_t) dirp->i_op->create;
break;
case S_IFDIR:
- err = dirp->i_op->mkdir(dirp, dchild, iap->ia_mode);
+ opfunc = (nfsd_dirop_t) dirp->i_op->mkdir;
break;
case S_IFCHR:
case S_IFBLK:
case S_IFIFO:
- err = dirp->i_op->mknod(dirp, dchild, iap->ia_mode, rdev);
+ opfunc = dirp->i_op->mknod;
break;
}
- DQUOT_DROP(dirp);
- fh_unlock(fhp);
+ if (!opfunc)
+ goto out;
+ if (!(iap->ia_valid & ATTR_MODE))
+ iap->ia_mode = 0;
+
+ /*
+ * Call the dir op function to create the object.
+ */
+ DQUOT_INIT(dirp);
+ err = opfunc(dirp, dchild, iap->ia_mode, rdev);
+ DQUOT_DROP(dirp);
if (err < 0)
goto out_nfserr;
+
if (EX_ISSYNC(fhp->fh_export))
write_inode_now(dirp);
@@ -778,21 +824,29 @@
if (IS_ERR(dnew))
goto out_nfserr;
- err = -EEXIST;
+ /*
+ * Lock the parent before checking for existence
+ */
+ err = fh_lock_parent(fhp, dnew);
+ if (err)
+ goto out_compose;
+
+ err = nfserr_exist;
if (!dnew->d_inode) {
- fh_lock(fhp);
DQUOT_INIT(dirp);
err = dirp->i_op->symlink(dirp, dnew, path);
DQUOT_DROP(dirp);
- fh_unlock(fhp);
if (!err) {
if (EX_ISSYNC(fhp->fh_export))
write_inode_now(dirp);
- }
+ } else
+ err = nfserrno(-err);
}
+ fh_unlock(fhp);
+
+ /* Compose the fh so the dentry will be freed ... */
+out_compose:
fh_compose(resfhp, fhp->fh_export, dnew);
- if (err)
- goto out_nfserr;
out:
return err;
@@ -820,6 +874,10 @@
if (err)
goto out;
+ err = nfserr_perm;
+ if (!len)
+ goto out;
+
ddir = ffhp->fh_dentry;
dirp = ddir->d_inode;
@@ -827,44 +885,48 @@
err = PTR_ERR(dnew);
if (IS_ERR(dnew))
goto out_nfserr;
+ /*
+ * Lock the parent before checking for existence
+ */
+ err = fh_lock_parent(ffhp, dnew);
+ if (err)
+ goto out_dput;
- err = -EEXIST;
+ err = nfserr_exist;
if (dnew->d_inode)
- goto dput_and_out;
-
- err = -EPERM;
- if (!len)
- goto dput_and_out;
+ goto out_unlock;
dold = tfhp->fh_dentry;
dest = dold->d_inode;
- err = -EACCES;
+ err = nfserr_acces;
if (nfsd_iscovered(ddir, ffhp->fh_export))
- goto dput_and_out;
+ goto out_unlock;
+ /* FIXME: nxdev for NFSv3 */
if (dirp->i_dev != dest->i_dev)
- goto dput_and_out; /* FIXME: nxdev for NFSv3 */
+ goto out_unlock;
- err = -EPERM;
+ err = nfserr_perm;
if (IS_IMMUTABLE(dest) /* || IS_APPEND(dest) */ )
- goto dput_and_out;
+ goto out_unlock;
if (!dirp->i_op || !dirp->i_op->link)
- goto dput_and_out;
+ goto out_unlock;
- fh_lock(ffhp);
DQUOT_INIT(dirp);
err = dirp->i_op->link(dold, dirp, dnew);
DQUOT_DROP(dirp);
- fh_unlock(ffhp);
+ if (!err) {
+ if (EX_ISSYNC(ffhp->fh_export)) {
+ write_inode_now(dirp);
+ write_inode_now(dest);
+ }
+ } else
+ err = nfserrno(-err);
- if (!err && EX_ISSYNC(ffhp->fh_export)) {
- write_inode_now(dirp);
- write_inode_now(dest);
- }
-dput_and_out:
+out_unlock:
+ fh_unlock(ffhp);
+out_dput:
dput(dnew);
- if (err)
- goto out_nfserr;
out:
return err;
@@ -1005,7 +1067,10 @@
if (IS_ERR(rdentry))
goto out_nfserr;
- fh_lock(fhp);
+ err = fh_lock_parent(fhp, rdentry);
+ if (err)
+ goto out;
+
DQUOT_INIT(dirp);
if (type == S_IFDIR) {
err = -ENOTDIR;
--- linux-2.1.103/fs/nfsd/nfsproc.c.old Tue Jan 27 09:36:34 1998
+++ linux-2.1.103/fs/nfsd/nfsproc.c Sun May 31 13:35:04 1998
@@ -180,10 +180,8 @@
/*
* CREATE processing is complicated. The keyword here is `overloaded.'
- * There's a small race condition here between the check for existence
- * and the actual create() call, but one could even consider this a
- * feature because this only happens if someone else creates the file
- * at the same time.
+ * The parent directory is kept locked between the check for existence
+ * and the actual create() call in compliance with VFS protocols.
* N.B. After this call _both_ argp->fh and resp->fh need an fh_put
*/
static int
@@ -193,15 +191,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 +211,32 @@
} else if (nfserr)
goto done;
- /* First, check if the file already exists. */
- exists = !nfsd_lookup(rqstp, dirfhp, argp->name, argp->len, newfhp);
-
- if (newfhp->fh_dverified)
- inode = newfhp->fh_dentry->d_inode;
+ /*
+ * Do a lookup to verify the new filehandle.
+ */
+ nfserr = nfsd_lookup(rqstp, dirfhp, argp->name, argp->len, newfhp);
+ if (nfserr) {
+ if (nfserr != nfserr_noent)
+ goto done;
+ /*
+ * If the new filehandle wasn't verified, we can't tell
+ * whether the file exists or not. Time to bail ...
+ */
+ nfserr = nfserr_acces;
+ if (!newfhp->fh_dverified) {
+ printk(KERN_WARNING
+ "nfsd_proc_create: filehandle not verified\n");
+ goto done;
+ }
+ }
- /* Get rid of this soon... */
- if (exists && !inode) {
- printk("nfsd_proc_create: Wheee... exists but d_inode==NULL\n");
- nfserr = nfserr_rofs;
+ /*
+ * Lock the parent directory and check for existence.
+ */
+ nfserr = fh_lock_parent(dirfhp, newfhp->fh_dentry);
+ if (nfserr)
goto done;
- }
+ inode = newfhp->fh_dentry->d_inode;
/* Unfudge the mode bits */
if (attr->ia_valid & ATTR_MODE) {
@@ -233,7 +244,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 +254,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 +263,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 +277,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 +307,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);
@@ -359,9 +373,9 @@
int nfserr;
dprintk("nfsd: SYMLINK %p %s -> %s\n",
- SVCFH_DENTRY(&argp->ffh),
- argp->fname, argp->tname);
+ SVCFH_DENTRY(&argp->ffh), argp->fname, argp->tname);
+ memset(&newfh, 0, sizeof(struct svc_fh));
/*
* Create the link, look up new file and set attrs.
*/
@@ -386,12 +400,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);
--------------D03821EEA133BB2EB455297A--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu