[PATCH 0/7 RFC] fixes for vfs_lookup_open, and integration with nfsd

[PATCH 0/7 RFC] fixes for vfs_lookup_open, and integration with nfsd

[NeilBrown]

I've been looking at the problems Chuck and Jeff found with my
vfs_lookup_open() work for opening files in nfsd and think the following
patches address the issues.

I have labeled this an RFC because:
1/ Patch 2/7 changes user-visible behaviour - in a good way I think.
   Currently O_NONBLOCK avoids waiting for a leases but doesn't
   avoid waiting for a directory delegation.  I think it should do both. 
2/ vfs_lookup_open() is changed to update the 'dentry' in the path arg
   to report what was found.  On success this is identical to the
   returned file->f_path.dentry. On -EFTYPE failure it is useful
   to report the actual type to the NFS client as required.

As an aside - what would people think of changing ->atomic_open
functions to *not* return -EFTYPE.  If they find a non-regular file they
should use finish_no_open() and let the caller decide if a non-regular
is an error.  ->atomic_open can still honour O_DIRECTORY if a network
filesystem does that some special way, but if it find a non-regular when
O_DIRECTORY wasn't requested, it is best to report what it found and let
others deal with it.  As it is we need an extra d_lookup() in
vfs_lookup_open() to find the dentry that ->atomic_open refused to
return.

Or maybe we shouldn't pass __O_REGULAR because that only affects "open",
and lookup_open() doesn't even try to open non-regular files.

I'm keen to read your thoughts on the above.

Thanks,
NeilBrown


 [PATCH 1/7] vfs: add some allowed open flags to vfs_lookup_open()
 [PATCH 2/7] vfs: O_NONBLOCK|O_CREAT open shouldn't wait for directory
 [PATCH 3/7] vfs: vfs_lookup_open() should only return -EFTYPE for
 [PATCH 4/7] vfs: change vfs_lookup_open() to return found dentry in
 [PATCH 5/7] nfsd: switch NFS4 OPEN to use vfs_lookup_open()
 [PATCH 6/7] nfsd: nfsd_check_obj_isreg() to use nfs error codes.
 [PATCH 7/7] nfsd: use vfs_lookup_open() for non-creating open

[PATCH 1/7] vfs: add some allowed open flags to vfs_lookup_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

vfs_lookup_open() must allow:
  O_LARGEFILE so that large files can be opened.
  O_NONBLOCK so that break_lease() can be asked to return -EWOULDBLOCK.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index d95249dd527c..c9ac703110f8 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4632,7 +4632,8 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	int error = 0;
 
 	WARN_ONCE(mode & ~S_IALLUGO, "mode must only have permission bits");
-	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR),
+	WARN_ONCE(open_flag & ~(O_ACCMODE|O_CREAT|O_EXCL|O_TRUNC|__O_REGULAR|
+				O_NONBLOCK|O_LARGEFILE),
 		  "open_flag has unsupported flags");
 
 	mode |= S_IFREG;

base-commit: 9189e6a6f89e32d3a604b221ea64e67e1a35957c
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 2/7] vfs: O_NONBLOCK|O_CREAT open shouldn't wait for directory delegation

[NeilBrown]

From: NeilBrown <neil@brown.name>

An open() of a regular file can block if there is an active lease on that
file, but this can be prevented by opening with O_NONBLOCK.
An open(O_CREAT) of a non-existent file can block if there is an active
delegation on the parent directory but this CANNOT be prevented with
O_NONBLOCK.

If nfsd is to use common VFS code for open/create it needs to be able to
prevent this blocking.  It is conceivable that a user-space application
might need this too.

So extend O_NONBLOCK protection to not block on a directory delegation
when creating a file.

Fixes: 134796f43a5e ("vfs: break parent dir delegations in open(..., O_CREAT) codepath")
Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index c9ac703110f8..c4be59213352 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4554,7 +4554,12 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		goto out_dput;
 	}
 
-	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, &delegated_inode);
+	if (op->open_flag & O_NONBLOCK)
+		error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE,
+					NULL);
+	else
+		error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE,
+					&delegated_inode);
 	if (error)
 		goto out_dput;
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 3/7] vfs: vfs_lookup_open() should only return -EFTYPE for non-regular files

[NeilBrown]

From: NeilBrown <neil@brown.name>

Decoding the non-regular type into over-loaded error codes doesn't
really help nfsd much and it isn't particularly elegant.
It also isn't reliable as some filesystems may return -EFTYPE
from their atomic_open() for any non-regular file, given that
__O_REGULAR was passed.

So change to only return -EFTYPE for non-regular files.
Also use d_is_reg() to detect this case.

Caller can repeat the lookup in the dcache to determine what sort of
object was there.  This might find a different dentry if there is a
race, but that is not significant.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c4be59213352..0c787363b101 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4616,14 +4616,8 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
  * nfsd.
  * It uses ->atomic_open or ->lookup / ->create / ->open as appropriate.
  *
- * If the fs object found is not a regular file then an error is returned.
- * In some cases, related errors are repurposed so that the caller can
- * determine the type of file found from the error.
- * -EISDIR : a directory was found
- * -ELOOP  : a symlink was found
- * -ENODEV : a block or character device special file was found
- * -EFTYPE : any other non-regular file was found, such as FIFO or SOCK.
- *           or ->atomic_open responded to __O_REGULAR.
+ * If the fs object found is not a regular file then -EFTYPE is
+ * returned.
  *
  * Returns: the opened struct file, or an error.
  */
@@ -4671,24 +4665,8 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 		error = -ENOENT;
 	} else if (!(file->f_mode & FMODE_CREATED) && (open_flag & O_EXCL)) {
 		error = -EEXIST;
-	} else if ((dentry->d_inode->i_mode & S_IFMT) != S_IFREG) {
-		switch (dentry->d_inode->i_mode & S_IFMT) {
-		case S_IFDIR:
-			error = -EISDIR;
-			break;
-		case S_IFLNK:
-			error = -ELOOP;
-			break;
-		case S_IFBLK:
-		case S_IFCHR:
-			error = -ENODEV;
-			break;
-		case S_IFIFO:
-		case S_IFSOCK:
-		default:
-			error = -EFTYPE;
-			break;
-		}
+	} else if (!d_is_reg(dentry)) {
+		error = -EFTYPE;
 	} else if (!(file->f_mode & FMODE_OPENED)) {
 		nd.path.dentry = dentry;
 		error = vfs_open(&nd.path, file);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 4/7] vfs: change vfs_lookup_open() to return found dentry in path.dentry.

