nfs: don't open in ->d_revalidate

NFSv4 can't do reliable opens in d_revalidate, since it cannot know whether a mount needs to be followed or not. It does check d_mountpoint() on the dentry, which can result in a weird error if the VFS found that the mount does not in fact need to be followed, e.g.: # mount --bind /mnt/nfs /mnt/nfs-clone # echo something > /mnt/nfs/tmp/bar # echo x > /tmp/file # mount --bind /tmp/file /mnt/nfs-clone/tmp/bar # cat /mnt/nfs/tmp/bar cat: /mnt/nfs/tmp/bar: Not a directory Which should, by any sane filesystem, result in "something" being printed. So instead do the open in f_op->open() and in the unlikely case that the cached dentry turned out to be invalid, drop the dentry and return EOPENSTALE to let the VFS retry. Signed-off-by: Miklos Szeredi <mszeredi@suse.cz> CC: Trond Myklebust <Trond.Myklebust@netapp.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

nfs: don't open in ->d_revalidate
NFSv4 can't do reliable opens in d_revalidate, since it cannot know whether a mount needs to be followed or not. It does check d_mountpoint() on the dentry, which can result in a weird error if the VFS found that the mount does not in fact need to be followed, e.g.: # mount --bind /mnt/nfs /mnt/nfs-clone # echo something > /mnt/nfs/tmp/bar # echo x > /tmp/file # mount --bind /tmp/file /mnt/nfs-clone/tmp/bar # cat /mnt/nfs/tmp/bar cat: /mnt/nfs/tmp/bar: Not a directory Which should, by any sane filesystem, result in "something" being printed. So instead do the open in f_op->open() and in the unlikely case that the cached dentry turned out to be invalid, drop the dentry and return EOPENSTALE to let the VFS retry. Signed-off-by: Miklos Szeredi <mszeredi@suse.cz> CC: Trond Myklebust <Trond.Myklebust@netapp.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Miklos Szeredi · Al Viro
1 parent 16b1c1cd71
Showing 2 changed files with 78 additions and 55 deletions Side-by-side Diff
fs/nfs/dir.c
fs/nfs/file.c
@@ -1354,10 +1354,10 @@
 }
  
 #ifdef CONFIG_NFS_V4
-static int nfs_open_revalidate(struct dentry *, struct nameidata *);
+static int nfs4_lookup_revalidate(struct dentry *, struct nameidata *);
  
 const struct dentry_operations nfs4_dentry_operations = {
-	.d_revalidate	= nfs_open_revalidate,
+	.d_revalidate	= nfs4_lookup_revalidate,
 	.d_delete	= nfs_dentry_delete,
 	.d_iput		= nfs_dentry_iput,
 	.d_automount	= nfs_d_automount,
  
@@ -1519,13 +1519,11 @@
 	return nfs_lookup(dir, dentry, nd);
 }
  
-static int nfs_open_revalidate(struct dentry *dentry, struct nameidata *nd)
+static int nfs4_lookup_revalidate(struct dentry *dentry, struct nameidata *nd)
 {
 	struct dentry *parent = NULL;
 	struct inode *inode;
 	struct inode *dir;
-	struct nfs_open_context *ctx;
-	struct iattr attr;
 	int openflags, ret = 0;
  
 	if (nd->flags & LOOKUP_RCU)
  
  
  
@@ -1554,57 +1552,13 @@
 	/* We cannot do exclusive creation on a positive dentry */
 	if ((openflags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL))
 		goto no_open_dput;
-	/* We can't create new files here */
-	openflags &= ~(O_CREAT|O_EXCL);
  
-	ctx = create_nfs_open_context(dentry, openflags);
-	ret = PTR_ERR(ctx);
-	if (IS_ERR(ctx))
-		goto out;
+	/* Let f_op->open() actually open (and revalidate) the file */
+	ret = 1;
  
-	attr.ia_valid = ATTR_OPEN;
-	if (openflags & O_TRUNC) {
-		attr.ia_valid |= ATTR_SIZE;
-		attr.ia_size = 0;
-		nfs_wb_all(inode);
-	}
-
-	/*
-	 * Note: we're not holding inode->i_mutex and so may be racing with
-	 * operations that change the directory. We therefore save the
-	 * change attribute *before* we do the RPC call.
-	 */
-	inode = NFS_PROTO(dir)->open_context(dir, ctx, openflags, &attr);
-	if (IS_ERR(inode)) {
-		ret = PTR_ERR(inode);
-		switch (ret) {
-		case -EPERM:
-		case -EACCES:
-		case -EDQUOT:
-		case -ENOSPC:
-		case -EROFS:
-			goto out_put_ctx;
-		default:
-			goto out_drop;
-		}
-	}
-	iput(inode);
-	if (inode != dentry->d_inode)
-		goto out_drop;
-
-	nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
-	ret = nfs_intent_set_file(nd, ctx);
-	if (ret >= 0)
-		ret = 1;
 out:
 	dput(parent);
 	return ret;
-out_drop:
-	d_drop(dentry);
-	ret = 0;
-out_put_ctx:
-	put_nfs_open_context(ctx);
-	goto out;
  
 no_open_dput:
 	dput(parent);
@@ -879,12 +879,81 @@
 static int
 nfs4_file_open(struct inode *inode, struct file *filp)
 {
+	struct nfs_open_context *ctx;
+	struct dentry *dentry = filp->f_path.dentry;
+	struct dentry *parent = NULL;
+	struct inode *dir;
+	unsigned openflags = filp->f_flags;
+	struct iattr attr;
+	int err;
+
+	BUG_ON(inode != dentry->d_inode);
 	/*
-	 * NFSv4 opens are handled in d_lookup and d_revalidate. If we get to
-	 * this point, then something is very wrong
+	 * If no cached dentry exists or if it's negative, NFSv4 handled the
+	 * opens in ->lookup() or ->create().
+	 *
+	 * We only get this far for a cached positive dentry.  We skipped
+	 * revalidation, so handle it here by dropping the dentry and returning
+	 * -EOPENSTALE.  The VFS will retry the lookup/create/open.
 	 */
-	dprintk("NFS: %s called! inode=%p filp=%p\n", __func__, inode, filp);
-	return -ENOTDIR;
+
+	dprintk("NFS: open file(%s/%s)\n",
+		dentry->d_parent->d_name.name,
+		dentry->d_name.name);
+
+	if ((openflags & O_ACCMODE) == 3)
+		openflags--;
+
+	/* We can't create new files here */
+	openflags &= ~(O_CREAT|O_EXCL);
+
+	parent = dget_parent(dentry);
+	dir = parent->d_inode;
+
+	ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
+	err = PTR_ERR(ctx);
+	if (IS_ERR(ctx))
+		goto out;
+
+	attr.ia_valid = ATTR_OPEN;
+	if (openflags & O_TRUNC) {
+		attr.ia_valid |= ATTR_SIZE;
+		attr.ia_size = 0;
+		nfs_wb_all(inode);
+	}
+
+	inode = NFS_PROTO(dir)->open_context(dir, ctx, openflags, &attr);
+	if (IS_ERR(inode)) {
+		err = PTR_ERR(inode);
+		switch (err) {
+		case -EPERM:
+		case -EACCES:
+		case -EDQUOT:
+		case -ENOSPC:
+		case -EROFS:
+			goto out_put_ctx;
+		default:
+			goto out_drop;
+		}
+	}
+	iput(inode);
+	if (inode != dentry->d_inode)
+		goto out_drop;
+
+	nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
+	nfs_file_set_open_context(filp, ctx);
+	err = 0;
+
+out_put_ctx:
+	put_nfs_open_context(ctx);
+out:
+	dput(parent);
+	return err;
+
+out_drop:
+	d_drop(dentry);
+	err = -EOPENSTALE;
+	goto out_put_ctx;
 }
  
 const struct file_operations nfs4_file_operations = {
...	...	@@ -1354,10 +1354,10 @@
1354	1354	}
1355	1355
1356	1356	#ifdef CONFIG_NFS_V4
1357		-static int nfs_open_revalidate(struct dentry , struct nameidata );
	1357	+static int nfs4_lookup_revalidate(struct dentry , struct nameidata );
1358	1358
1359	1359	const struct dentry_operations nfs4_dentry_operations = {
1360		- .d_revalidate = nfs_open_revalidate,
	1360	+ .d_revalidate = nfs4_lookup_revalidate,
1361	1361	.d_delete = nfs_dentry_delete,
1362	1362	.d_iput = nfs_dentry_iput,
1363	1363	.d_automount = nfs_d_automount,
1364	1364
...	...	@@ -1519,13 +1519,11 @@
1519	1519	return nfs_lookup(dir, dentry, nd);
1520	1520	}
1521	1521
1522		-static int nfs_open_revalidate(struct dentry dentry, struct nameidata nd)
	1522	+static int nfs4_lookup_revalidate(struct dentry dentry, struct nameidata nd)
1523	1523	{
1524	1524	struct dentry *parent = NULL;
1525	1525	struct inode *inode;
1526	1526	struct inode *dir;
1527		- struct nfs_open_context *ctx;
1528		- struct iattr attr;
1529	1527	int openflags, ret = 0;
1530	1528
1531	1529	if (nd->flags & LOOKUP_RCU)
1532	1530
1533	1531
1534	1532
...	...	@@ -1554,57 +1552,13 @@
1554	1552	/* We cannot do exclusive creation on a positive dentry */
1555	1553	if ((openflags & (O_CREAT\|O_EXCL)) == (O_CREAT\|O_EXCL))
1556	1554	goto no_open_dput;
1557		- /* We can't create new files here */
1558		- openflags &= ~(O_CREAT\|O_EXCL);
1559	1555
1560		- ctx = create_nfs_open_context(dentry, openflags);
1561		- ret = PTR_ERR(ctx);
1562		- if (IS_ERR(ctx))
1563		- goto out;
	1556	+ /* Let f_op->open() actually open (and revalidate) the file */
	1557	+ ret = 1;
1564	1558
1565		- attr.ia_valid = ATTR_OPEN;
1566		- if (openflags & O_TRUNC) {
1567		- attr.ia_valid \|= ATTR_SIZE;
1568		- attr.ia_size = 0;
1569		- nfs_wb_all(inode);
1570		- }
1571		-
1572		- /*
1573		- * Note: we're not holding inode->i_mutex and so may be racing with
1574		- * operations that change the directory. We therefore save the
1575		- * change attribute before we do the RPC call.
1576		- */
1577		- inode = NFS_PROTO(dir)->open_context(dir, ctx, openflags, &attr);
1578		- if (IS_ERR(inode)) {
1579		- ret = PTR_ERR(inode);
1580		- switch (ret) {
1581		- case -EPERM:
1582		- case -EACCES:
1583		- case -EDQUOT:
1584		- case -ENOSPC:
1585		- case -EROFS:
1586		- goto out_put_ctx;
1587		- default:
1588		- goto out_drop;
1589		- }
1590		- }
1591		- iput(inode);
1592		- if (inode != dentry->d_inode)
1593		- goto out_drop;
1594		-
1595		- nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
1596		- ret = nfs_intent_set_file(nd, ctx);
1597		- if (ret >= 0)
1598		- ret = 1;
1599	1559	out:
1600	1560	dput(parent);
1601	1561	return ret;
1602		-out_drop:
1603		- d_drop(dentry);
1604		- ret = 0;
1605		-out_put_ctx:
1606		- put_nfs_open_context(ctx);
1607		- goto out;
1608	1562
1609	1563	no_open_dput:
1610	1564	dput(parent);
...	...	@@ -879,12 +879,81 @@
879	879	static int
880	880	nfs4_file_open(struct inode inode, struct file filp)
881	881	{
	882	+ struct nfs_open_context *ctx;
	883	+ struct dentry *dentry = filp->f_path.dentry;
	884	+ struct dentry *parent = NULL;
	885	+ struct inode *dir;
	886	+ unsigned openflags = filp->f_flags;
	887	+ struct iattr attr;
	888	+ int err;
	889	+
	890	+ BUG_ON(inode != dentry->d_inode);
882	891	/*
883		- * NFSv4 opens are handled in d_lookup and d_revalidate. If we get to
884		- * this point, then something is very wrong
	892	+ * If no cached dentry exists or if it's negative, NFSv4 handled the
	893	+ * opens in ->lookup() or ->create().
	894	+ *
	895	+ * We only get this far for a cached positive dentry. We skipped
	896	+ * revalidation, so handle it here by dropping the dentry and returning
	897	+ * -EOPENSTALE. The VFS will retry the lookup/create/open.
885	898	*/
886		- dprintk("NFS: %s called! inode=%p filp=%p\n", __func__, inode, filp);
887		- return -ENOTDIR;
	899	+
	900	+ dprintk("NFS: open file(%s/%s)\n",
	901	+ dentry->d_parent->d_name.name,
	902	+ dentry->d_name.name);
	903	+
	904	+ if ((openflags & O_ACCMODE) == 3)
	905	+ openflags--;
	906	+
	907	+ /* We can't create new files here */
	908	+ openflags &= ~(O_CREAT\|O_EXCL);
	909	+
	910	+ parent = dget_parent(dentry);
	911	+ dir = parent->d_inode;
	912	+
	913	+ ctx = alloc_nfs_open_context(filp->f_path.dentry, filp->f_mode);
	914	+ err = PTR_ERR(ctx);
	915	+ if (IS_ERR(ctx))
	916	+ goto out;
	917	+
	918	+ attr.ia_valid = ATTR_OPEN;
	919	+ if (openflags & O_TRUNC) {
	920	+ attr.ia_valid \|= ATTR_SIZE;
	921	+ attr.ia_size = 0;
	922	+ nfs_wb_all(inode);
	923	+ }
	924	+
	925	+ inode = NFS_PROTO(dir)->open_context(dir, ctx, openflags, &attr);
	926	+ if (IS_ERR(inode)) {
	927	+ err = PTR_ERR(inode);
	928	+ switch (err) {
	929	+ case -EPERM:
	930	+ case -EACCES:
	931	+ case -EDQUOT:
	932	+ case -ENOSPC:
	933	+ case -EROFS:
	934	+ goto out_put_ctx;
	935	+ default:
	936	+ goto out_drop;
	937	+ }
	938	+ }
	939	+ iput(inode);
	940	+ if (inode != dentry->d_inode)
	941	+ goto out_drop;
	942	+
	943	+ nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
	944	+ nfs_file_set_open_context(filp, ctx);
	945	+ err = 0;
	946	+
	947	+out_put_ctx:
	948	+ put_nfs_open_context(ctx);
	949	+out:
	950	+ dput(parent);
	951	+ return err;
	952	+
	953	+out_drop:
	954	+ d_drop(dentry);
	955	+ err = -EOPENSTALE;
	956	+ goto out_put_ctx;
888	957	}
889	958
890	959	const struct file_operations nfs4_file_operations = {