[NeilBrown]

From: NeilBrown <neil@brown.name>

The caller - nfsd - needs to know the type of object found when a
non-regular file was found to exist.  This is most easily done
by passing back the dentry.

So change calling pattern so that the path (now called "path") contains
the parent dentry on entry and the child dentry on exit.
vfs_lookup_open() will dput() the parent and caller must dput() the
child.

If lookup_open() returns -EFTYPE (from ->atomic_open()) we need to
use d_lookup() to find the dentry.  If ->atomic_open() didn't add a
dentry to the dcache we return %NULL in %path->dentry.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 0c787363b101..30ccc0c2f02f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4606,7 +4606,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 
 /**
  * vfs_lookup_open - open and possibly create a regular file
- * @parent: directory to contain file
+ * @path: directory to contain file
  * @last: final component of file name
  * @open_flag: O_flags
  * @mode: initial permissions for file
@@ -4619,9 +4619,15 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
  * If the fs object found is not a regular file then -EFTYPE is
  * returned.
  *
+ * @path is updated to contain the dentry that was found if possible.
+ * In particular, if -EFTYPE is returned, then @path.dentry will be the
+ * object that is not a regular file, or %NULL.
+ * Consequently the caller must be prepared for @path.dentry to be
+ * dput(), an it must dput() whatever is in @path.dentry after the call.
+ *
  * Returns: the opened struct file, or an error.
  */
-struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
+struct file *vfs_lookup_open(struct path *path, struct qstr *last,
 			     int open_flag, umode_t mode)
 {
 	struct file *file __free(fput) = NULL;
@@ -4638,7 +4644,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	mode |= S_IFREG;
 	open_flag |= __O_REGULAR;
 
-	error = lookup_noperm_common(last, parent->dentry);
+	error = lookup_noperm_common(last, path->dentry);
 	if (error)
 		return ERR_PTR(error);
 
@@ -4646,7 +4652,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	if (IS_ERR(file))
 		return file;
 
-	nd.path = *parent;
+	nd.path = *path;
 	nd.last = *last;
 	nd.flags = LOOKUP_OPEN;
 	if (open_flag & O_CREAT) {
@@ -4658,9 +4664,18 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	op.mode = mode;
 	dentry = lookup_open(&nd, file, &op);
 
-	if (IS_ERR(dentry))
+	if (IS_ERR(dentry)) {
+		if (dentry == -EFTYPE) {
+			/* Try to determine what was found */
+			struct dentry *child = d_lookup(path->dentry, &nd.last);
+			dput(path->dentry);
+			path->dentry = child;
+			WARN_ON_ONCE(child && d_is_reg(child));
+		}
 		return ERR_CAST(dentry);
-
+	}
+	dput(path->dentry);
+	path->dentry = dentry;
 	if (d_really_is_negative(dentry)) {
 		error = -ENOENT;
 	} else if (!(file->f_mode & FMODE_CREATED) && (open_flag & O_EXCL)) {
@@ -4668,10 +4683,8 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	} else if (!d_is_reg(dentry)) {
 		error = -EFTYPE;
 	} else if (!(file->f_mode & FMODE_OPENED)) {
-		nd.path.dentry = dentry;
-		error = vfs_open(&nd.path, file);
+		error = vfs_open(path, file);
 	}
-	dput(dentry);
 
 	if (error)
 		return ERR_PTR(error);
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 4/7] vfs: change vfs_lookup_open() to return found dentry in path.dentry.

[NeilBrown]

On Thu, 10 Sep 2026, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The caller - nfsd - needs to know the type of object found when a
> non-regular file was found to exist.  This is most easily done
> by passing back the dentry.
> 
> So change calling pattern so that the path (now called "path") contains
> the parent dentry on entry and the child dentry on exit.
> vfs_lookup_open() will dput() the parent and caller must dput() the
> child.
> 
> If lookup_open() returns -EFTYPE (from ->atomic_open()) we need to
> use d_lookup() to find the dentry.  If ->atomic_open() didn't add a
> dentry to the dcache we return %NULL in %path->dentry.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 0c787363b101..30ccc0c2f02f 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4606,7 +4606,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  
>  /**
>   * vfs_lookup_open - open and possibly create a regular file
> - * @parent: directory to contain file
> + * @path: directory to contain file
>   * @last: final component of file name
>   * @open_flag: O_flags
>   * @mode: initial permissions for file
> @@ -4619,9 +4619,15 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>   * If the fs object found is not a regular file then -EFTYPE is
>   * returned.
>   *
> + * @path is updated to contain the dentry that was found if possible.
> + * In particular, if -EFTYPE is returned, then @path.dentry will be the
> + * object that is not a regular file, or %NULL.
> + * Consequently the caller must be prepared for @path.dentry to be
> + * dput(), an it must dput() whatever is in @path.dentry after the call.
> + *
>   * Returns: the opened struct file, or an error.
>   */
> -struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
> +struct file *vfs_lookup_open(struct path *path, struct qstr *last,
>  			     int open_flag, umode_t mode)
>  {
>  	struct file *file __free(fput) = NULL;
> @@ -4638,7 +4644,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	mode |= S_IFREG;
>  	open_flag |= __O_REGULAR;
>  
> -	error = lookup_noperm_common(last, parent->dentry);
> +	error = lookup_noperm_common(last, path->dentry);
>  	if (error)
>  		return ERR_PTR(error);
>  
> @@ -4646,7 +4652,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	if (IS_ERR(file))
>  		return file;
>  
> -	nd.path = *parent;
> +	nd.path = *path;
>  	nd.last = *last;
>  	nd.flags = LOOKUP_OPEN;
>  	if (open_flag & O_CREAT) {
> @@ -4658,9 +4664,18 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	op.mode = mode;
>  	dentry = lookup_open(&nd, file, &op);
>  
> -	if (IS_ERR(dentry))
> +	if (IS_ERR(dentry)) {
> +		if (dentry == -EFTYPE) {

                if (dentry == ERR_PTR(-EFTYPE)) {

of course.

NeilBrown

> +			/* Try to determine what was found */
> +			struct dentry *child = d_lookup(path->dentry, &nd.last);
> +			dput(path->dentry);
> +			path->dentry = child;
> +			WARN_ON_ONCE(child && d_is_reg(child));
> +		}
>  		return ERR_CAST(dentry);
> -
> +	}
> +	dput(path->dentry);
> +	path->dentry = dentry;
>  	if (d_really_is_negative(dentry)) {
>  		error = -ENOENT;
>  	} else if (!(file->f_mode & FMODE_CREATED) && (open_flag & O_EXCL)) {
> @@ -4668,10 +4683,8 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	} else if (!d_is_reg(dentry)) {
>  		error = -EFTYPE;
>  	} else if (!(file->f_mode & FMODE_OPENED)) {
> -		nd.path.dentry = dentry;
> -		error = vfs_open(&nd.path, file);
> +		error = vfs_open(path, file);
>  	}
> -	dput(dentry);
>  
>  	if (error)
>  		return ERR_PTR(error);
> -- 
> 2.50.0.107.gf914562f5916.dirty
> 
> 
> 

Re: [PATCH 4/7] vfs: change vfs_lookup_open() to return found dentry in path.dentry.

[Chuck Lever]

On Thu, 10 Sep 2026, NeilBrown wrote:

> vfs: change vfs_lookup_open() to return found dentry in path.dentry.
>
> The caller - nfsd - needs to know the type of object found when a
> non-regular file was found to exist.  This is most easily done
> by passing back the dentry.
>
> So change calling pattern so that the path (now called "path") contains
> the parent dentry on entry and the child dentry on exit.
> vfs_lookup_open() will dput() the parent and caller must dput() the
> child.
>
> If lookup_open() returns -EFTYPE (from ->atomic_open()) we need to
> use d_lookup() to find the dentry.  If ->atomic_open() didn't add a
> dentry to the dcache we return %NULL in %path->dentry.
>
> Signed-off-by: NeilBrown <neil@brown.name>

"vfs_lookup_open() will dput() the parent and caller must dput() the
child."

Is that the whole contract? When lookup_noperm_common() or
alloc_empty_file() fails, path->dentry still holds the parent on return:

	error = lookup_noperm_common(last, path->dentry);
	if (error)
		return ERR_PTR(error);

	file = alloc_empty_file(open_flag, current_cred());
	if (IS_ERR(file))
		return file;

The parent is replaced only after lookup_open() returns. Could the
message say that on success and on -EFTYPE the parent is replaced by
the child and its reference dropped, and that on any other error @path
is untouched?


> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4606,7 +4606,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>
>  /**
>   * vfs_lookup_open - open and possibly create a regular file
> - * @parent: directory to contain file
> + * @path: directory to contain file

Now that @path is in/out, could this say what it holds on return as
well? Something like "on entry, the directory to contain the file; on
return, the dentry found".

The prototype in include/linux/namei.h still names this parameter
"parent".

> @@ -4619,9 +4619,15 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>   * If the fs object found is not a regular file then -EFTYPE is
>   * returned.
>   *
> + * @path is updated to contain the dentry that was found if possible.
> + * In particular, if -EFTYPE is returned, then @path.dentry will be the
> + * object that is not a regular file, or %NULL.
> + * Consequently the caller must be prepared for @path.dentry to be
> + * dput(), an it must dput() whatever is in @path.dentry after the call.

"an it must dput()" -> "and it must dput()".

"if possible" leaves the reader guessing which paths update @path. As
above, it is updated on success and on -EFTYPE, and untouched on every
other error. Since the caller cannot tell the two cases apart from the
return value alone, is that worth stating here?


> @@ -4658,9 +4664,18 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	op.mode = mode;
>  	dentry = lookup_open(&nd, file, &op);
>
> -	if (IS_ERR(dentry))
> +	if (IS_ERR(dentry)) {
> +		if (dentry == -EFTYPE) {

dentry is a struct dentry * and -EFTYPE is an int. Both compilers warn:

warning: comparison between pointer and integer ('struct dentry *' and
'int') [-Wpointer-integer-compare]

and with CONFIG_WERROR (the default for COMPILE_TEST builds) fs/namei.c
no longer compiles. It evaluates as intended only because the int is
converted to a pointer with the same bits as ERR_PTR(-EFTYPE). Should
this be:

		if (PTR_ERR(dentry) == -EFTYPE) {


> +			/* Try to determine what was found */
> +			struct dentry *child = d_lookup(path->dentry, &nd.last);
> +			dput(path->dentry);
> +			path->dentry = child;
> +			WARN_ON_ONCE(child && d_is_reg(child));

lookup_open() drops the parent's inode lock before returning, so this
d_lookup() runs unlocked. Between ->atomic_open() rejecting a
non-regular object and this lookup, another task can unlink it and
create a regular file with the same name. d_lookup() can also return a
hashed negative dentry.

Either way @path.dentry is then neither "the object that is not a
regular file" nor NULL, and the WARN fires on a race that ordinary
concurrent activity (remote, over nfsd) can drive. Could this store
NULL when the child is negative or regular, and drop the WARN, so the
documented contract always holds?

Does this recovery find anything for most ->atomic_open()
implementations? lookup_open() only reaches ->atomic_open() with a
negative or in-lookup dentry, and atomic_open() dputs it on error, so
what d_lookup() sees is whatever the filesystem hashed before failing.

The NFS client d_drop()s the dentry before returning -EFTYPE:

fs/nfs/dir.c:nfs_atomic_open() {
	...
		d_drop(dentry);
		switch (err) {
		...
		case -EISDIR:
		case -ENOTDIR:
			if (open_flags & __O_REGULAR) {
				err = -EFTYPE;
				break;
			}

gfs2 and cifs return -EFTYPE before instantiating anything. Only ceph
checks d_is_reg() on a dentry it has already spliced. So on gfs2, cifs
and a re-exported NFS mount, path->dentry is always NULL here, and the
caller cannot distinguish a directory from a symlink. That might not
be acceptable for what nfsd needs. The found type needs to come
back from ->atomic_open() itself, probably.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

[PATCH 5/7] nfsd: switch NFS4 OPEN to use vfs_lookup_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

The functionality that was recently gathered into do_lookup_open() is
now available from the vfs as vfs_lookup_open().  So nfsd4_create_file()
can call that, with a few adjustments.

This implementation shares more code with syscall open paths and so uses
some filesystem interfaces slightly more correctly.  It also takes the
responsibility for locking out of nfsd so that planned changes can
happen entirely in VFS code.

We need to pass O_NONBLOCK so that that EWOULDBLOCK errors from
break_lease of try_break_deleg() get passed back.

We need to mask any type out of "mode" else vfs_lookup_open() will warn.

vfs_lookup_open() always returns -EFTYPE if a non-regular-file was
found, and provides the dentry in parent.dentry.  We can use
nfsd_check_obj_is_reg() to turn this into an error.

As parent.dentry could be NULL, we enhance nfsd_check_obj_is_reg() to
cope with that.

vfs_lookup_open() will return -EEXIST if required for
NFS_CREATE_GUARDED4 as O_EXCL is passed in.  Other checks for
and existing object need only test for nfsd4_create_is_exclusive().
Make both these tests (for non-regular and for regular) the same.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 83 ++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 62 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3a82af381a8d..0fd5a6411ed3 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -225,8 +225,11 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 
 static int nfsd_check_obj_isreg(struct dentry *child)
 {
-	umode_t mode = d_inode(child)->i_mode;
+	umode_t mode;
 
+	if (!child || !d_inode(child))
+		return -EFTYPE;
+	mode = d_inode(child)->i_mode;
 	if (S_ISREG(mode))
 		return 0;
 	if (S_ISDIR(mode))
@@ -250,52 +253,6 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
 		createmode == NFS4_CREATE_EXCLUSIVE4_1;
 }
 
-static struct file *do_lookup_open(struct path *parent,
-				   struct qstr *name,
-				   unsigned int oflags,
-				   umode_t mode)
-{
-	struct file *filp = NULL;
-	struct path path;
-	struct dentry *child;
-	int want_write_err = 0;
-
-	want_write_err = mnt_want_write(parent->mnt);
-
-	child = start_creating(&nop_mnt_idmap, parent->dentry, name);
-	if (IS_ERR(child)) {
-		filp = ERR_CAST(child);
-		goto out;
-	}
-	path.mnt = parent->mnt;
-	path.dentry = child;
-
-	if (d_really_is_positive(child)) {
-		/*
-		 * open the file so that we consistently have a valid
-		 * op_filp and consequently a valid ->f_path.dentry.
-		 */
-		int err = nfsd_check_obj_isreg(child);
-
-		if (err)
-			filp = ERR_PTR(err);
-		else
-			filp = dentry_open(&path, oflags, current_cred());
-	} else if (!(oflags & O_CREAT)) {
-		filp = ERR_PTR(-ENOENT);
-	} else if (want_write_err) {
-		filp = ERR_PTR(want_write_err);
-	} else {
-		filp = dentry_create(&path, oflags, mode, current_cred());
-		child = path.dentry;
-	}
-	end_creating(child);
-out:
-	if (!want_write_err)
-		mnt_drop_write(parent->mnt);
-	return filp;
-}
-
 /*
  * Implement NFSv4's unchecked, guarded, and exclusive create
  * semantics for regular files. Open state for this new file is
@@ -312,7 +269,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		.na_iattr	= iap,
 		.na_seclabel	= &open->op_label,
 	};
-	int oflags = O_CREAT | O_LARGEFILE;
+	int oflags = O_CREAT | O_LARGEFILE | O_NONBLOCK;
 	struct dentry *child = ERR_PTR(-EINVAL);
 	struct path parent = {
 		.mnt = fhp->fh_export->ex_path.mnt,
@@ -424,28 +381,29 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		/* Might still succeed if no create is needed */
 		oflags &= ~O_CREAT;
 
-	open->op_filp = do_lookup_open(&parent,
-				       &QSTR_LEN(open->op_fname,
-						 open->op_fnamelen),
-				       oflags,
-				       open->op_iattr.ia_mode);
+	dget(parent.dentry);
+	open->op_filp = vfs_lookup_open(&parent,
+					&QSTR_LEN(open->op_fname,
+						  open->op_fnamelen),
+					oflags,
+					open->op_iattr.ia_mode & S_IALLUGO);
 	if (IS_ERR(open->op_filp)) {
 		int hosterr = PTR_ERR(open->op_filp);
 
-		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
-			switch (hosterr) {
-			case -EISDIR:
-			case -ELOOP:
-			case -EFTYPE:
+		if (hosterr == -EFTYPE) {
+			if (nfsd4_create_is_exclusive(open->op_createmode))
 				hosterr = -EEXIST;
-			}
+			else
+				hosterr = nfsd_check_obj_isreg(parent.dentry);
 		}
 		status = nfserrno(hosterr);
 		open->op_filp = NULL;
 		if (status == nfserr_noent && create_status)
 			status = create_status;
+		dput(parent.dentry);
 		goto out;
 	}
+	dput(parent.dentry);
 
 	child = open->op_filp->f_path.dentry;
 	open->op_created = open->op_filp->f_mode & FMODE_CREATED;
@@ -462,7 +420,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		open->op_created = true;
 
 	if (!open->op_created) {
-		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
+		if (nfsd4_create_is_exclusive(open->op_createmode)) {
+			status = nfserr_exist;
+		} else {
 			/* NFSv4 protocol requires change attributes
 			 * even though no change happened.
 			 */
@@ -477,8 +437,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			open->op_truncate = (d_is_reg(child) &&
 					     (iap->ia_valid & ATTR_SIZE) &&
 					     !iap->ia_size);
-		} else
-			status = nfserr_exist;
+		}
 		goto out;
 	}
 	/* file was created */
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 5/7] nfsd: switch NFS4 OPEN to use vfs_lookup_open()

[Chuck Lever]

>     This implementation shares more code with syscall open paths and so uses
>     some filesystem interfaces slightly more correctly.  It also takes the

Which interfaces? The concrete difference I can see is that lookup_open()
honors ->atomic_open, which do_lookup_open() never called. Naming that
would be clearer than "slightly more correctly".


>     We need to pass O_NONBLOCK so that that EWOULDBLOCK errors from

Repeated word: "that that".


>     vfs_lookup_open() always returns -EFTYPE if a non-regular-file was
>     found, and provides the dentry in parent.dentry.  We can use
>     nfsd_check_obj_is_reg() to turn this into an error.
>
>     As parent.dentry could be NULL, we enhance nfsd_check_obj_is_reg() to
>     cope with that.

The function is nfsd_check_obj_isreg(), in both places.


>     vfs_lookup_open() will return -EEXIST if required for
>     NFS_CREATE_GUARDED4 as O_EXCL is passed in.  Other checks for
>     and existing object need only test for nfsd4_create_is_exclusive().

s/and existing/an existing/, and the identifier is NFS4_CREATE_GUARDED.

The -EEXIST claim holds only on the path through the tail of
vfs_lookup_open(), where the O_EXCL test runs before the d_is_reg()
test. When lookup_open() itself returns -EFTYPE, vfs_lookup_open()
returns before either test. See below.


> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3a82af381a8d..0fd5a6411ed3 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c

[ ... ]

> @@ -312,7 +269,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		.na_iattr	= iap,
>  		.na_seclabel	= &open->op_label,
>  	};
> -	int oflags = O_CREAT | O_LARGEFILE;
> +	int oflags = O_CREAT | O_LARGEFILE | O_NONBLOCK;

Nit: do_dentry_open() strips O_CREAT, O_EXCL and O_TRUNC from f_flags
and leaves O_NONBLOCK in place, so op_filp now carries O_NONBLOCK into
the filecache entry that nfsd_file_acquire_opened() builds from it.
Files opened through __nfsd_open() do not. Is that divergence
intended?


>  	struct dentry *child = ERR_PTR(-EINVAL);
>  	struct path parent = {
>  		.mnt = fhp->fh_export->ex_path.mnt,
> @@ -424,28 +381,29 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		/* Might still succeed if no create is needed */
>  		oflags &= ~O_CREAT;
>  
> -	open->op_filp = do_lookup_open(&parent,
> -				       &QSTR_LEN(open->op_fname,
> -						 open->op_fnamelen),
> -				       oflags,
> -				       open->op_iattr.ia_mode);
> +	dget(parent.dentry);
> +	open->op_filp = vfs_lookup_open(&parent,
> +					&QSTR_LEN(open->op_fname,
> +						  open->op_fnamelen),
> +					oflags,
> +					open->op_iattr.ia_mode & S_IALLUGO);

Nit: after this call "parent.dentry" is the child (or NULL), while a
few lines up IS_POSIXACL(d_inode(parent.dentry)) reads the same
expression as the parent directory. The VFS patch renamed its parameter
to @path for this reason. Would renaming the local here avoid the same
trap?


>  	if (IS_ERR(open->op_filp)) {
>  		int hosterr = PTR_ERR(open->op_filp);
>  
> -		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
> -			switch (hosterr) {
> -			case -EISDIR:
> -			case -ELOOP:
> -			case -EFTYPE:
> +		if (hosterr == -EFTYPE) {
> +			if (nfsd4_create_is_exclusive(open->op_createmode))
>  				hosterr = -EEXIST;
> -			}
> +			else
> +				hosterr = nfsd_check_obj_isreg(parent.dentry);
>  		}

Does this drop the NFS4ERR_EXIST mapping for GUARDED4?

The old test covered GUARDED as well as the two EXCLUSIVE modes.
nfsd4_create_is_exclusive() covers only EXCLUSIVE4 and EXCLUSIVE4_1, so
a GUARDED open that finds a non-regular object now takes the else arm
and returns NFS4ERR_ISDIR, NFS4ERR_WRONG_TYPE, and so on. RFC 8881
section 18.16.3 says a GUARDED4 duplicate is NFS4ERR_EXIST.

Keeping "open->op_createmode != NFS4_CREATE_UNCHECKED" here would
restore the old mapping without depending on the ordering of checks
inside each filesystem's ->atomic_open.

Separately, can this arm return nfs_ok? nfsd_check_obj_isreg() returns
0 for a regular file, and vfs_lookup_open() can leave a regular file in
parent.dentry when the d_lookup() it does after an -EFTYPE races with a
rename (the VFS marks that case with a WARN_ON_ONCE rather than
excluding it). nfserrno(0) is nfs_ok, op_filp is then set to NULL, and
nfsd4_create_file() returns success with resfhp never composed. It only
fails safely today because do_open_lookup() then calls
nfsd_check_obj_isreg() on the NULL fh_dentry. Something like

		if (!hosterr)
			hosterr = -EFTYPE;

would address that.


>  		status = nfserrno(hosterr);
>  		open->op_filp = NULL;
>  		if (status == nfserr_noent && create_status)
>  			status = create_status;
> +		dput(parent.dentry);
>  		goto out;
>  	}
> +	dput(parent.dentry);
>  
>  	child = open->op_filp->f_path.dentry;
>  	open->op_created = open->op_filp->f_mode & FMODE_CREATED;
> @@ -462,7 +420,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		open->op_created = true;
>  
>  	if (!open->op_created) {
> -		if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
> +		if (nfsd4_create_is_exclusive(open->op_createmode)) {
> +			status = nfserr_exist;
> +		} else {

Same question here. GUARDED used to get nfserr_exist from this branch
and now gets the "no change happened" arm. It is unreachable for GUARDED
today only because vfs_lookup_open() returns -EEXIST whenever O_EXCL is
set and nothing was created. Should this also stay as
"!= NFS4_CREATE_UNCHECKED" so the protocol rule is enforced here rather
than relying on that ordering?


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

[PATCH 6/7] nfsd: nfsd_check_obj_isreg() to use nfs error codes.

[NeilBrown]

From: NeilBrown <neil@brown.name>

There is no longer any value in having nfsd_check_obj_isreg() return
over-loaded error codes which are converted to nfs error codes.
So revert to directly returning the required nfs error code.

Also take the opportunity to avoid dereferencing the inode and determine
the type directly from the dentry.

We can now remove ELOOP from nfs_errtbl[] as it doesn't *really* mean
the same as nfserr_symlink.  We probably don't need EFTYPE either, but
there is a very good match of meaning, so let's leave it.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 36 ++++++++++++++++--------------------
 fs/nfsd/vfs.c      |  1 -
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 0fd5a6411ed3..a6ba7618a307 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -223,20 +223,17 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 	return fh_verify(rqstp, current_fh, S_IFREG, accmode);
 }
 
-static int nfsd_check_obj_isreg(struct dentry *child)
+static __be32 nfsd_check_obj_isreg(struct dentry *child)
 {
-	umode_t mode;
-
-	if (!child || !d_inode(child))
-		return -EFTYPE;
-	mode = d_inode(child)->i_mode;
-	if (S_ISREG(mode))
+	if (!child)
+		return nfserr_wrong_type;
+	if (d_is_reg(child))
 		return 0;
-	if (S_ISDIR(mode))
-		return -EISDIR;
-	if (S_ISLNK(mode))
-		return -ELOOP;
-	return -EFTYPE;
+	if (d_is_dir(child))
+		return nfserr_isdir;
+	if (d_is_symlink(child))
+		return nfserr_symlink;
+	return nfserr_wrong_type;
 }
 
 static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh *resfh)
@@ -388,18 +385,17 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 					oflags,
 					open->op_iattr.ia_mode & S_IALLUGO);
 	if (IS_ERR(open->op_filp)) {
-		int hosterr = PTR_ERR(open->op_filp);
-
-		if (hosterr == -EFTYPE) {
+		status = nfserrno(PTR_ERR(open->op_filp));
+		if (status == nfserr_wrong_type) {
 			if (nfsd4_create_is_exclusive(open->op_createmode))
-				hosterr = -EEXIST;
+				status = nfserr_exist;
 			else
-				hosterr = nfsd_check_obj_isreg(parent.dentry);
+				status = nfsd_check_obj_isreg(parent.dentry);
 		}
-		status = nfserrno(hosterr);
-		open->op_filp = NULL;
 		if (status == nfserr_noent && create_status)
 			status = create_status;
+
+		open->op_filp = NULL;
 		dput(parent.dentry);
 		goto out;
 	}
@@ -558,7 +554,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	}
 	if (status)
 		goto out;
-	status = nfserrno(nfsd_check_obj_isreg((*resfh)->fh_dentry));
+	status = nfsd_check_obj_isreg((*resfh)->fh_dentry);
 	if (status)
 		goto out;
 
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f9131827d391..8def58e92a8e 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -106,7 +106,6 @@ nfserrno(int errno)
 		{ nfserr_perm, -ENOKEY },
 		{ nfserr_no_grace, -ENOGRACE},
 		{ nfserr_io, -EBADMSG },
-		{ nfserr_symlink, -ELOOP },
 		{ nfserr_wrong_type, -EFTYPE },
 	};
 	int	i;
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 7/7] nfsd: use vfs_lookup_open() for non-creating open requests too.

[NeilBrown]

From: NeilBrown <neil@brown.name>

Now that we have vfs_lookup_open() for open requests which create, we
can use it for non-creating requests too as vfs_lookup_open() is already
able to do that.  nfsd4_create_file() is renamed to nfsd4_open_file()
and enhanced to not always create, and is then used for all OPEN
requests.

The resulting simplification allows fh_fill_pre_attrs_unlocked() to be
moved into nfsd4_open_file() so it is closer to fh_full_post_attrs and
fh_fill_post_noop calls.

As ->op_create_mode isn't defined when op_create is zero, we need a
local create_mode which is -1 (illegal value) when op_create is zero.

The non-create path now doesn't use nfsd_lookup().  As mount-point
crossing including nfsd_check_access() is already included for existing
names, this does not lose us anything.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4proc.c | 109 ++++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 56 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a6ba7618a307..1f3096486565 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -251,29 +251,30 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
 }
 
 /*
- * Implement NFSv4's unchecked, guarded, and exclusive create
- * semantics for regular files. Open state for this new file is
- * subsequently fabricated in nfsd4_process_open2().
- *
+ * Implement NFSv4's open semantics for regular files.
+ * Both create (unchecked, guarded, and exclusive) and non-create.
+ * Open state for this new file is subsequently fabricated in
+ * nfsd4_process_open2().
  * Upon return, caller must release @fhp and @resfhp.
  */
 static __be32
-nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
-		  struct svc_fh *resfhp, struct nfsd4_open *open)
+nfsd4_open_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
+		struct svc_fh *resfhp, struct nfsd4_open *open)
 {
 	struct iattr *iap = &open->op_iattr;
 	struct nfsd_attrs attrs = {
 		.na_iattr	= iap,
 		.na_seclabel	= &open->op_label,
 	};
-	int oflags = O_CREAT | O_LARGEFILE | O_NONBLOCK;
+	int oflags = O_LARGEFILE | O_NONBLOCK;
 	struct dentry *child = ERR_PTR(-EINVAL);
 	struct path parent = {
 		.mnt = fhp->fh_export->ex_path.mnt,
 		.dentry = fhp->fh_dentry,
 	};
 	__u32 v_mtime, v_atime;
-	__be32 status, create_status;
+	int createmode = -1;
+	__be32 status, create_status = 0;
 	int want_write_err;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
@@ -285,6 +286,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (status != nfs_ok)
 		return status;
 
+	status = fh_fill_pre_attrs_unlocked(fhp);
+	if (status)
+		return status;
+
 	if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
 		/*
 		 * If name is already in dcache we need to check for mountpoints
@@ -315,11 +320,15 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (!IS_POSIXACL(d_inode(parent.dentry)))
 		iap->ia_mode &= ~current_umask();
 
+	if (open->op_create) {
+		createmode = open->op_createmode;
+		oflags |= O_CREAT;
+	}
 	/*
 	 * For the EXCLUSIVE modes we do our own uniqueness tests
 	 * so don't want O_EXCL.
 	 */
-	if (open->op_createmode == NFS4_CREATE_GUARDED)
+	if (createmode == NFS4_CREATE_GUARDED)
 		oflags |= O_EXCL;
 
 	switch (open->op_share_access & NFS4_SHARE_ACCESS_BOTH) {
@@ -351,7 +360,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	v_mtime = 0;
 	v_atime = 0;
-	if (nfsd4_create_is_exclusive(open->op_createmode)) {
+	if (nfsd4_create_is_exclusive(createmode)) {
 		u32 *verifier = (u32 *)open->op_verf.data;
 
 		/*
@@ -373,11 +382,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		iap->ia_atime.tv_nsec = 0;
 	}
 
-	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
-	if (create_status)
-		/* Might still succeed if no create is needed */
-		oflags &= ~O_CREAT;
-
+	if (oflags & O_CREAT) {
+		create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
+		if (create_status)
+			oflags &= ~O_CREAT;
+	}
 	dget(parent.dentry);
 	open->op_filp = vfs_lookup_open(&parent,
 					&QSTR_LEN(open->op_fname,
@@ -387,7 +396,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (IS_ERR(open->op_filp)) {
 		status = nfserrno(PTR_ERR(open->op_filp));
 		if (status == nfserr_wrong_type) {
-			if (nfsd4_create_is_exclusive(open->op_createmode))
+			if (nfsd4_create_is_exclusive(createmode))
 				status = nfserr_exist;
 			else
 				status = nfsd_check_obj_isreg(parent.dentry);
@@ -409,14 +418,14 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 		goto out;
 
 	if (!open->op_created &&
-	    nfsd4_create_is_exclusive(open->op_createmode) &&
+	    nfsd4_create_is_exclusive(createmode) &&
 	    inode_get_mtime_sec(d_inode(child)) == v_mtime &&
 	    inode_get_atime_sec(d_inode(child)) == v_atime &&
 	    d_inode(child)->i_size == 0)
 		open->op_created = true;
 
 	if (!open->op_created) {
-		if (nfsd4_create_is_exclusive(open->op_createmode)) {
+		if (nfsd4_create_is_exclusive(createmode)) {
 			status = nfserr_exist;
 		} else {
 			/* NFSv4 protocol requires change attributes
@@ -512,46 +521,34 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 	fh_init(*resfh, NFS4_FHSIZE);
 	open->op_truncate = false;
 
-	status = fh_fill_pre_attrs_unlocked(current_fh);
-	if (status)
-		goto out;
-	if (open->op_create) {
-		/* FIXME: check session persistence and pnfs flags.
-		 * The nfsv4.1 spec requires the following semantics:
-		 *
-		 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
-		 * Reply Cache  | server |                 |
-		 * -------------+--------+-----------------+--------------------
-		 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
-		 *              |        |                 | (SHOULD)
-		 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
-		 *              |        |                 | (SHOULD NOT)
-		 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
-		 * yes          | no     | GUARDED4        | GUARDED4
-		 * yes          | yes    | GUARDED4        | GUARDED4
-		 */
+	/* FIXME: check session persistence and pnfs flags.
+	 * The nfsv4.1 spec requires the following semantics:
+	 *
+	 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
+	 * Reply Cache  | server |                 |
+	 * -------------+--------+-----------------+--------------------
+	 * no           | no     | EXCLUSIVE4_1    | EXCLUSIVE4_1
+	 *              |        |                 | (SHOULD)
+	 *              |        | and EXCLUSIVE4  | or EXCLUSIVE4
+	 *              |        |                 | (SHOULD NOT)
+	 * no           | yes    | EXCLUSIVE4_1    | EXCLUSIVE4_1
+	 * yes          | no     | GUARDED4        | GUARDED4
+	 * yes          | yes    | GUARDED4        | GUARDED4
+	 */
 
-		current->fs->umask = open->op_umask;
-		status = nfsd4_create_file(rqstp, current_fh, *resfh, open);
-		current->fs->umask = 0;
+	current->fs->umask = open->op_umask;
+	status = nfsd4_open_file(rqstp, current_fh, *resfh, open);
+	current->fs->umask = 0;
 
-		/*
-		 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
-		 * use the returned bitmask to indicate which attributes
-		 * we used to store the verifier:
-		 */
-		if (nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
-			open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
-						FATTR4_WORD1_TIME_MODIFY);
-	} else {
-		status = nfsd_lookup(rqstp, current_fh,
-				     open->op_fname, open->op_fnamelen, *resfh);
-		/*
-		 * NFSv4 protocol requires change attributes even though
-		 * no change happened.
-		 */
-		fh_fill_post_noop(current_fh);
-	}
+	/*
+	 * Following rfc 3530 14.2.16, and rfc 5661 18.16.4
+	 * use the returned bitmask to indicate which attributes
+	 * we used to store the verifier:
+	 */
+	if (open->op_create &&
+	    nfsd4_create_is_exclusive(open->op_createmode) && status == 0)
+		open->op_bmval[1] |= (FATTR4_WORD1_TIME_ACCESS |
+				      FATTR4_WORD1_TIME_MODIFY);
 	if (status)
 		goto out;
 	status = nfsd_check_obj_isreg((*resfh)->fh_dentry);
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 7/7] nfsd: use vfs_lookup_open() for non-creating open requests too.

[Chuck Lever]

On Thu, 10 Sep 2026, NeilBrown wrote:

> nfsd: use vfs_lookup_open() for non-creating open requests too.
>
> Now that we have vfs_lookup_open() for open requests which create, we
> can use it for non-creating requests too as vfs_lookup_open() is already
> able to do that.  nfsd4_create_file() is renamed to nfsd4_open_file()
> and enhanced to not always create, and is then used for all OPEN
> requests.
>
> The resulting simplification allows fh_fill_pre_attrs_unlocked() to be
> moved into nfsd4_open_file() so it is closer to fh_full_post_attrs and
> fh_fill_post_noop calls.

fh_full_post_attrs is fh_fill_post_attrs().


> As ->op_create_mode isn't defined when op_create is zero, we need a
> local create_mode which is -1 (illegal value) when op_create is zero.

The field is ->op_createmode and the local is createmode.


> The non-create path now doesn't use nfsd_lookup().  As mount-point
> crossing including nfsd_check_access() is already included for existing
> names, this does not lose us anything.

The function is check_nfsd_access(), reached through nfsd_cross_mnt().

Is "already included for existing names" accurate? The only mount-point
crossing left in nfsd4_open_file() is the try_lookup_noperm() block,
and that consults only the dcache. More on that below.


> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 109 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 53 insertions(+), 56 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a6ba7618a307..1f3096486565 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -251,29 +251,30 @@ static inline bool nfsd4_create_is_exclusive(int createmode)
>  }
>
>  /*
> - * Implement NFSv4's unchecked, guarded, and exclusive create
> - * semantics for regular files. Open state for this new file is
> - * subsequently fabricated in nfsd4_process_open2().
> - *
> + * Implement NFSv4's open semantics for regular files.
> + * Both create (unchecked, guarded, and exclusive) and non-create.
> + * Open state for this new file is subsequently fabricated in
> + * nfsd4_process_open2().
>   * Upon return, caller must release @fhp and @resfhp.
>   */

"Both create ... and non-create." is a fragment, and "this new file" no
longer fits when the file may already exist. Perhaps:

 * Implement NFSv4 OPEN semantics for regular files, both non-creating
 * and creating (unchecked, guarded, and exclusive). Open state for the
 * file is subsequently fabricated in nfsd4_process_open2().
 *
 * Upon return, caller must release @fhp and @resfhp.


> @@ -285,6 +286,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (status != nfs_ok)
>  		return status;
>
> +	status = fh_fill_pre_attrs_unlocked(fhp);
> +	if (status)
> +		return status;
> +
>  	if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
>  		/*
>  		 * If name is already in dcache we need to check for mountpoints

First, the test reads open->op_createmode, which the commit message says
is undefined when op_create is zero. It works because
nfsd4_decode_open() zeroes the whole struct, so a non-creating OPEN
sees NFS4_CREATE_UNCHECKED here. Could the op_create test that sets
createmode be hoisted above this block, and the condition become

	if (!open->op_create || createmode == NFS4_CREATE_UNCHECKED) {

so the non-create path does not depend on that zero?

Second, nfsd_lookup_dentry() did a real lookup and then called
nfsd_cross_mnt() whenever nfsd_mountpoint() was true. try_lookup_noperm()
returns NULL when the name is not cached. In that case vfs_lookup_open()
opens whatever it finds on the parent's mount and fh_compose() uses the
parent's export.

For a referral junction (a regular file carrying the junction xattr),
nfsd_cross_mnt() would have swapped in the referral export. On a cold
dcache the client instead gets an ordinary filehandle for the junction
file, and a second OPEN of the same name, now cached, takes the
crossing path. The same applies to a bind-mounted file whose mount
target has its own export.

The Linux client sends OPEN by name without a preceding LOOKUP, so the
first access after a server reboot reaches this. Could the block do a
real lookup of the name, as nfsd_lookup_dentry() does, before deciding
whether to cross?


> @@ -373,11 +382,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		iap->ia_atime.tv_nsec = 0;
>  	}
>
> -	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> -	if (create_status)
> -		/* Might still succeed if no create is needed */
> -		oflags &= ~O_CREAT;
> -
> +	if (oflags & O_CREAT) {

Nit: O_CREAT is set only from open->op_create a few lines up. Would
testing open->op_create here read more directly?


> +		create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> +		if (create_status)
> +			oflags &= ~O_CREAT;
> +	}
>  	dget(parent.dentry);
>  	open->op_filp = vfs_lookup_open(&parent,
>  					&QSTR_LEN(open->op_fname,

This changes when a delegation is recalled relative to the access
check. oflags carries O_WRONLY or O_RDWR from op_share_access, and
vfs_lookup_open() opens the child with those flags. do_dentry_open()
does no permission check of its own and calls break_lease() on the
inode, so a conflicting delegation held by another client is recalled
here. Only afterwards does do_open_lookup() call do_open_permission()
and return NFS4ERR_ACCESS.

Before this patch a non-creating OPEN went through nfsd_lookup() and
do_open_permission() first, and the open that breaks the lease
happened later in nfsd_file_do_acquire(), where fh_verify() precedes
nfsd_open_break_lease(). So a client that can search the directory
but has no write permission on the file can now recall other clients'
write delegations on it by sending OPEN with SHARE_ACCESS_WRITE.

The create path has had this ordering since nfsd4_create_file() started
opening the file itself, but it was limited to opens that pass
NFSD_MAY_CREATE on the directory. Extending it to every OPEN makes it
reachable from any client with lookup access.

Could the access check move ahead of the open for the non-creating
case, or could the open be done O_RDONLY for the type check and the
share-access open deferred to the filecache as before?


> @@ -387,7 +396,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (IS_ERR(open->op_filp)) {
>  		status = nfserrno(PTR_ERR(open->op_filp));
>  		if (status == nfserr_wrong_type) {
> -			if (nfsd4_create_is_exclusive(open->op_createmode))
> +			if (nfsd4_create_is_exclusive(createmode))
>  				status = nfserr_exist;
>  			else
>  				status = nfsd_check_obj_isreg(parent.dentry);

On a re-export this arm changes the status a non-creating OPEN of a
directory returns. nfsd_check_obj_isreg(NULL) returns
nfserr_wrong_type, where nfsd_lookup() used to return nfserr_isdir.

The Linux client has no mapping for NFS4ERR_WRONG_TYPE, so
nfs4_map_errors() turns it into EIO. A "cat" of a re-exported directory
that is not yet in the server's dcache now fails with EIO instead of
EISDIR. NFSv4.0 clients are covered by the nfserr_symlink conversion in
do_open_lookup() and recover through a LOOKUP; v4.1 and later see the
change. Ceph splices before returning -EFTYPE, so it is not affected,
but nfs, gfs2 and smb all return -EFTYPE before instantiating anything.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)