[PATCH v2 01/14] fixes for vfs_lookup_open, and integration with nfsd

[PATCH v2 01/14] fixes for vfs_lookup_open, and integration with nfsd

[NeilBrown]

Greetings.

 NFSv4 has an "OPEN" request which combines lookup and create and
 truncate and permission checks etc much like the open() syscall.  When
 nfsd implements this, I want it to share a much code as (reasonably)
 possible with the open() paths - in particular I want it to use lookup_open() 
 and so that it uses ->atomic_open() the same way that other code does.
 (Longer term I want to make some locking changes and having all the code
 central makes that easier.)
 vfs_lookup_open() is a step towards that  but it isn't quite ready yet.
 This series aims to make it ready, then use it.

 A particular issue is that using __O_REGULAR exactly meets the needs of
 nfsd (it doesn't want to open anything else) but the errors returned by
 __O_REGULAR aren't what nfsd needs.  nfsd needs to know if it was a
 directory, or a symlink, or something else.

 I don't think __O_REGULAR should cause symlinks to result in -EFTYPE.
 If a symlink is found, then it should be followed.  That is what
 happens with filesystems that don't support ->atomic_open, but some
 ->atomic_open handlers return -EFTYPE for symlinks when __O_REGULAR is
 present.  I think that O_NOFOLLOW can affect how symlink are handled,
 but where possible it is best to just return the symlink to the caller
 and let it figure out what to do.

 For directories, nfsd wants EISDIR rather than EFTYPE.  It may be that
 user-space could benefit from seeing EISDIR too, but that is a separate
 issue.  So I want ->atomic_open to return EISDIR (if it returns an
 error at all) rather than EFTYPE if a directory is found.  VFS code can
 then map that to EFTYPE if needed.

 So the first few patches in this series improve the documentation for
 atomic_open and then make changes to nfs, gfs2, ceph, cifs to better
 match this documentation.  I would appreciate an Ack-by (or whatever
 else might be appropriate) from fs maintainers for those.

 Subsequent patches make changes to vfs_lookup_open() and then to nfsd
 to use it.

 A significant change here is that opening a file with
 O_NONBLOCK|O_CREAT will result in -EWOULDBLOCK if a delegation
 exists on the parent directory - currently it blocks.
 Jeff - could you comment on that change (07/14)?

 There are quite a lot of changes here since my previous post,
 particularly the changes to various filesystems.
 I've stopped trying to return the dentry from vfs_lookup_open()
 in the error case - no-one liked that.

 Thanks for your time,
 NeilBrown


 [PATCH v2 01/14] VFS: revise and expand documentation for
 [PATCH v2 02/14] nfs: correctly handle NFS4ERR_WRONG_TYPE from v4
 [PATCH v2 03/14] gfs2: simplify atomic_open handling.
 [PATCH v2 04/14] ceph: simplify atomic_open to use finish_no_open()
 [PATCH v2 05/14] cifs: allow -EISDIR precedence over -EFTYPE in
 [PATCH v2 06/14] vfs: add some allowed open flags to
 [PATCH v2 07/14] vfs: O_NONBLOCK|O_CREAT open shouldn't wait for
 [PATCH v2 08/14] vfs: don't return -ENODEV from vfs_lookup_open()
 [PATCH v2 09/14] vfs: change vfs_lookup_open() to use do_open(), not
 [PATCH v2 10/14] nfsd: make EEXIST checks in nfsd4_create_file() more
 [PATCH v2 11/14] nfsd: check for mountpoints after non-creating open.
 [PATCH v2 12/14] nfsd: switch NFS4 OPEN to use vfs_lookup_open()
 [PATCH v2 13/14] nfsd: change nfsd_check_obj_isreg() to use nfs error
 [PATCH v2 14/14] nfsd: use vfs_lookup_open() for non-creating open

[PATCH v2 01/14] VFS: revise and expand documentation for atomic_open.

[NeilBrown]

From: NeilBrown <neil@brown.name>

atomic_open is a complex operation which different filesystems implement
quite differently.  The available documentation doesn't give clear
guidance on how it should be implemented.

nfsd has a particular need to open only regular files, but to get
precise information about what was found if it wasn't a regular file.
This is slightly different to the syscall calling needs.  In particular
it suggests that __O_REGULAR shouldn't always result in -EFTYPE.

In any case that does involve creating open state, using
finish_no_open() is simplest as it reduces the need to check
__O_REGULAR, O_DIRECTORY, O_NOFOLLOW.

So refresh the documentation to give guidance on the choice between
finish_no_open, finish_open, and an error.  Efficiency always wins, but
when that isn't an issue, prefer finish_no_open().

Also clarify the required behaviour when __O_REGULAR is given.  This
should return -EISDIR if a directory is found as nfsd needs this.  If a
symlink is found then __O_REGULAR does NOT apply: O_NOFOLLOW must be
used to decided if it is safe to not return the looked-up dentry.

Signed-off-by: NeilBrown <neil@brown.name>
---
 Documentation/filesystems/vfs.rst | 67 ++++++++++++++++++++++++++-----
 fs/namei.c                        |  3 ++
 2 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index d3a93eec3945..00ada8cc85ae 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -599,17 +599,62 @@ otherwise noted.
 
 ``atomic_open``
 	called on the last component of an open.  Using this optional
-	method the filesystem can look up, possibly create and open the
-	file in one atomic operation.  If it wants to leave actual
-	opening to the caller (e.g. if the file turned out to be a
-	symlink, device, or just something filesystem won't do atomic
-	open for), it may signal this by returning finish_no_open(file,
-	dentry).  This method is only called if the last component is
-	negative or needs lookup.  Cached positive dentries are still
-	handled by f_op->open().  If the file was created, FMODE_CREATED
-	flag should be set in file->f_mode.  In case of O_EXCL the
-	method must only succeed if the file didn't exist and hence
-	FMODE_CREATED shall always be set on success.
+	method the filesystem can look up, create, truncate, and open
+	the file in one atomic operation.  This is needed if the
+	filesystem content can be changed asynchronously and
+	specifically if a negative dentry is not a guarantee that the
+	object doesn't exist.  It is also useful if it is possible to
+	perform combinations of revalidate, lookup, create, open, and
+	truncate more efficiently what with a sequence of individual
+	operations.
+
+	If the object found is not a file or directory, or if
+	lookup/create succeeded without establishing any "open" state,
+	then finish_no_open() should be called to confirm that the
+	dentry is ready to be handled by normal VFS processing.
+	FMODE_CREATED should be set in the "file" if the object was
+	created, and this will prevent further access permission checks,
+	or handling of O_TRUNC and O_EXCL.
+
+	If the lookup/create operation established some open state for a
+	file or directory, the open should be completed by calling
+	finish_open().  Passing NULL as the "open" function to
+	finish_open() is unlikely to be useful as that assumes that no
+	open state has been established.
+
+	atomic_open() may generate errors related to O_DIRECTORY,
+	__O_REGULAR, O_EXCL, O_NOFOLLOW but is not required to as the
+	caller will check those against the resulting dentry and
+	generate any error needed, possibly closing the file if it was
+	opened by finish_open().  atomic_open() is encouraged to handle
+	these flags only when doing so is more efficient than not.
+
+	If __O_REGULAR is handled, it should generate -EISDIR if the
+	name is known to be a directory or -EFTYPE if it is some other
+	non-regular file other than a symbolic link.  Handling of a
+	symbolic link should be guided by O_NOFOLLOW, not __O_REGULAR:
+	-ELOOP can be return if O_NOFOLLOW is set, otherwise the symlink
+	should be returned through finish_no_open().
+
+	The focus for atomic_open() is to provide the correct dentry and
+	to set FMODE_CREATED as accurately as possible.  If O_EXCL was
+	set, FMODE_CREATED should only be set if this operation
+	certainly created the object.  If O_EXCL was not set,
+	FMODE_CREATE should be set if it is possible that this operation
+	created the object.
+
+	This method is only called if the last component is negative or
+	needs lookup.  Cached positive dentries are still handled by
+	f_op->open().
+
+	If the dentry provided is negative (not in-lookup) and O_CREAT
+	isn't set, then there is no guarantee of exclusive access to the
+	dentry - another thread might call ->atomic_open() on the same
+	dentry at the same time.  If needed a filesystem can ensure this
+	doesn't happen by returning 0 from ->d_revalidate when that is
+	called with LOOKUP_OPEN on a negative dentry.  This will ensure
+	that ->atomic_open() only receives an in-lookup dentry, which
+	always ensures exclusive access.
 
 ``tmpfile``
 	called in the end of O_TMPFILE open().  Optional, equivalent to
diff --git a/fs/namei.c b/fs/namei.c
index d95249dd527c..0f69abb3743b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5007,6 +5007,9 @@ static struct file *path_openat(struct nameidata *nd,
 		error = -EINVAL;
 	}
 	fput_close(file);
+	if (error == -EISDIR &&
+	    (op->open_flag & __O_REGULAR))
+		error = -EFTYPE;
 	if (error == -EOPENSTALE) {
 		if (flags & LOOKUP_RCU)
 			error = -ECHILD;

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

[PATCH v2 02/14] nfs: correctly handle NFS4ERR_WRONG_TYPE from v4 OPEN request

[NeilBrown]

From: NeilBrown <neil@brown.name>

Now that we have the -EFTYPE error code, we can return it from
->open_context when the server returns NFS4ERR_WRONG_TYPE.
This can be directly returned when __O_REGULAR is in effect,
or can trigger a lookup and finish_no_open().

Also don't over-ride the err code when __O_REGULAR is in effect - nfsd
wants the see the original error, and VFS code will map when needed.

Finally don't consult __O_REGULAR for -ENOTDIR.  It isn't clear what
that means and is safest to leave the original handling.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfs/dir.c           | 8 ++++----
 fs/nfs_common/common.c | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 49394123bd09..11bcc922198e 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2191,11 +2191,11 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 			d_splice_alias(NULL, dentry);
 			break;
 		case -EISDIR:
-		case -ENOTDIR:
-			if (open_flags & __O_REGULAR) {
-				err = -EFTYPE;
+		case -EFTYPE:
+			if (open_flags & __O_REGULAR)
 				break;
-			}
+			goto no_open;
+		case -ENOTDIR:
 			goto no_open;
 		case -ELOOP:
 			if (!(open_flags & O_NOFOLLOW))
diff --git a/fs/nfs_common/common.c b/fs/nfs_common/common.c
index 0778743ae2c2..24add750c8d5 100644
--- a/fs/nfs_common/common.c
+++ b/fs/nfs_common/common.c
@@ -102,6 +102,7 @@ static const struct {
 	{ NFS4ERR_BADTYPE,	-EBADTYPE	},
 	{ NFS4ERR_SYMLINK,	-ELOOP		},
 	{ NFS4ERR_DEADLOCK,	-EDEADLK	},
+	{ NFS4ERR_WRONG_TYPE,	-EFTYPE		},
 };
 
 static const struct {
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 03/14] gfs2: simplify atomic_open handling.

[NeilBrown]

From: NeilBrown <neil@brown.name>

gfs2 incorrectly returns -EFTYPE for any non-regular when __O_REGULAR is
in force.  A symlink might not be an error, and nfsd needs to know if a
directory was found.

Neither this check, or the following check for a directory is needed.
Both cases are handled correctly by instantiating the dentry and passing
it to finish_no_open() - the caller will interpret the type.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/gfs2/inode.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index f361876c5583..7aa92bb31cf4 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -738,19 +738,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
 	inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
 	error = PTR_ERR(inode);
 	if (!IS_ERR(inode)) {
-		if (file && (file->f_flags & __O_REGULAR) &&
-		    !S_ISREG(inode->i_mode)) {
-			iput(inode);
-			inode = NULL;
-			error = -EFTYPE;
-			goto fail_gunlock;
-		}
-		if (S_ISDIR(inode->i_mode)) {
-			iput(inode);
-			inode = NULL;
-			error = -EISDIR;
-			goto fail_gunlock;
-		}
+		/*
+		 * This can only happen if "S_ISREG(mode) && !excl" so "file"
+		 * cannot be NULL.
+		 */
 		d_instantiate(dentry, inode);
 		error = 0;
 		if (file) {
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 04/14] ceph: simplify atomic_open to use finish_no_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

The cephfs protocol does not combine lookup/create and open into a
single request/response.  atomic_open() first performs the lookup/create
and then if a positive dentry results, this is passed to
finish_open(..., ceph_open) which simply performs a normal open.

The same effect can be achieved more simply be calling finish_no_open().
VFS code will call file_operations->open which is exactly ceph_open.

This fixes a potential bug where an open with __O_REGULAR but not
O_NOFOLLOW may treat a symlink as an error, and also avoids any
possibility of passing a non-regular file to ceph_open()
in ceph_finish_async_create() if __O_REGULAR were set.

It also ensures correct handling of any alias returned by
d_splice_alias().

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/ceph/file.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index bd3e3f5c269e..83ceb2906f85 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -754,7 +754,7 @@ static int ceph_finish_async_create(struct inode *dir, struct inode *inode,
 			d_drop(dentry);
 		discard_new_inode(inode);
 	} else {
-		struct dentry *dn;
+		struct dentry *dn = NULL;
 
 		doutc(cl, "d_adding new inode 0x%llx to 0x%llx/%s\n",
 		      vino.ino, ceph_ino(dir), dentry->d_name.name);
@@ -775,10 +775,9 @@ static int ceph_finish_async_create(struct inode *dir, struct inode *inode,
 			if (!d_unhashed(dentry))
 				d_drop(dentry);
 			dn = d_splice_alias(inode, dentry);
-			WARN_ON_ONCE(dn && dn != dentry);
 		}
 		file->f_mode |= FMODE_CREATED;
-		ret = finish_open(file, dentry, ceph_open);
+		ret = finish_no_open(file, dn);
 	}
 
 	spin_lock(&dentry->d_lock);
@@ -977,33 +976,25 @@ retry:
 	}
 	if (err)
 		goto out_req;
-	if (dn || d_really_is_negative(dentry) || d_is_symlink(dentry)) {
-		/* make vfs retry on splice, ENOENT, or symlink */
-		doutc(cl, "finish_no_open on dn %p\n", dn);
-		err = finish_no_open(file, dn);
-	} else {
-		if (IS_ENCRYPTED(dir) &&
-		    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
-			pr_warn_client(cl,
-				"Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
-				ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
-			goto out_req;
-		}
 
-		doutc(cl, "finish_open on dn %p\n", dn);
-		if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
-			struct inode *newino = d_inode(dentry);
+	if (IS_ENCRYPTED(dir) &&
+	    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
+		pr_warn_client(cl,
+			       "Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
+			       ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
+		goto out_req;
+	}
 
-			cache_file_layout(dir, newino);
-			ceph_init_inode_acls(newino, &as_ctx);
-			file->f_mode |= FMODE_CREATED;
-		}
-		if ((flags & __O_REGULAR) && !d_is_reg(dentry)) {
-			err = -EFTYPE;
-			goto out_req;
-		}
-		err = finish_open(file, dentry, ceph_open);
+	doutc(cl, "finish_no_open on dn %p\n", dn);
+	if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
+		struct inode *newino = d_inode(dentry);
+
+		cache_file_layout(dir, newino);
+		ceph_init_inode_acls(newino, &as_ctx);
+		file->f_mode |= FMODE_CREATED;
 	}
+	err = finish_no_open(file, dn);
+
 out_req:
 	ceph_mdsc_put_request(req);
 	iput(new_inode);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 05/14] cifs: allow -EISDIR precedence over -EFTYPE in __cifs_do_create()

[NeilBrown]

From: NeilBrown <neil@brown.name>

If atomic_open is given __O_REGULAR, we now would prefer -EISDIR
if a directory was found.  So move the S_ISDIR() tests earlier.

Also don't return -EFTYPE for S_ISLNK() - that should get -ELOOP and only
if O_NOFOLLOW.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/smb/client/dir.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 6fa6d48fdfd3..08425c9eea42 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -237,16 +237,18 @@ static int __cifs_do_create(struct inode *dir, struct dentry *direntry,
 				goto cifs_create_get_file_info;
 			}
 
-			if ((oflags & __O_REGULAR) && !S_ISREG(newinode->i_mode)) {
+			if (S_ISDIR(newinode->i_mode)) {
 				CIFSSMBClose(xid, tcon, fid->netfid);
 				iput(newinode);
-				return -EFTYPE;
+				return -EISDIR;
 			}
 
-			if (S_ISDIR(newinode->i_mode)) {
+			if ((oflags & __O_REGULAR) &&
+			    !S_ISREG(newinode->i_mode) &&
+			    !S_ISLNK(newinode->i_mode)) {
 				CIFSSMBClose(xid, tcon, fid->netfid);
 				iput(newinode);
-				return -EISDIR;
+				return -EFTYPE;
 			}
 
 			if (!S_ISREG(newinode->i_mode)) {
@@ -461,14 +463,16 @@ cifs_create_set_dentry:
 	}
 
 	if (newinode) {
-		if ((oflags & __O_REGULAR) && !S_ISREG(newinode->i_mode)) {
-			rc = -EFTYPE;
-			goto out_err;
-		}
 		if (S_ISDIR(newinode->i_mode)) {
 			rc = -EISDIR;
 			goto out_err;
 		}
+		if ((oflags & __O_REGULAR) &&
+		    !S_ISREG(newinode->i_mode) &&
+		    !S_ISLNK(newinode->i_mode)) {
+			rc = -EFTYPE;
+			goto out_err;
+		}
 	}
 
 	*inode = newinode;
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 06/14] 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.

Also O_NOFOLLOW as we don't/can't handle symlinks.  In fact we
should enforce O_NOFOLLOW for the same reason we enforce __O_REGULAR.

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

diff --git a/fs/namei.c b/fs/namei.c
index 0f69abb3743b..a08f37aca3e1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4632,11 +4632,12 @@ 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|O_NOFOLLOW),
 		  "open_flag has unsupported flags");
 
 	mode |= S_IFREG;
-	open_flag |= __O_REGULAR;
+	open_flag |= __O_REGULAR | O_NOFOLLOW;
 
 	error = lookup_noperm_common(last, parent->dentry);
 	if (error)
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 07/14] 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 a08f37aca3e1..b9fca38ad489 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4554,7 +4554,12 @@ retry:
 		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 v2 08/14] vfs: don't return -ENODEV from vfs_lookup_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

-ENODEV is not generally an error meaning "the object is a device"
and nfsd - the only intended caller of vfs_lookup_open() - does not
benefit from knowing it was a device file.  So return -EFTYPE in
that case.
Also switch to testing the dentry type rather than dereferencing the
inode to get the type.

-EISDIR is widely used to mean "the object is a directory which is
   not what is wanted".
-ELOOP is sometimes used elsewhere to mean "a symlink was found but
   cannot be handled".

Also remove note about ->atomic_open returning -EFTYPE as that now only
happens for non regular/dir/symlink.

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

diff --git a/fs/namei.c b/fs/namei.c
index b9fca38ad489..ba3e7e4b5fdb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4621,9 +4621,7 @@ out_dput:
  * 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.
+ * -EFTYPE : any other non-regular file was found, device-special, FIFO or SOCK
  *
  * Returns: the opened struct file, or an error.
  */
@@ -4671,24 +4669,12 @@ 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:
+	} else if (!d_is_reg(dentry)) {
+		error = -EFTYPE;
+		if (d_is_dir(dentry))
 			error = -EISDIR;
-			break;
-		case S_IFLNK:
+		if (d_is_symlink(dentry))
 			error = -ELOOP;
-			break;
-		case S_IFBLK:
-		case S_IFCHR:
-			error = -ENODEV;
-			break;
-		case S_IFIFO:
-		case S_IFSOCK:
-		default:
-			error = -EFTYPE;
-			break;
-		}
 	} else if (!(file->f_mode & FMODE_OPENED)) {
 		nd.path.dentry = dentry;
 		error = vfs_open(&nd.path, file);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 09/14] vfs: change vfs_lookup_open() to use do_open(), not vfs_open()

[NeilBrown]

From: NeilBrown <neil@brown.name>

The vfs_open() call misses permission checks which should happen before
an open is attempted (atomic_open does separate permission checks).

Much of the code in do_open() will have no effect as relevant LOOKUP_
flags aren't set.  truncation will be done (which nfsd is not expected
to use) along with some audit logs and security hook.  The important
benefit is getting the may_open() check.

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

diff --git a/fs/namei.c b/fs/namei.c
index ba3e7e4b5fdb..8ef2d44b6108 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4604,6 +4604,9 @@ out_dput:
 	goto out;
 }
 
+static int do_open(struct nameidata *nd,
+		   struct file *file, const struct open_flags *op);
+
 /**
  * vfs_lookup_open - open and possibly create a regular file
  * @parent: directory to contain file
@@ -4660,6 +4663,9 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	}
 	op.open_flag = open_flag;
 	op.mode = mode;
+	op.acc_mode = ACC_MODE(open_flag);
+	if (open_flag & O_TRUNC)
+		op.acc_mode |= MAY_WRITE;
 	dentry = lookup_open(&nd, file, &op);
 
 	if (IS_ERR(dentry))
@@ -4677,7 +4683,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 			error = -ELOOP;
 	} else if (!(file->f_mode & FMODE_OPENED)) {
 		nd.path.dentry = dentry;
-		error = vfs_open(&nd.path, file);
+		error = do_open(&nd, file, &op);
 	}
 	dput(dentry);
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 10/14] nfsd: make EEXIST checks in nfsd4_create_file() more consistent.

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_create_file() has two places that check if an "eexists" style
error is needed - only if create_mode is not NFS4_CREATE_UNCHECKED.

One if when checking the error code from do_lookup_open(), one when
checking if ->op_created wasn't set.

These are not consistent - one tests if op_createmode IS
NFS4_CREATE_UNCHECKED, the other tests if it isn't.

Rearrange the second piece of code so that the tests look similar.
This will make some following patches a bit cleaner.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3a82af381a8d..6e443503d0b7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -462,23 +462,24 @@ 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) {
-			/* NFSv4 protocol requires change attributes
-			 * even though no change happened.
-			 */
-			fh_fill_post_noop(fhp);
-
-			/*
-			 * In NFSv4, we don't want to truncate the file
-			 * now. This would be wrong if the OPEN fails for
-			 * some other reason. Furthermore, if the size is
-			 * nonzero, we should ignore it according to spec!
-			 */
-			open->op_truncate = (d_is_reg(child) &&
-					     (iap->ia_valid & ATTR_SIZE) &&
-					     !iap->ia_size);
-		} else
+		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
 			status = nfserr_exist;
+			goto out;
+		}
+		/* NFSv4 protocol requires change attributes
+		 * even though no change happened.
+		 */
+		fh_fill_post_noop(fhp);
+
+		/*
+		 * In NFSv4, we don't want to truncate the file
+		 * now. This would be wrong if the OPEN fails for
+		 * some other reason. Furthermore, if the size is
+		 * nonzero, we should ignore it according to spec!
+		 */
+		open->op_truncate = (d_is_reg(child) &&
+				     (iap->ia_valid & ATTR_SIZE) &&
+				     !iap->ia_size);
 		goto out;
 	}
 	/* file was created */
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 11/14] nfsd: check for mountpoints after non-creating open.

[NeilBrown]

From: NeilBrown <neil@brown.name>

A file can be a mountpoint for nfsd purposes when it isn't in the
dcache.  This happens when it is a junction (nfsd4_is_junction()).
So when we open a file and find that it already existed though
not in the dcache, we have to check if it is a mountpoint (or junction)
and potentially follow the junction.

So move the mountpoint crossing code in nfsd4_create_file() to a new
label at the end of the function and goto there both when an in-dcache
lookup finds an existing file, and when vfs_lookup_open() finds an
existing file.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 6e443503d0b7..3d37754f787b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -320,6 +320,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	};
 	__u32 v_mtime, v_atime;
 	__be32 status, create_status;
+	struct svc_export *exp;
 	int want_write_err;
 
 	if (name_is_dot_dotdot(open->op_fname, open->op_fnamelen))
@@ -339,21 +340,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 						    open->op_fnamelen),
 					  parent.dentry);
 		if (child && !IS_ERR(child) && d_is_reg(child) &&
-		    unlikely(nfsd_mountpoint(child, fhp->fh_export))) {
-			struct svc_export *exp = exp_get(fhp->fh_export);
-
-			status = nfsd_cross_mnt(rqstp, &child, &exp);
-			if (status == nfs_ok)
-				status = fh_compose(resfhp, exp,
-						    child, fhp);
-			fh_fill_post_noop(fhp);
-			open->op_truncate =
-				(iap->ia_valid & ATTR_SIZE) &&
-				!iap->ia_size;
-			dput(child);
-			exp_put(exp);
-			return status;
-		}
+		    unlikely(nfsd_mountpoint(child, fhp->fh_export)))
+			goto do_cross_mnt;
 		if (!IS_ERR(child))
 			dput(child);
 	}
@@ -466,6 +454,14 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 			status = nfserr_exist;
 			goto out;
 		}
+		/* We opened an existing file, it might be a junction. */
+		if (unlikely(nfsd_mountpoint(child, fhp->fh_export) == 1)) {
+			dget(child);
+			nfsd_filp_close(open->op_filp);
+			open->op_filp = NULL;
+			goto do_cross_mnt;
+		}
+
 		/* NFSv4 protocol requires change attributes
 		 * even though no change happened.
 		 */
@@ -511,6 +507,21 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 out:
 	nfsd_attrs_free(&attrs);
 	return status;
+
+do_cross_mnt:
+	exp = exp_get(fhp->fh_export);
+
+	status = nfsd_cross_mnt(rqstp, &child, &exp);
+	if (status == nfs_ok)
+		status = fh_compose(resfhp, exp,
+				    child, fhp);
+	fh_fill_post_noop(fhp);
+	open->op_truncate =
+		(iap->ia_valid & ATTR_SIZE) &&
+		!iap->ia_size;
+	dput(child);
+	exp_put(exp);
+	goto out;
 }
 
 /**
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 12/14] 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. Specifically:
 - it doesn't call ->lookup before ->atomic_open is called
 - it does pass LOOKUP_OPEN to ->d_revalidate
 - only gets exclusive lock on parent if name is NOT in dcache
These are minor.  The main benefit is code-sharing. In particular
it takes 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.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 3d37754f787b..50baf125d5f9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -250,52 +250,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 +266,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,
@@ -412,11 +366,11 @@ 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);
+	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);
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 13/14] nfsd: change 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.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 50baf125d5f9..40bd1179bf60 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -223,17 +223,15 @@ 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 = d_inode(child)->i_mode;
-
-	if (S_ISREG(mode))
+	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)
@@ -565,7 +563,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;
 
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH v2 14/14] 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_fill_post_attrs() and
fh_fill_post_noop() calls.

As ->op_createmode isn't defined when op_create is zero, we need a
local createmode 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 check_nfsd_access(), is already included for existing
names, this does not lose us anything.

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

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 40bd1179bf60..6be6ca473586 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -249,28 +249,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 creating
+ * (unchecked, guarded, and exclusive) and non-creating.  The "struct file"
+ * is openned here, and other open state for 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;
+	int createmode;
 	__be32 status, create_status;
 	struct svc_export *exp;
 	int want_write_err;
@@ -284,7 +286,27 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (status != nfs_ok)
 		return status;
 
-	if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
+	status = fh_fill_pre_attrs_unlocked(fhp);
+	if (status)
+		return status;
+
+	if (open->op_create) {
+		createmode = open->op_createmode;
+
+		create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
+		if (create_status == nfs_ok)
+			oflags |= O_CREAT;
+	} else {
+		/*
+		 * The only difference between no-create and CREATE_UNCHECKED
+		 * is the presence of O_CREAT.  In all other ways we can treat
+		 * them the same.
+		 */
+		createmode = NFS4_CREATE_UNCHECKED;
+		create_status = nfs_ok;
+	}
+
+	if (createmode == NFS4_CREATE_UNCHECKED) {
 		/*
 		 * If name is already in dcache we need to check for mountpoints
 		 */
@@ -305,7 +327,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	 * 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) {
@@ -337,7 +359,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;
 
 		/*
@@ -359,11 +381,6 @@ 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;
-
 	open->op_filp = vfs_lookup_open(&parent,
 					&QSTR_LEN(open->op_fname,
 						  open->op_fnamelen),
@@ -372,7 +389,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (IS_ERR(open->op_filp)) {
 		int hosterr = PTR_ERR(open->op_filp);
 
-		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
+		if (createmode != NFS4_CREATE_UNCHECKED) {
 			switch (hosterr) {
 			case -EISDIR:
 			case -ELOOP:
@@ -395,14 +412,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 (open->op_createmode != NFS4_CREATE_UNCHECKED) {
+		if (createmode != NFS4_CREATE_UNCHECKED) {
 			status = nfserr_exist;
 			goto out;
 		}
@@ -521,46 +538,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 v2 03/14] gfs2: simplify atomic_open handling.

[Andreas Gruenbacher]

Neil,

On Sat, Sep 19, 2026 at 4:31 AM NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> gfs2 incorrectly returns -EFTYPE for any non-regular when __O_REGULAR is
> in force.  A symlink might not be an error, and nfsd needs to know if a
> directory was found.
>
> Neither this check, or the following check for a directory is needed.
> Both cases are handled correctly by instantiating the dentry and passing
> it to finish_no_open() - the caller will interpret the type.
>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/gfs2/inode.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index f361876c5583..7aa92bb31cf4 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -738,19 +738,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>         inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
>         error = PTR_ERR(inode);
>         if (!IS_ERR(inode)) {
> -               if (file && (file->f_flags & __O_REGULAR) &&
> -                   !S_ISREG(inode->i_mode)) {
> -                       iput(inode);
> -                       inode = NULL;
> -                       error = -EFTYPE;
> -                       goto fail_gunlock;
> -               }
> -               if (S_ISDIR(inode->i_mode)) {
> -                       iput(inode);
> -                       inode = NULL;
> -                       error = -EISDIR;
> -                       goto fail_gunlock;
> -               }

as you note below, when the inode exists but it isn't a regular file,
we won't even get here (gfs2_dir_search() will have returned
ERR_PTR(-EEXIST)). So the above two if statements are dead code, which
contradicts the patch description. Could you please fix that?

> +               /*
> +                * This can only happen if "S_ISREG(mode) && !excl" so "file"
> +                * cannot be NULL.
> +                */

Is that comment useful without removing the below NULL check for file?

>                 d_instantiate(dentry, inode);
>                 error = 0;
>                 if (file) {
> --
> 2.50.0.107.gf914562f5916.dirty
>

Thanks,
Andreas

Re: [PATCH v2 03/14] gfs2: simplify atomic_open handling.

[NeilBrown]

On Sun, 20 Sep 2026, Andreas Gruenbacher wrote:
> Neil,
> 
> On Sat, Sep 19, 2026 at 4:31 AM NeilBrown <neilb@ownmail.net> wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > gfs2 incorrectly returns -EFTYPE for any non-regular when __O_REGULAR is
> > in force.  A symlink might not be an error, and nfsd needs to know if a
> > directory was found.
> >
> > Neither this check, or the following check for a directory is needed.
> > Both cases are handled correctly by instantiating the dentry and passing
> > it to finish_no_open() - the caller will interpret the type.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/gfs2/inode.c | 17 ++++-------------
> >  1 file changed, 4 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > index f361876c5583..7aa92bb31cf4 100644
> > --- a/fs/gfs2/inode.c
> > +++ b/fs/gfs2/inode.c
> > @@ -738,19 +738,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> >         inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
> >         error = PTR_ERR(inode);
> >         if (!IS_ERR(inode)) {
> > -               if (file && (file->f_flags & __O_REGULAR) &&
> > -                   !S_ISREG(inode->i_mode)) {
> > -                       iput(inode);
> > -                       inode = NULL;
> > -                       error = -EFTYPE;
> > -                       goto fail_gunlock;
> > -               }
> > -               if (S_ISDIR(inode->i_mode)) {
> > -                       iput(inode);
> > -                       inode = NULL;
> > -                       error = -EISDIR;
> > -                       goto fail_gunlock;
> > -               }
> 
> as you note below, when the inode exists but it isn't a regular file,
> we won't even get here (gfs2_dir_search() will have returned
> ERR_PTR(-EEXIST)). So the above two if statements are dead code, which
> contradicts the patch description. Could you please fix that?

There are two different modes here: the "mode" that was requested and
the "inode->i_mode" that was found.
This branch will only be taken if "mode" is "S_IFREG", but the inode
that is found could be anything.

> 
> > +               /*
> > +                * This can only happen if "S_ISREG(mode) && !excl" so "file"
> > +                * cannot be NULL.
> > +                */
> 
> Is that comment useful without removing the below NULL check for file?

Fair point.  I'll remove the NULL check.

I'd feel more comfortable if gfs2_symlink(), gfs2_mkdir(), and
gfs2_mknod() all passed "1" (or "true") for the "excl" arg.
Then it would be even more obvious that we don't need to test
for NULL, as all callers that pass a NULL file, pass a true excl.
(you could remove the "!S_ISREG(mode) ||" which would make the code
look cleaner).
But that is beyond the scope of this patch.

hmm... you could even pass "!file || excl" to gfs2_dir_search() to make
it impossible to use file if it were NULL :-)

Thanks,
NeilBrown

> 
> >                 d_instantiate(dentry, inode);
> >                 error = 0;
> >                 if (file) {
> > --
> > 2.50.0.107.gf914562f5916.dirty
> >
> 
> Thanks,
> Andreas
> 
> 

Re: [PATCH v2 03/14] gfs2: simplify atomic_open handling.

[Andreas Gruenbacher]

On Sun, Sep 20, 2026 at 12:22 AM NeilBrown <neilb@ownmail.net> wrote:
> On Sun, 20 Sep 2026, Andreas Gruenbacher wrote:
> > Neil,
> >
> > On Sat, Sep 19, 2026 at 4:31 AM NeilBrown <neilb@ownmail.net> wrote:
> > > From: NeilBrown <neil@brown.name>
> > >
> > > gfs2 incorrectly returns -EFTYPE for any non-regular when __O_REGULAR is
> > > in force.  A symlink might not be an error, and nfsd needs to know if a
> > > directory was found.
> > >
> > > Neither this check, or the following check for a directory is needed.
> > > Both cases are handled correctly by instantiating the dentry and passing
> > > it to finish_no_open() - the caller will interpret the type.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > >  fs/gfs2/inode.c | 17 ++++-------------
> > >  1 file changed, 4 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > > index f361876c5583..7aa92bb31cf4 100644
> > > --- a/fs/gfs2/inode.c
> > > +++ b/fs/gfs2/inode.c
> > > @@ -738,19 +738,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> > >         inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
> > >         error = PTR_ERR(inode);
> > >         if (!IS_ERR(inode)) {
> > > -               if (file && (file->f_flags & __O_REGULAR) &&
> > > -                   !S_ISREG(inode->i_mode)) {
> > > -                       iput(inode);
> > > -                       inode = NULL;
> > > -                       error = -EFTYPE;
> > > -                       goto fail_gunlock;
> > > -               }
> > > -               if (S_ISDIR(inode->i_mode)) {
> > > -                       iput(inode);
> > > -                       inode = NULL;
> > > -                       error = -EISDIR;
> > > -                       goto fail_gunlock;
> > > -               }
> >
> > as you note below, when the inode exists but it isn't a regular file,
> > we won't even get here (gfs2_dir_search() will have returned
> > ERR_PTR(-EEXIST)). So the above two if statements are dead code, which
> > contradicts the patch description. Could you please fix that?
>
> There are two different modes here: the "mode" that was requested and
> the "inode->i_mode" that was found.
> This branch will only be taken if "mode" is "S_IFREG", but the inode
> that is found could be anything.

Ah right, we could be looking for a regular inode non-exclusively and
get back any inode type.

> >
> > > +               /*
> > > +                * This can only happen if "S_ISREG(mode) && !excl" so "file"
> > > +                * cannot be NULL.
> > > +                */
> >
> > Is that comment useful without removing the below NULL check for file?
>
> Fair point.  I'll remove the NULL check.
>
> I'd feel more comfortable if gfs2_symlink(), gfs2_mkdir(), and
> gfs2_mknod() all passed "1" (or "true") for the "excl" arg.
> Then it would be even more obvious that we don't need to test
> for NULL, as all callers that pass a NULL file, pass a true excl.
> (you could remove the "!S_ISREG(mode) ||" which would make the code
> look cleaner).
>
> But that is beyond the scope of this patch.

Yes, makes sense. I'll happily clean that up.

In fact, could just remove the two if statements in this patch queue?
We can then clean up the rest on gfs2 for-next without causing a merge
conflict.

Feel free to send a cleanup, or let me know if I should write that.

> hmm... you could even pass "!file || excl" to gfs2_dir_search() to make
> it impossible to use file if it were NULL :-)

Right.

Thanks,
Andreas

Re: [PATCH v2 12/14] nfsd: switch NFS4 OPEN to use vfs_lookup_open()

[Chuck Lever]

On Sat, Sep 19, 2026, NeilBrown wrote:
> 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. Specifically:
>  - it doesn't call ->lookup before ->atomic_open is called
>  - it does pass LOOKUP_OPEN to ->d_revalidate
>  - only gets exclusive lock on parent if name is NOT in dcache

Thanks for listing these. Is the third item accurate, though?
vfs_lookup_open() calls lookup_open() with no lookup_fast() step ahead of
it, and lookup_open() picks the lock from O_CREAT alone, before it
consults the dcache:

fs/namei.c:lookup_open() {
    ...
	if (open_flag & O_CREAT)
		inode_lock(dir_inode);
	else
		inode_lock_shared(dir_inode);
    ...
	dentry = d_lookup(dir, &nd->last);
    ...
}

AFAICT the parent is locked shared only when nfsd4_create_file() has
already cleared O_CREAT because the NFSD_MAY_CREATE fh_verify()
failed.


> These are minor.  The main benefit is code-sharing. In particular
> it takes 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.

"that that" is still here from v1. Also, s/break_lease of/break_lease() or/


> We need to mask any type out of "mode" else vfs_lookup_open() will warn.
>
> Signed-off-by: NeilBrown <neil@brown.name>

[ ... ]

> @@ -412,11 +366,11 @@ 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);
> +	open->op_filp = vfs_lookup_open(&parent,
> +					&QSTR_LEN(open->op_fname,
> +						  open->op_fnamelen),
> +					oflags,
> +					open->op_iattr.ia_mode & S_IALLUGO);

Can this fail an OPEN that nfsd_permission() would have allowed?

With 09/14 applied, an existing file that ->atomic_open did not open goes
through vfs_lookup_open()->do_open()->may_open(), which does:

	error = inode_permission(idmap, inode, MAY_OPEN | acc_mode);

do_open() zeroes acc_mode only when FMODE_CREATED is set. The removed
do_lookup_open() used dentry_open() for a positive dentry, which makes no
permission check, the same as __nfsd_open(). The permission decision was
left to do_open_lookup()->do_open_permission()->nfsd_permission(), which
runs after nfsd4_create_file() returns. Now may_open() fails first, and
the two relaxations in nfsd_permission() are never consulted.

The case I am most concerned about is a replayed exclusive create. An
NFSv4.0 EXCLUSIVE4 OPEN carries no attributes, so:

fs/nfsd/nfs4proc.c:nfsd4_create_file() {
    ...
	if (!(iap->ia_valid & ATTR_MODE))
		iap->ia_mode = 0;
    ...
}

and the file is created with mode 0000 until the client's SETATTR
arrives. If the client resends the OPEN in that window (after a server
restart, say), the dentry is positive and FMODE_CREATED is clear, so
may_open() returns -EACCES for a non-root owner. The verifier comparison
further down:

	if (!open->op_created &&
	    nfsd4_create_is_exclusive(open->op_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;

is never reached, so do_open_lookup() never gets to add
NFSD_MAY_OWNER_OVERRIDE, and the retry gets NFS4ERR_ACCESS where it
used to succeed.

The other relaxation is NFSD_MAY_READ_IF_EXEC, which do_open_permission()
always sets. An UNCHECKED create for read that finds an existing mode
0111 file now fails in may_open() with -EACCES. That is rare for a
creating OPEN, but would 14/14 not make it the common case, since a
client reading an execute-only binary sends a non-creating OPEN?

Also, vfs_lookup_open() calls do_open() only when FMODE_OPENED is clear.
Does that mean the result differs by exported filesystem? ext4, xfs,
btrfs and tmpfs have no ->atomic_open and get the new may_open() check,
while a re-exported NFS or a fuse export skips it.

Would it work to let this caller reach do_open() with an acc_mode of
zero, so that nfsd_permission() stays the only authority for NFS
requests? If the new check is wanted on the nfsd side, the patch
description needs to say so and explain how the exclusive replay case
is handled.


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

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

[Chuck Lever]

On Sat, Sep 19, 2026, NeilBrown wrote:
> 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.

[ ... ]

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

The code below sets createmode to NFS4_CREATE_UNCHECKED for the
non-creating case, not -1. Is this paragraph left over from v1?

Also, is ->op_createmode really undefined there? nfsd4_decode_open()
starts with memset(open, 0, sizeof(*open)), and NFS4_CREATE_UNCHECKED
is 0, so op_createmode already reads as UNCHECKED for a non-creating
OPEN. If that can be relied on, the local createmode can go away, along
with the op_create test added to do_open_lookup() at the end of this
patch. If the intent is to stop depending on the decoder's memset,
the description should say that instead.


> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 40bd1179bf60..6be6ca473586 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -249,28 +249,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 creating
> + * (unchecked, guarded, and exclusive) and non-creating.  The "struct file"
> + * is openned here, and other open state for is subsequently fabricated in
> + * nfsd4_process_open2().

s/openned/opened/, and "other open state for is subsequently" has lost
a word or gained one.


> @@ -359,11 +381,6 @@ 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;
> -
>  	open->op_filp = vfs_lookup_open(&parent,
>  					&QSTR_LEN(open->op_fname,
>  						  open->op_fnamelen),

With every open-by-name now coming through here, does may_open() get
in ahead of nfsd_permission()? For a file that already exists,
vfs_lookup_open()->do_open() calls may_open() with MAY_READ or
MAY_WRITE. The old non-create path was nfsd_lookup(), then
do_open_permission(), and later __nfsd_open()->dentry_open(), which
does not call may_open(). So nfsd_permission() had the only say, and
there are two places where it is deliberately more generous than
inode_permission():

 - do_open_permission() always sets NFSD_MAY_READ_IF_EXEC, so that a
   client can read a mode 0111 binary in order to execute it. Now
   inode_permission(MAY_READ) fails in may_open() before we get that
   far, and the OPEN returns NFS4ERR_ACCESS.

 - do_open_lookup() sets NFSD_MAY_OWNER_OVERRIDE for
   CLAIM_DELEGATE_CUR. That claim type is never a create, so it always
   takes this path. An owner re-opening for write after a chmod 0444
   would fail in may_open().

That is from auditing the code; I have not run either case. I think an
UNCHECKED create that finds an existing file has had the same exposure
since 12/14, but here it becomes the common case. Could
vfs_lookup_open() let the caller skip may_open(), perhaps by passing an
acc_mode of 0 the way do_open() already does for FMODE_CREATED? Or
should the non-creating case stay with nfsd_lookup()?


> @@ -395,14 +412,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 (open->op_createmode != NFS4_CREATE_UNCHECKED) {
> +		if (createmode != NFS4_CREATE_UNCHECKED) {
>  			status = nfserr_exist;
>  			goto out;
>  		}

This one is not new here, it arrived with 11/14. This patch makes
it reachable from every non-creating OPEN. Just below this hunk:

		if (unlikely(nfsd_mountpoint(child, fhp->fh_export) == 1)) {
			dget(child);
			nfsd_filp_close(open->op_filp);
			open->op_filp = NULL;
			goto do_cross_mnt;
		}

By this point fh_compose(resfhp, fhp->fh_export, child, fhp) has
already succeeded, and do_cross_mnt calls fh_compose(resfhp, ...) a
second time. When fh_dentry is already set, fh_compose() prints
"fh_compose: fh ... not initialized!" and then overwrites fh_dentry and
fh_export. Does that leak the first dentry and svc_export references?
Would an fh_put(resfhp) before the goto be enough, or is it better to
do the nfsd_mountpoint() check before the first fh_compose()?


> @@ -521,46 +538,34 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru

> +	/* FIXME: check session persistence and pnfs flags.
> +	 * The nfsv4.1 spec requires the following semantics:
> +	 *
> +	 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
> +	 * Reply Cache  | server |                 |

This table is only about create modes. Out of the op_create block it
now reads as though it applies to every OPEN. Could it say "for
creating OPENs", or move into the op_create arm of nfsd4_open_file()?


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

Re: [PATCH v2 03/14] gfs2: simplify atomic_open handling.

[NeilBrown]

On Mon, 21 Sep 2026, Andreas Gruenbacher wrote:
> On Sun, Sep 20, 2026 at 12:22 AM NeilBrown <neilb@ownmail.net> wrote:
> > On Sun, 20 Sep 2026, Andreas Gruenbacher wrote:
> > > Neil,
> > >
> > > On Sat, Sep 19, 2026 at 4:31 AM NeilBrown <neilb@ownmail.net> wrote:
> > > > From: NeilBrown <neil@brown.name>
> > > >
> > > > gfs2 incorrectly returns -EFTYPE for any non-regular when __O_REGULAR is
> > > > in force.  A symlink might not be an error, and nfsd needs to know if a
> > > > directory was found.
> > > >
> > > > Neither this check, or the following check for a directory is needed.
> > > > Both cases are handled correctly by instantiating the dentry and passing
> > > > it to finish_no_open() - the caller will interpret the type.
> > > >
> > > > Signed-off-by: NeilBrown <neil@brown.name>
> > > > ---
> > > >  fs/gfs2/inode.c | 17 ++++-------------
> > > >  1 file changed, 4 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> > > > index f361876c5583..7aa92bb31cf4 100644
> > > > --- a/fs/gfs2/inode.c
> > > > +++ b/fs/gfs2/inode.c
> > > > @@ -738,19 +738,10 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
> > > >         inode = gfs2_dir_search(dir, &dentry->d_name, !S_ISREG(mode) || excl);
> > > >         error = PTR_ERR(inode);
> > > >         if (!IS_ERR(inode)) {
> > > > -               if (file && (file->f_flags & __O_REGULAR) &&
> > > > -                   !S_ISREG(inode->i_mode)) {
> > > > -                       iput(inode);
> > > > -                       inode = NULL;
> > > > -                       error = -EFTYPE;
> > > > -                       goto fail_gunlock;
> > > > -               }
> > > > -               if (S_ISDIR(inode->i_mode)) {
> > > > -                       iput(inode);
> > > > -                       inode = NULL;
> > > > -                       error = -EISDIR;
> > > > -                       goto fail_gunlock;
> > > > -               }
> > >
> > > as you note below, when the inode exists but it isn't a regular file,
> > > we won't even get here (gfs2_dir_search() will have returned
> > > ERR_PTR(-EEXIST)). So the above two if statements are dead code, which
> > > contradicts the patch description. Could you please fix that?
> >
> > There are two different modes here: the "mode" that was requested and
> > the "inode->i_mode" that was found.
> > This branch will only be taken if "mode" is "S_IFREG", but the inode
> > that is found could be anything.
> 
> Ah right, we could be looking for a regular inode non-exclusively and
> get back any inode type.
> 
> > >
> > > > +               /*
> > > > +                * This can only happen if "S_ISREG(mode) && !excl" so "file"
> > > > +                * cannot be NULL.
> > > > +                */
> > >
> > > Is that comment useful without removing the below NULL check for file?
> >
> > Fair point.  I'll remove the NULL check.
> >
> > I'd feel more comfortable if gfs2_symlink(), gfs2_mkdir(), and
> > gfs2_mknod() all passed "1" (or "true") for the "excl" arg.
> > Then it would be even more obvious that we don't need to test
> > for NULL, as all callers that pass a NULL file, pass a true excl.
> > (you could remove the "!S_ISREG(mode) ||" which would make the code
> > look cleaner).
> >
> > But that is beyond the scope of this patch.
> 
> Yes, makes sense. I'll happily clean that up.
> 
> In fact, could just remove the two if statements in this patch queue?
> We can then clean up the rest on gfs2 for-next without causing a merge
> conflict.

I've changed the comment to not mention the !S_ISREG() || excl condition
directly so it can be changed without confusion.  Instead it now says:

		/*
		 * Above gfs2_dir_search() can only return an inode when
		 * called from gfs2_so file cannot be NULL.
		 */

It shouldn't conflict with your patch.
I'll repost shortly.

Thanks,
NeilBrown


> 
> Feel free to send a cleanup, or let me know if I should write that.
> 
> > hmm... you could even pass "!file || excl" to gfs2_dir_search() to make
> > it impossible to use file if it were NULL :-)
> 
> Right.
> 
> Thanks,
> Andreas
> 
> 

Re: [PATCH v2 12/14] nfsd: switch NFS4 OPEN to use vfs_lookup_open()

[NeilBrown]

On Mon, 21 Sep 2026, Chuck Lever wrote:
> On Sat, Sep 19, 2026, NeilBrown wrote:
> > 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. Specifically:
> >  - it doesn't call ->lookup before ->atomic_open is called
> >  - it does pass LOOKUP_OPEN to ->d_revalidate
> >  - only gets exclusive lock on parent if name is NOT in dcache
> 
> Thanks for listing these. Is the third item accurate, though?
> vfs_lookup_open() calls lookup_open() with no lookup_fast() step ahead of
> it, and lookup_open() picks the lock from O_CREAT alone, before it
> consults the dcache:
> 
> fs/namei.c:lookup_open() {
>     ...
> 	if (open_flag & O_CREAT)
> 		inode_lock(dir_inode);
> 	else
> 		inode_lock_shared(dir_inode);
>     ...
> 	dentry = d_lookup(dir, &nd->last);
>     ...
> }
> 
> AFAICT the parent is locked shared only when nfsd4_create_file() has
> already cleared O_CREAT because the NFSD_MAY_CREATE fh_verify()
> failed.

Yes, I was getting ahead of myself - or maybe thinking of
lookup_fast_for_open(), which we aren't using.
I think my proposed patches for locking changes will avoid the exclusive
lock in lookup_open(), but as you say we are not there yet.
I've dropped that line.

> 
> 
> > These are minor.  The main benefit is code-sharing. In particular
> > it takes 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.
> 
> "that that" is still here from v1. Also, s/break_lease of/break_lease() or/

Thanks.

> 
> 
> > We need to mask any type out of "mode" else vfs_lookup_open() will warn.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> 
> [ ... ]
> 
> > @@ -412,11 +366,11 @@ 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);
> > +	open->op_filp = vfs_lookup_open(&parent,
> > +					&QSTR_LEN(open->op_fname,
> > +						  open->op_fnamelen),
> > +					oflags,
> > +					open->op_iattr.ia_mode & S_IALLUGO);
> 
> Can this fail an OPEN that nfsd_permission() would have allowed?

Yes.  As you explain in detail (thanks) this is a problem.

This highlights a problem with using nfsv4 to re-export a remote filesystem.

Any non-standard permission checking we do in nfsd to handle reading
execute-only files, or opening recently created files without the
requested access, also needs to be performed on the server for a
re-exported filesystem.

Re-exporting NFS is probably safe because nfsd is already relaxed about
these permissions, with NFSD_MAY_OWNER_OVERRIDE and NFSD_MAY_READ_IF_EXEC.
But other filesystems may not be.

I wonder if we should communicate this through the VFS so we can at
least tell ->atomic_open and ->permission that it can be a little
relaxed about certain permission checks...

I don't know what is best, but will dwell on if for a while.

Thanks,
NeilBrown


> 
> With 09/14 applied, an existing file that ->atomic_open did not open goes
> through vfs_lookup_open()->do_open()->may_open(), which does:
> 
> 	error = inode_permission(idmap, inode, MAY_OPEN | acc_mode);
> 
> do_open() zeroes acc_mode only when FMODE_CREATED is set. The removed
> do_lookup_open() used dentry_open() for a positive dentry, which makes no
> permission check, the same as __nfsd_open(). The permission decision was
> left to do_open_lookup()->do_open_permission()->nfsd_permission(), which
> runs after nfsd4_create_file() returns. Now may_open() fails first, and
> the two relaxations in nfsd_permission() are never consulted.
> 
> The case I am most concerned about is a replayed exclusive create. An
> NFSv4.0 EXCLUSIVE4 OPEN carries no attributes, so:
> 
> fs/nfsd/nfs4proc.c:nfsd4_create_file() {
>     ...
> 	if (!(iap->ia_valid & ATTR_MODE))
> 		iap->ia_mode = 0;
>     ...
> }
> 
> and the file is created with mode 0000 until the client's SETATTR
> arrives. If the client resends the OPEN in that window (after a server
> restart, say), the dentry is positive and FMODE_CREATED is clear, so
> may_open() returns -EACCES for a non-root owner. The verifier comparison
> further down:
> 
> 	if (!open->op_created &&
> 	    nfsd4_create_is_exclusive(open->op_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;
> 
> is never reached, so do_open_lookup() never gets to add
> NFSD_MAY_OWNER_OVERRIDE, and the retry gets NFS4ERR_ACCESS where it
> used to succeed.
> 
> The other relaxation is NFSD_MAY_READ_IF_EXEC, which do_open_permission()
> always sets. An UNCHECKED create for read that finds an existing mode
> 0111 file now fails in may_open() with -EACCES. That is rare for a
> creating OPEN, but would 14/14 not make it the common case, since a
> client reading an execute-only binary sends a non-creating OPEN?
> 
> Also, vfs_lookup_open() calls do_open() only when FMODE_OPENED is clear.
> Does that mean the result differs by exported filesystem? ext4, xfs,
> btrfs and tmpfs have no ->atomic_open and get the new may_open() check,
> while a re-exported NFS or a fuse export skips it.
> 
> Would it work to let this caller reach do_open() with an acc_mode of
> zero, so that nfsd_permission() stays the only authority for NFS
> requests? If the new check is wanted on the nfsd side, the patch
> description needs to say so and explain how the exclusive replay case
> is handled.
> 
> 
> -- 
> Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)
> 

Re: [PATCH v2 12/14] nfsd: switch NFS4 OPEN to use vfs_lookup_open()

[NeilBrown]

On Wed, 23 Sep 2026, NeilBrown wrote:
> On Mon, 21 Sep 2026, Chuck Lever wrote:
> > 
> > Can this fail an OPEN that nfsd_permission() would have allowed?
> 
> Yes.  As you explain in detail (thanks) this is a problem.
> 
> This highlights a problem with using nfsv4 to re-export a remote filesystem.
> 
> Any non-standard permission checking we do in nfsd to handle reading
> execute-only files, or opening recently created files without the
> requested access, also needs to be performed on the server for a
> re-exported filesystem.
> 
> Re-exporting NFS is probably safe because nfsd is already relaxed about
> these permissions, with NFSD_MAY_OWNER_OVERRIDE and NFSD_MAY_READ_IF_EXEC.
> But other filesystems may not be.
> 
> I wonder if we should communicate this through the VFS so we can at
> least tell ->atomic_open and ->permission that it can be a little
> relaxed about certain permission checks...
> 
> I don't know what is best, but will dwell on if for a while.

This is the sort of thing I'm thinking of.
The FMODE_SERVE_FILE flag isn't used at the moment but it could be.
e.g. nfs4_opendata_access() could use it to allow an FMODE_READ open
to be satisfied by NFS4_ACCESS_EXECUTE.
Currently if you re-export NFSv4 and the client tries to execute
a file which it doesn't have read-access to, then it will fail.
I haven't actually tried this - maybe I should.

Anything reflections on this approach most welcome.

Thanks,
NeilBrown



diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
index 4a58450d8310..6afb7cdb2be8 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -642,6 +642,11 @@ otherwise noted.
 	FMODE_CREATED should be set if it is possible that this operation
 	created the object.
 
+	If atomic_open() calls finish_open() then it must have performed
+	all necessary access permission checks.  If FMODE_SERVE_FILE
+	is set in file->f_mode, this it should assume MAY_SERVE_FILE if 
+	possible.
+
 	This method is only called if the last component is negative or
 	needs lookup.  Cached positive dentries are still handled by
 	f_op->open().
diff --git a/fs/namei.c b/fs/namei.c
index 8ef2d44b6108..16c6ee8cfd74 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -441,6 +441,11 @@ static int acl_permission_check(struct mnt_idmap *idmap,
 	unsigned int mode = inode->i_mode;
 	vfsuid_t vfsuid;
 
+	if (mask & MAY_SERVE_FILE) {
+		/* 'x' permission grant 'r' permission */
+		mode |= (mode & 0111) << 2;
+	}
+
 	/*
 	 * Common cheap case: everybody has the requested
 	 * rights, and there are no ACLs to check. No need
@@ -468,7 +473,12 @@ static int acl_permission_check(struct mnt_idmap *idmap,
 	if (likely(vfsuid_eq_kuid(vfsuid, current_fsuid()))) {
 		mask &= 7;
 		mode >>= 6;
-		return (mask & ~mode) ? -EACCES : 0;
+		if (likely((mask & ~mode) == 0))
+			return 0;
+		if (mask & MAY_SERVE_FILE)
+			/* Owner always gets access */
+			return 0;
+		return -EACCES;
 	}
 
 	/* Do we have ACL's? */
@@ -4652,6 +4662,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	file = alloc_empty_file(open_flag, current_cred());
 	if (IS_ERR(file))
 		return file;
+	file->f_mode |= FMODE_SERVE_FILE;
 
 	nd.path = *parent;
 	nd.last = *last;
@@ -4666,6 +4677,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
 	op.acc_mode = ACC_MODE(open_flag);
 	if (open_flag & O_TRUNC)
 		op.acc_mode |= MAY_WRITE;
+	op.acc_mode |= MAY_SERVE_FILE;
 	dentry = lookup_open(&nd, file, &op);
 
 	if (IS_ERR(dentry))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f9d1e05e8ae6..83c36a5dc51f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -98,6 +98,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define MAY_CHDIR		0x00000040
 /* called from RCU mode, don't block */
 #define MAY_NOT_BLOCK		0x00000080
+/* Passed by nfsd - owner may have any access, X access gives R */
+#define	MAY_SERVE_FILE		0x00000100
 
 /*
  * flags in file.f_mode.  Note that FMODE_READ and FMODE_WRITE must correspond
@@ -120,8 +122,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 #define FMODE_WRITE_RESTRICTED	((__force fmode_t)(1 << 6))
 /* File supports atomic writes */
 #define FMODE_CAN_ATOMIC_WRITE	((__force fmode_t)(1 << 7))
-
-/* FMODE_* bit 8 */
+/* File used to serve file to a client, so MAY_SERVE_FILE applies */
+#define FMODE_SERVE_FILE	((__force fmode_t)(1 << 8))
 
 /* 32bit hashes as llseek() offset (for directories) */
 #define FMODE_32BITHASH         ((__force fmode_t)(1 << 9))

Re: [PATCH v2 05/14] cifs: allow -EISDIR precedence over -EFTYPE in __cifs_do_create()

[Namjae Jeon]

On Sat, Sep 19, 2026 at 11:25 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> If atomic_open is given __O_REGULAR, we now would prefer -EISDIR
> if a directory was found.  So move the S_ISDIR() tests earlier.
>
> Also don't return -EFTYPE for S_ISLNK() - that should get -ELOOP and only
> if O_NOFOLLOW.
Simply exempting symlinks from the __O_REGULAR check in cifs is not
sufficient. Once this check allows the symlink through,
cifs_atomic_open() still calls finish_open() on it, setting
FMODE_OPENED. open_last_lookups() then skips symlink traversal, so
do_open() returns -EFTYPE before may_open() can return -ELOOP. This
path also reaches cifs_new_fileinfo(), which pins the symlink dentry
with dget(). The symlink's file operations do not provide CIFS's
.release handler, so the failed open does not call cifs_close() to
release that reference.

Re: [PATCH v2 05/14] cifs: allow -EISDIR precedence over -EFTYPE in __cifs_do_create()

[NeilBrown]

On Wed, 23 Sep 2026, Namjae Jeon wrote:
> On Sat, Sep 19, 2026 at 11:25 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > If atomic_open is given __O_REGULAR, we now would prefer -EISDIR
> > if a directory was found.  So move the S_ISDIR() tests earlier.
> >
> > Also don't return -EFTYPE for S_ISLNK() - that should get -ELOOP and only
> > if O_NOFOLLOW.
> Simply exempting symlinks from the __O_REGULAR check in cifs is not
> sufficient. Once this check allows the symlink through,
> cifs_atomic_open() still calls finish_open() on it, setting
> FMODE_OPENED. open_last_lookups() then skips symlink traversal, so
> do_open() returns -EFTYPE before may_open() can return -ELOOP. This
> path also reaches cifs_new_fileinfo(), which pins the symlink dentry
> with dget(). The symlink's file operations do not provide CIFS's
> .release handler, so the failed open does not call cifs_close() to
> release that reference.
> 
> 

Thanks for looking at this.  I think I can see what you mean.

If __cifs_do_open() chooses not to call cifs_posix_open(), if that call
succeeds without providing an inode, then we could end up at
cifs_get_inode_info_unix() or cifs_get_inode_info() and if that returns
a symlink inode, this will be returned to cifs_atomic_open() which will
try to open it (finish_open()).
This is a pre-existing problem - is that right?  If an O_CREAT open
which doesn't have __O_REGULAR happens to find a symlink, then it will
already try to open it.

Presumably after calling cifs_do_create(), cifs_atomic_open() should
check if the inode is something that it can open.  If not it should call
finish_no_open().  Does it need to call server->ops->close() too?
Can I leave you to clean that up?  My goal here wasn't to fix
everything, but to improve the documentation and change some the details
of what errors are created by __O_REGULAR.

Thanks,
NeilBrown

Re: [PATCH v2 05/14] cifs: allow -EISDIR precedence over -EFTYPE in __cifs_do_create()

[Namjae Jeon]

On Wed, Sep 23, 2026 at 3:51 PM NeilBrown <neilb@ownmail.net> wrote:
>
> On Wed, 23 Sep 2026, Namjae Jeon wrote:
> > On Sat, Sep 19, 2026 at 11:25 AM NeilBrown <neilb@ownmail.net> wrote:
> > >
> > > From: NeilBrown <neil@brown.name>
> > >
> > > If atomic_open is given __O_REGULAR, we now would prefer -EISDIR
> > > if a directory was found.  So move the S_ISDIR() tests earlier.
> > >
> > > Also don't return -EFTYPE for S_ISLNK() - that should get -ELOOP and only
> > > if O_NOFOLLOW.
> > Simply exempting symlinks from the __O_REGULAR check in cifs is not
> > sufficient. Once this check allows the symlink through,
> > cifs_atomic_open() still calls finish_open() on it, setting
> > FMODE_OPENED. open_last_lookups() then skips symlink traversal, so
> > do_open() returns -EFTYPE before may_open() can return -ELOOP. This
> > path also reaches cifs_new_fileinfo(), which pins the symlink dentry
> > with dget(). The symlink's file operations do not provide CIFS's
> > .release handler, so the failed open does not call cifs_close() to
> > release that reference.
> >
> >
>
> Thanks for looking at this.  I think I can see what you mean.
>
> If __cifs_do_open() chooses not to call cifs_posix_open(), if that call
> succeeds without providing an inode, then we could end up at
> cifs_get_inode_info_unix() or cifs_get_inode_info() and if that returns
> a symlink inode, this will be returned to cifs_atomic_open() which will
> try to open it (finish_open()).
> This is a pre-existing problem - is that right?  If an O_CREAT open
> which doesn't have __O_REGULAR happens to find a symlink, then it will
> already try to open it.
Yes, I agree this is a pre-existing problem.

>
> Presumably after calling cifs_do_create(), cifs_atomic_open() should
> check if the inode is something that it can open.  If not it should call
> finish_no_open().  Does it need to call server->ops->close() too?
> Can I leave you to clean that up?  My goal here wasn't to fix
> everything, but to improve the documentation and change some the details
> of what errors are created by __O_REGULAR.
If the server-side handle is still open, it needs to be closed before
calling finish_no_open().
I’ll take a look at it.
Thanks!

Re: [PATCH v2 05/14] cifs: allow -EISDIR precedence over -EFTYPE in __cifs_do_create()

[Namjae Jeon]

On Sat, Sep 19, 2026 at 11:25 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> If atomic_open is given __O_REGULAR, we now would prefer -EISDIR
> if a directory was found.  So move the S_ISDIR() tests earlier.
>
> Also don't return -EFTYPE for S_ISLNK() - that should get -ELOOP and only
> if O_NOFOLLOW.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Namjae Jeon <linkinjeon@kernel.org>
Thanks!

Re: [PATCH v2 01/14] VFS: revise and expand documentation for atomic_open.

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> atomic_open is a complex operation which different filesystems implement
> quite differently.  The available documentation doesn't give clear
> guidance on how it should be implemented.
> 
> nfsd has a particular need to open only regular files, but to get
> precise information about what was found if it wasn't a regular file.
> This is slightly different to the syscall calling needs.  In particular
> it suggests that __O_REGULAR shouldn't always result in -EFTYPE.
> 
> In any case that does involve creating open state, using
> finish_no_open() is simplest as it reduces the need to check
> __O_REGULAR, O_DIRECTORY, O_NOFOLLOW.
> 
> So refresh the documentation to give guidance on the choice between
> finish_no_open, finish_open, and an error.  Efficiency always wins, but
> when that isn't an issue, prefer finish_no_open().
> 
> Also clarify the required behaviour when __O_REGULAR is given.  This
> should return -EISDIR if a directory is found as nfsd needs this.  If a
> symlink is found then __O_REGULAR does NOT apply: O_NOFOLLOW must be
> used to decided if it is safe to not return the looked-up dentry.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/vfs.rst | 67 ++++++++++++++++++++++++++-----
>  fs/namei.c                        |  3 ++
>  2 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index d3a93eec3945..00ada8cc85ae 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -599,17 +599,62 @@ otherwise noted.
>  
>  ``atomic_open``
>  	called on the last component of an open.  Using this optional
> -	method the filesystem can look up, possibly create and open the
> -	file in one atomic operation.  If it wants to leave actual
> -	opening to the caller (e.g. if the file turned out to be a
> -	symlink, device, or just something filesystem won't do atomic
> -	open for), it may signal this by returning finish_no_open(file,
> -	dentry).  This method is only called if the last component is
> -	negative or needs lookup.  Cached positive dentries are still
> -	handled by f_op->open().  If the file was created, FMODE_CREATED
> -	flag should be set in file->f_mode.  In case of O_EXCL the
> -	method must only succeed if the file didn't exist and hence
> -	FMODE_CREATED shall always be set on success.
> +	method the filesystem can look up, create, truncate, and open
> +	the file in one atomic operation.  This is needed if the
> +	filesystem content can be changed asynchronously and
> +	specifically if a negative dentry is not a guarantee that the
> +	object doesn't exist.  It is also useful if it is possible to
> +	perform combinations of revalidate, lookup, create, open, and
> +	truncate more efficiently what with a sequence of individual
> +	operations.
> +
> +	If the object found is not a file or directory, or if
> +	lookup/create succeeded without establishing any "open" state,
> +	then finish_no_open() should be called to confirm that the
> +	dentry is ready to be handled by normal VFS processing.
> +	FMODE_CREATED should be set in the "file" if the object was
> +	created, and this will prevent further access permission checks,
> +	or handling of O_TRUNC and O_EXCL.
> +
> +	If the lookup/create operation established some open state for a
> +	file or directory, the open should be completed by calling
> +	finish_open().  Passing NULL as the "open" function to
> +	finish_open() is unlikely to be useful as that assumes that no
> +	open state has been established.
> +
> +	atomic_open() may generate errors related to O_DIRECTORY,
> +	__O_REGULAR, O_EXCL, O_NOFOLLOW but is not required to as the
> +	caller will check those against the resulting dentry and
> +	generate any error needed, possibly closing the file if it was
> +	opened by finish_open().  atomic_open() is encouraged to handle
> +	these flags only when doing so is more efficient than not.
> +
> +	If __O_REGULAR is handled, it should generate -EISDIR if the
> +	name is known to be a directory or -EFTYPE if it is some other
> +	non-regular file other than a symbolic link.  Handling of a
> +	symbolic link should be guided by O_NOFOLLOW, not __O_REGULAR:
> +	-ELOOP can be return if O_NOFOLLOW is set, otherwise the symlink
> +	should be returned through finish_no_open().
> +
> +	The focus for atomic_open() is to provide the correct dentry and
> +	to set FMODE_CREATED as accurately as possible.  If O_EXCL was
> +	set, FMODE_CREATED should only be set if this operation
> +	certainly created the object.  If O_EXCL was not set,
> +	FMODE_CREATE should be set if it is possible that this operation
> +	created the object.
> +
> +	This method is only called if the last component is negative or
> +	needs lookup.  Cached positive dentries are still handled by
> +	f_op->open().
> +
> +	If the dentry provided is negative (not in-lookup) and O_CREAT
> +	isn't set, then there is no guarantee of exclusive access to the
> +	dentry - another thread might call ->atomic_open() on the same
> +	dentry at the same time.  If needed a filesystem can ensure this
> +	doesn't happen by returning 0 from ->d_revalidate when that is
> +	called with LOOKUP_OPEN on a negative dentry.  This will ensure
> +	that ->atomic_open() only receives an in-lookup dentry, which
> +	always ensures exclusive access.
>  
>  ``tmpfile``
>  	called in the end of O_TMPFILE open().  Optional, equivalent to
> diff --git a/fs/namei.c b/fs/namei.c
> index d95249dd527c..0f69abb3743b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5007,6 +5007,9 @@ static struct file *path_openat(struct nameidata *nd,
>  		error = -EINVAL;
>  	}
>  	fput_close(file);
> +	if (error == -EISDIR &&
> +	    (op->open_flag & __O_REGULAR))
> +		error = -EFTYPE;
>  	if (error == -EOPENSTALE) {
>  		if (flags & LOOKUP_RCU)
>  			error = -ECHILD;
> 
> base-commit: 9189e6a6f89e32d3a604b221ea64e67e1a35957c

It just keeps growing! But on a more serious note, it's better to have
more clear verbiage here.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 07/14] vfs: O_NONBLOCK|O_CREAT open shouldn't wait for directory delegation

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> 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 a08f37aca3e1..b9fca38ad489 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4554,7 +4554,12 @@ retry:
>  		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;
>  

That seems right. We don't want to block the opener while breaking the
directory lease.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 06/14] vfs: add some allowed open flags to vfs_lookup_open()

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> 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.
> 
> Also O_NOFOLLOW as we don't/can't handle symlinks.  In fact we
> should enforce O_NOFOLLOW for the same reason we enforce __O_REGULAR.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 0f69abb3743b..a08f37aca3e1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4632,11 +4632,12 @@ 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|O_NOFOLLOW),
>  		  "open_flag has unsupported flags");
>  
>  	mode |= S_IFREG;
> -	open_flag |= __O_REGULAR;
> +	open_flag |= __O_REGULAR | O_NOFOLLOW;
>  
>  	error = lookup_noperm_common(last, parent->dentry);
>  	if (error)

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 02/14] nfs: correctly handle NFS4ERR_WRONG_TYPE from v4 OPEN request

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> Now that we have the -EFTYPE error code, we can return it from
> ->open_context when the server returns NFS4ERR_WRONG_TYPE.
> This can be directly returned when __O_REGULAR is in effect,
> or can trigger a lookup and finish_no_open().
> 
> Also don't over-ride the err code when __O_REGULAR is in effect - nfsd
> wants the see the original error, and VFS code will map when needed.
> 
> Finally don't consult __O_REGULAR for -ENOTDIR.  It isn't clear what
> that means and is safest to leave the original handling.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfs/dir.c           | 8 ++++----
>  fs/nfs_common/common.c | 1 +
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 49394123bd09..11bcc922198e 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2191,11 +2191,11 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  			d_splice_alias(NULL, dentry);
>  			break;
>  		case -EISDIR:
> -		case -ENOTDIR:
> -			if (open_flags & __O_REGULAR) {
> -				err = -EFTYPE;
> +		case -EFTYPE:
> +			if (open_flags & __O_REGULAR)
>  				break;
> -			}
> +			goto no_open;
> +		case -ENOTDIR:
>  			goto no_open;
>  		case -ELOOP:
>  			if (!(open_flags & O_NOFOLLOW))
> diff --git a/fs/nfs_common/common.c b/fs/nfs_common/common.c
> index 0778743ae2c2..24add750c8d5 100644
> --- a/fs/nfs_common/common.c
> +++ b/fs/nfs_common/common.c
> @@ -102,6 +102,7 @@ static const struct {
>  	{ NFS4ERR_BADTYPE,	-EBADTYPE	},
>  	{ NFS4ERR_SYMLINK,	-ELOOP		},
>  	{ NFS4ERR_DEADLOCK,	-EDEADLK	},
> +	{ NFS4ERR_WRONG_TYPE,	-EFTYPE		},
>  };
>  
>  static const struct {

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 02/14] nfs: correctly handle NFS4ERR_WRONG_TYPE from v4 OPEN request

[Jeff Layton]

Some agentic review found a potential issue here:

> diff --git a/fs/nfs_common/common.c b/fs/nfs_common/common.c
> index 0778743ae2c2..24add750c8d5 100644
> --- a/fs/nfs_common/common.c
> +++ b/fs/nfs_common/common.c
> @@ -102,6 +102,7 @@ static const struct {
>  	{ NFS4ERR_BADTYPE,	-EBADTYPE	},
>  	{ NFS4ERR_SYMLINK,	-ELOOP		},
>  	{ NFS4ERR_DEADLOCK,	-EDEADLK	},
> +	{ NFS4ERR_WRONG_TYPE,	-EFTYPE		},
>  };

[ ... ]

The new mapping also changes what filelayout_async_handle_error() in
fs/nfs/filelayout/filelayout.c sees for this status. That function
switches on the decoded task status, which mixes mapped errnos with raw
NFSv4 codes:

fs/nfs/filelayout/filelayout.c:filelayout_async_handle_error() {
	...
	case -ESTALE:           /* mapped NFS4ERR_STALE */
	case -EBADHANDLE:       /* mapped NFS4ERR_BADHANDLE */
	case -EISDIR:           /* mapped NFS4ERR_ISDIR */
	case -NFS4ERR_FHEXPIRED:
	case -NFS4ERR_WRONG_TYPE:
		...
		pnfs_destroy_layout(NFS_I(inode));
		rpc_wake_up(&tbl->slot_tbl_waitq);
		goto reset;
	...
	default:
reset:
		return -NFS4ERR_RESET_TO_MDS;
}

Before this commit a data server returning WRONG_TYPE arrived as
-NFS4ERR_WRONG_TYPE and took the layout-invalidating path above. With
the new mapping it arrives as -EFTYPE, which matches no case and falls
into default, so the I/O is still retried through the MDS but the
layout is no longer destroyed and the slot waitqueue is not woken.

Should the "Invalidate Layout errors" group gain a case for -EFTYPE
alongside the mapped -EISDIR case, the way the other mapped codes are
handled there?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 04/14] ceph: simplify atomic_open to use finish_no_open()

[Jeff Layton]

Another LLM review:

> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index bd3e3f5c269e..83ceb2906f85 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c

[ ... ]

> @@ -977,33 +976,25 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
>  	}
>  	if (err)
>  		goto out_req;
> -	if (dn || d_really_is_negative(dentry) || d_is_symlink(dentry)) {
> -		/* make vfs retry on splice, ENOENT, or symlink */
> -		doutc(cl, "finish_no_open on dn %p\n", dn);
> -		err = finish_no_open(file, dn);
> -	} else {
> -		if (IS_ENCRYPTED(dir) &&
> -		    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> -			pr_warn_client(cl,
> -					"Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> -					ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> -			goto out_req;
> -		}
> +	if (IS_ENCRYPTED(dir) &&
> +	    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> +		pr_warn_client(cl,
> +			       "Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> +			       ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> +		goto out_req;
> +	}

The fscrypt check used to run only when the dentry was positive, not
a symlink, and had no alias. Now it runs unconditionally, but
d_inode() can still be NULL here: a lookup that found nothing leaves
a negative dentry with err == 0, and when the MDS reply splices an
alias the original dentry is never instantiated.

fscrypt_has_permitted_context() dereferences its child argument
without checking for NULL:

fs/crypto/policy.c:fscrypt_has_permitted_context() {
	...
	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
	    !S_ISLNK(child->i_mode))
		return 1;
	...
}

Every other in-tree caller passes a known inode, while this one can
pass NULL. An open of a name that does not exist in an encrypted
directory reaches this through lookup_open() -> atomic_open() ->
ceph_atomic_open().

Could the fscrypt check stay behind the positive-dentry test, as
before?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 11/14] nfsd: check for mountpoints after non-creating open.

[Jeff Layton]

LLM review:

> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 6e443503d0b7..3d37754f787b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c

[ ... ]

> @@ -466,6 +454,14 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			status = nfserr_exist;
>  			goto out;
>  		}
> +		/* We opened an existing file, it might be a junction. */
> +		if (unlikely(nfsd_mountpoint(child, fhp->fh_export) == 1)) {
> +			dget(child);
> +			nfsd_filp_close(open->op_filp);
> +			open->op_filp = NULL;
> +			goto do_cross_mnt;
> +		}
> +

[ ... ]

> @@ -511,6 +507,21 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  out:
>  	nfsd_attrs_free(&attrs);
>  	return status;
> +
> +do_cross_mnt:
> +	exp = exp_get(fhp->fh_export);
> +
> +	status = nfsd_cross_mnt(rqstp, &child, &exp);
> +	if (status == nfs_ok)
> +		status = fh_compose(resfhp, exp,
> +				    child, fhp);

When the new junction check triggers, resfhp was already composed
earlier in nfsd4_create_file(), so the fh_compose() in do_cross_mnt
runs a second time on the same handle:

fs/nfsd/nfsfh.c:fh_compose() {
	...
	if (fhp->fh_dentry) {
		printk(KERN_ERR "fh_compose: fh %pd2 not initialized!\n",
		       dentry);
	}
	...
	fhp->fh_dentry = dget(dentry); /* our internal copy */
	fhp->fh_export = exp_get(exp);
	...
}

The first composed dentry and export references are overwritten
without being released. Should resfhp be released before crossing,
or the mountpoint check done before the first compose?

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 08/14] vfs: don't return -ENODEV from vfs_lookup_open()

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> -ENODEV is not generally an error meaning "the object is a device"
> and nfsd - the only intended caller of vfs_lookup_open() - does not
> benefit from knowing it was a device file.  So return -EFTYPE in
> that case.
> Also switch to testing the dentry type rather than dereferencing the
> inode to get the type.
> 
> -EISDIR is widely used to mean "the object is a directory which is
>    not what is wanted".
> -ELOOP is sometimes used elsewhere to mean "a symlink was found but
>    cannot be handled".
> 
> Also remove note about ->atomic_open returning -EFTYPE as that now only
> happens for non regular/dir/symlink.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index b9fca38ad489..ba3e7e4b5fdb 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4621,9 +4621,7 @@ out_dput:
>   * 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.
> + * -EFTYPE : any other non-regular file was found, device-special, FIFO or SOCK
>   *
>   * Returns: the opened struct file, or an error.
>   */
> @@ -4671,24 +4669,12 @@ 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:
> +	} else if (!d_is_reg(dentry)) {
> +		error = -EFTYPE;
> +		if (d_is_dir(dentry))
>  			error = -EISDIR;
> -			break;
> -		case S_IFLNK:
> +		if (d_is_symlink(dentry))
>  			error = -ELOOP;
> -			break;
> -		case S_IFBLK:
> -		case S_IFCHR:
> -			error = -ENODEV;
> -			break;
> -		case S_IFIFO:
> -		case S_IFSOCK:
> -		default:
> -			error = -EFTYPE;
> -			break;
> -		}
>  	} else if (!(file->f_mode & FMODE_OPENED)) {
>  		nd.path.dentry = dentry;
>  		error = vfs_open(&nd.path, file);

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 09/14] vfs: change vfs_lookup_open() to use do_open(), not vfs_open()

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> The vfs_open() call misses permission checks which should happen before
> an open is attempted (atomic_open does separate permission checks).
> 
> Much of the code in do_open() will have no effect as relevant LOOKUP_
> flags aren't set.  truncation will be done (which nfsd is not expected
> to use) along with some audit logs and security hook.  The important
> benefit is getting the may_open() check.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index ba3e7e4b5fdb..8ef2d44b6108 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4604,6 +4604,9 @@ out_dput:
>  	goto out;
>  }
>  
> +static int do_open(struct nameidata *nd,
> +		   struct file *file, const struct open_flags *op);
> +
>  /**
>   * vfs_lookup_open - open and possibly create a regular file
>   * @parent: directory to contain file
> @@ -4660,6 +4663,9 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  	}
>  	op.open_flag = open_flag;
>  	op.mode = mode;
> +	op.acc_mode = ACC_MODE(open_flag);
> +	if (open_flag & O_TRUNC)
> +		op.acc_mode |= MAY_WRITE;
>  	dentry = lookup_open(&nd, file, &op);
>  
>  	if (IS_ERR(dentry))
> @@ -4677,7 +4683,7 @@ struct file *vfs_lookup_open(struct path *parent, struct qstr *last,
>  			error = -ELOOP;
>  	} else if (!(file->f_mode & FMODE_OPENED)) {
>  		nd.path.dentry = dentry;
> -		error = vfs_open(&nd.path, file);
> +		error = do_open(&nd, file, &op);
>  	}
>  	dput(dentry);
>  

I think the code looks right, but the vfs_open -> do_open change is
largely unmentioned in the changelog. It'd be good to flesh that out
some.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 10/14] nfsd: make EEXIST checks in nfsd4_create_file() more consistent.

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_create_file() has two places that check if an "eexists" style
> error is needed - only if create_mode is not NFS4_CREATE_UNCHECKED.
> 
> One if when checking the error code from do_lookup_open(), one when
> checking if ->op_created wasn't set.
> 
> These are not consistent - one tests if op_createmode IS
> NFS4_CREATE_UNCHECKED, the other tests if it isn't.
> 
> Rearrange the second piece of code so that the tests look similar.
> This will make some following patches a bit cleaner.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 3a82af381a8d..6e443503d0b7 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -462,23 +462,24 @@ 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) {
> -			/* NFSv4 protocol requires change attributes
> -			 * even though no change happened.
> -			 */
> -			fh_fill_post_noop(fhp);
> -
> -			/*
> -			 * In NFSv4, we don't want to truncate the file
> -			 * now. This would be wrong if the OPEN fails for
> -			 * some other reason. Furthermore, if the size is
> -			 * nonzero, we should ignore it according to spec!
> -			 */
> -			open->op_truncate = (d_is_reg(child) &&
> -					     (iap->ia_valid & ATTR_SIZE) &&
> -					     !iap->ia_size);
> -		} else
> +		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
>  			status = nfserr_exist;
> +			goto out;
> +		}
> +		/* NFSv4 protocol requires change attributes
> +		 * even though no change happened.
> +		 */
> +		fh_fill_post_noop(fhp);
> +
> +		/*
> +		 * In NFSv4, we don't want to truncate the file
> +		 * now. This would be wrong if the OPEN fails for
> +		 * some other reason. Furthermore, if the size is
> +		 * nonzero, we should ignore it according to spec!
> +		 */

Some mention of where in the spec that's written would be helpful for
posterity. I think it's RFC 8881 § 18.16.3.

> +		open->op_truncate = (d_is_reg(child) &&
> +				     (iap->ia_valid & ATTR_SIZE) &&
> +				     !iap->ia_size);
>  		goto out;
>  	}
>  	/* file was created */

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 13/14] nfsd: change nfsd_check_obj_isreg() to use nfs error codes.

[Jeff Layton]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> 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.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 50baf125d5f9..40bd1179bf60 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -223,17 +223,15 @@ 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 = d_inode(child)->i_mode;
> -
> -	if (S_ISREG(mode))
> +	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)
> @@ -565,7 +563,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;
> 
> 

Nice cleanup.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 01/14] fixes for vfs_lookup_open, and integration with nfsd

[Christian Brauner]

On Sat, Sep 19, 2026 at 12:06:04PM +1000, NeilBrown wrote:
> Greetings.
> 
>  NFSv4 has an "OPEN" request which combines lookup and create and
>  truncate and permission checks etc much like the open() syscall.  When
>  nfsd implements this, I want it to share a much code as (reasonably)
>  possible with the open() paths - in particular I want it to use lookup_open() 
>  and so that it uses ->atomic_open() the same way that other code does.
>  (Longer term I want to make some locking changes and having all the code
>  central makes that easier.)
>  vfs_lookup_open() is a step towards that  but it isn't quite ready yet.
>  This series aims to make it ready, then use it.
> 
>  A particular issue is that using __O_REGULAR exactly meets the needs of
>  nfsd (it doesn't want to open anything else) but the errors returned by
>  __O_REGULAR aren't what nfsd needs.  nfsd needs to know if it was a
>  directory, or a symlink, or something else.
> 
>  I don't think __O_REGULAR should cause symlinks to result in -EFTYPE.
>  If a symlink is found, then it should be followed.  That is what
>  happens with filesystems that don't support ->atomic_open, but some
>  ->atomic_open handlers return -EFTYPE for symlinks when __O_REGULAR is
>  present.  I think that O_NOFOLLOW can affect how symlink are handled,
>  but where possible it is best to just return the symlink to the caller
>  and let it figure out what to do.
> 
>  For directories, nfsd wants EISDIR rather than EFTYPE.  It may be that
>  user-space could benefit from seeing EISDIR too, but that is a separate
>  issue.  So I want ->atomic_open to return EISDIR (if it returns an
>  error at all) rather than EFTYPE if a directory is found.  VFS code can
>  then map that to EFTYPE if needed.
> 
>  So the first few patches in this series improve the documentation for
>  atomic_open and then make changes to nfs, gfs2, ceph, cifs to better
>  match this documentation.  I would appreciate an Ack-by (or whatever
>  else might be appropriate) from fs maintainers for those.
> 
>  Subsequent patches make changes to vfs_lookup_open() and then to nfsd
>  to use it.
> 
>  A significant change here is that opening a file with
>  O_NONBLOCK|O_CREAT will result in -EWOULDBLOCK if a delegation
>  exists on the parent directory - currently it blocks.
>  Jeff - could you comment on that change (07/14)?
> 
>  There are quite a lot of changes here since my previous post,
>  particularly the changes to various filesystems.
>  I've stopped trying to return the dentry from vfs_lookup_open()
>  in the error case - no-one liked that.
> 
>  Thanks for your time,

Neil, I'll take the vfs specific fixes onto vfs-7.4.lookup and then nfs
can pull that all in?

Re: [PATCH v2 01/14] fixes for vfs_lookup_open, and integration with nfsd

[Chuck Lever]

On Fri, Sep 25, 2026, at 12:04 PM, Christian Brauner wrote:
> On Sat, Sep 19, 2026 at 12:06:04PM +1000, NeilBrown wrote:
>> Greetings.
>> 
>>  NFSv4 has an "OPEN" request which combines lookup and create and
>>  truncate and permission checks etc much like the open() syscall.  When
>>  nfsd implements this, I want it to share a much code as (reasonably)
>>  possible with the open() paths - in particular I want it to use lookup_open() 
>>  and so that it uses ->atomic_open() the same way that other code does.
>>  (Longer term I want to make some locking changes and having all the code
>>  central makes that easier.)
>>  vfs_lookup_open() is a step towards that  but it isn't quite ready yet.
>>  This series aims to make it ready, then use it.
>> 
>>  A particular issue is that using __O_REGULAR exactly meets the needs of
>>  nfsd (it doesn't want to open anything else) but the errors returned by
>>  __O_REGULAR aren't what nfsd needs.  nfsd needs to know if it was a
>>  directory, or a symlink, or something else.
>> 
>>  I don't think __O_REGULAR should cause symlinks to result in -EFTYPE.
>>  If a symlink is found, then it should be followed.  That is what
>>  happens with filesystems that don't support ->atomic_open, but some
>>  ->atomic_open handlers return -EFTYPE for symlinks when __O_REGULAR is
>>  present.  I think that O_NOFOLLOW can affect how symlink are handled,
>>  but where possible it is best to just return the symlink to the caller
>>  and let it figure out what to do.
>> 
>>  For directories, nfsd wants EISDIR rather than EFTYPE.  It may be that
>>  user-space could benefit from seeing EISDIR too, but that is a separate
>>  issue.  So I want ->atomic_open to return EISDIR (if it returns an
>>  error at all) rather than EFTYPE if a directory is found.  VFS code can
>>  then map that to EFTYPE if needed.
>> 
>>  So the first few patches in this series improve the documentation for
>>  atomic_open and then make changes to nfs, gfs2, ceph, cifs to better
>>  match this documentation.  I would appreciate an Ack-by (or whatever
>>  else might be appropriate) from fs maintainers for those.
>> 
>>  Subsequent patches make changes to vfs_lookup_open() and then to nfsd
>>  to use it.
>> 
>>  A significant change here is that opening a file with
>>  O_NONBLOCK|O_CREAT will result in -EWOULDBLOCK if a delegation
>>  exists on the parent directory - currently it blocks.
>>  Jeff - could you comment on that change (07/14)?
>> 
>>  There are quite a lot of changes here since my previous post,
>>  particularly the changes to various filesystems.
>>  I've stopped trying to return the dentry from vfs_lookup_open()
>>  in the error case - no-one liked that.
>> 
>>  Thanks for your time,
>
> Neil, I'll take the vfs specific fixes onto vfs-7.4.lookup and then nfs
> can pull that all in?

There are outstanding review comments on the NFSD patches in this
series, but once those are resolved I can merge vfs-7.4.lookup
into nfsd-next and apply Neil's work.

Let me know when I can merge your topic branch.


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

Re: [PATCH v2 04/14] ceph: simplify atomic_open to use finish_no_open()

[NeilBrown]

On Thu, 24 Sep 2026, Jeff Layton wrote:
> Another LLM review:
> 
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index bd3e3f5c269e..83ceb2906f85 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> 
> [ ... ]
> 
> > @@ -977,33 +976,25 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> >  	}
> >  	if (err)
> >  		goto out_req;
> > -	if (dn || d_really_is_negative(dentry) || d_is_symlink(dentry)) {
> > -		/* make vfs retry on splice, ENOENT, or symlink */
> > -		doutc(cl, "finish_no_open on dn %p\n", dn);
> > -		err = finish_no_open(file, dn);
> > -	} else {
> > -		if (IS_ENCRYPTED(dir) &&
> > -		    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> > -			pr_warn_client(cl,
> > -					"Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> > -					ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> > -			goto out_req;
> > -		}
> > +	if (IS_ENCRYPTED(dir) &&
> > +	    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> > +		pr_warn_client(cl,
> > +			       "Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> > +			       ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> > +		goto out_req;
> > +	}
> 
> The fscrypt check used to run only when the dentry was positive, not
> a symlink, and had no alias. Now it runs unconditionally, but
> d_inode() can still be NULL here: a lookup that found nothing leaves
> a negative dentry with err == 0, and when the MDS reply splices an
> alias the original dentry is never instantiated.
> 
> fscrypt_has_permitted_context() dereferences its child argument
> without checking for NULL:
> 
> fs/crypto/policy.c:fscrypt_has_permitted_context() {
> 	...
> 	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
> 	    !S_ISLNK(child->i_mode))
> 		return 1;
> 	...
> }
> 
> Every other in-tree caller passes a known inode, while this one can
> pass NULL. An open of a name that does not exist in an encrypted
> directory reaches this through lookup_open() -> atomic_open() ->
> ceph_atomic_open().
> 
> Could the fscrypt check stay behind the positive-dentry test, as
> before?

Alternately: can we simply remove the fscrypt check?  I don't know a
whole lot about fscrypt, but it seems that ceph_open() does all
necessary fscrypt checks, and we already didn't need this one before
calling ceph_open().

You added this in
  Commit: 94af0470924c ("ceph: add some fscrypt guardrails")
but the commit doesn't explain why ceph_atomic_open() needed more that
ceph_open() needed.

Thanks,
NeilBrown

Re: [PATCH v2 10/14] nfsd: make EEXIST checks in nfsd4_create_file() more consistent.

[NeilBrown]

On Thu, 24 Sep 2026, Jeff Layton wrote:
> On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> > 
> > nfsd4_create_file() has two places that check if an "eexists" style
> > error is needed - only if create_mode is not NFS4_CREATE_UNCHECKED.
> > 
> > One if when checking the error code from do_lookup_open(), one when
> > checking if ->op_created wasn't set.
> > 
> > These are not consistent - one tests if op_createmode IS
> > NFS4_CREATE_UNCHECKED, the other tests if it isn't.
> > 
> > Rearrange the second piece of code so that the tests look similar.
> > This will make some following patches a bit cleaner.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/nfsd/nfs4proc.c | 33 +++++++++++++++++----------------
> >  1 file changed, 17 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 3a82af381a8d..6e443503d0b7 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -462,23 +462,24 @@ 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) {
> > -			/* NFSv4 protocol requires change attributes
> > -			 * even though no change happened.
> > -			 */
> > -			fh_fill_post_noop(fhp);
> > -
> > -			/*
> > -			 * In NFSv4, we don't want to truncate the file
> > -			 * now. This would be wrong if the OPEN fails for
> > -			 * some other reason. Furthermore, if the size is
> > -			 * nonzero, we should ignore it according to spec!
> > -			 */
> > -			open->op_truncate = (d_is_reg(child) &&
> > -					     (iap->ia_valid & ATTR_SIZE) &&
> > -					     !iap->ia_size);
> > -		} else
> > +		if (open->op_createmode != NFS4_CREATE_UNCHECKED) {
> >  			status = nfserr_exist;
> > +			goto out;
> > +		}
> > +		/* NFSv4 protocol requires change attributes
> > +		 * even though no change happened.
> > +		 */
> > +		fh_fill_post_noop(fhp);
> > +
> > +		/*
> > +		 * In NFSv4, we don't want to truncate the file
> > +		 * now. This would be wrong if the OPEN fails for
> > +		 * some other reason. Furthermore, if the size is
> > +		 * nonzero, we should ignore it according to spec!
> > +		 */
> 
> Some mention of where in the spec that's written would be helpful for
> posterity. I think it's RFC 8881 § 18.16.3.
> 

I've added

		 * RFC 8881 § 18.16.3. says:
		 *   When an UNCHECKED4 create encounters an existing
		 *   file, the attributes specified by createattrs are
		 *   not used, except that when createattrs specifies
		 *   the size attribute with a size of zero, the
		 *   existing file is truncated.


> > +		open->op_truncate = (d_is_reg(child) &&
> > +				     (iap->ia_valid & ATTR_SIZE) &&
> > +				     !iap->ia_size);
> >  		goto out;
> >  	}
> >  	/* file was created */
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks,
NeilBrown


> 

Re: [PATCH v2 11/14] nfsd: check for mountpoints after non-creating open.

[NeilBrown]

On Thu, 24 Sep 2026, Jeff Layton wrote:
> LLM review:
> 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 6e443503d0b7..3d37754f787b 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> 
> [ ... ]
> 
> > @@ -466,6 +454,14 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  			status = nfserr_exist;
> >  			goto out;
> >  		}
> > +		/* We opened an existing file, it might be a junction. */
> > +		if (unlikely(nfsd_mountpoint(child, fhp->fh_export) == 1)) {
> > +			dget(child);
> > +			nfsd_filp_close(open->op_filp);
> > +			open->op_filp = NULL;
> > +			goto do_cross_mnt;
> > +		}
> > +
> 
> [ ... ]
> 
> > @@ -511,6 +507,21 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  out:
> >  	nfsd_attrs_free(&attrs);
> >  	return status;
> > +
> > +do_cross_mnt:
> > +	exp = exp_get(fhp->fh_export);
> > +
> > +	status = nfsd_cross_mnt(rqstp, &child, &exp);
> > +	if (status == nfs_ok)
> > +		status = fh_compose(resfhp, exp,
> > +				    child, fhp);
> 
> When the new junction check triggers, resfhp was already composed
> earlier in nfsd4_create_file(), so the fh_compose() in do_cross_mnt
> runs a second time on the same handle:
> 
> fs/nfsd/nfsfh.c:fh_compose() {
> 	...
> 	if (fhp->fh_dentry) {
> 		printk(KERN_ERR "fh_compose: fh %pd2 not initialized!\n",
> 		       dentry);
> 	}
> 	...
> 	fhp->fh_dentry = dget(dentry); /* our internal copy */
> 	fhp->fh_export = exp_get(exp);
> 	...
> }
> 
> The first composed dentry and export references are overwritten
> without being released. Should resfhp be released before crossing,
> or the mountpoint check done before the first compose?

Is LLM making me lazy?  Or was I already lazy?

I think the fh_compose() should be moved after the mountpoint check.

Thanks,
NeilBrown

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

[NeilBrown]

On Mon, 21 Sep 2026, Chuck Lever wrote:
> 
> > +	/* FIXME: check session persistence and pnfs flags.
> > +	 * The nfsv4.1 spec requires the following semantics:
> > +	 *
> > +	 * Persistent   | pNFS   | Server REQUIRED | Client Allowed
> > +	 * Reply Cache  | server |                 |
> 
> This table is only about create modes. Out of the op_create block it
> now reads as though it applies to every OPEN. Could it say "for
> creating OPENs", or move into the op_create arm of nfsd4_open_file()?

I've added that - but I wonder if this table serves any purpose.
We don't support session persistence at all, and we endeavour to support
all the create modes, so what is there to "FIX" ??

The comment was added in 2009 by 
Commit: 79fb54abd285 ("nfsd41: CREATE_EXCLUSIVE4_1")

Any objecting to me removing it as uninteresting?

Thanks,
NeilBrown

Re: [PATCH v2 04/14] ceph: simplify atomic_open to use finish_no_open()

[Jeff Layton]

On Sat, 2026-09-26 at 07:49 +1000, NeilBrown wrote:
> On Thu, 24 Sep 2026, Jeff Layton wrote:
> > Another LLM review:
> > 
> > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > > index bd3e3f5c269e..83ceb2906f85 100644
> > > --- a/fs/ceph/file.c
> > > +++ b/fs/ceph/file.c
> > 
> > [ ... ]
> > 
> > > @@ -977,33 +976,25 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> > >  	}
> > >  	if (err)
> > >  		goto out_req;
> > > -	if (dn || d_really_is_negative(dentry) || d_is_symlink(dentry)) {
> > > -		/* make vfs retry on splice, ENOENT, or symlink */
> > > -		doutc(cl, "finish_no_open on dn %p\n", dn);
> > > -		err = finish_no_open(file, dn);
> > > -	} else {
> > > -		if (IS_ENCRYPTED(dir) &&
> > > -		    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> > > -			pr_warn_client(cl,
> > > -					"Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> > > -					ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> > > -			goto out_req;
> > > -		}
> > > +	if (IS_ENCRYPTED(dir) &&
> > > +	    !fscrypt_has_permitted_context(dir, d_inode(dentry))) {
> > > +		pr_warn_client(cl,
> > > +			       "Inconsistent encryption context (parent %llx:%llx child %llx:%llx)\n",
> > > +			       ceph_vinop(dir), ceph_vinop(d_inode(dentry)));
> > > +		goto out_req;
> > > +	}
> > 
> > The fscrypt check used to run only when the dentry was positive, not
> > a symlink, and had no alias. Now it runs unconditionally, but
> > d_inode() can still be NULL here: a lookup that found nothing leaves
> > a negative dentry with err == 0, and when the MDS reply splices an
> > alias the original dentry is never instantiated.
> > 
> > fscrypt_has_permitted_context() dereferences its child argument
> > without checking for NULL:
> > 
> > fs/crypto/policy.c:fscrypt_has_permitted_context() {
> > 	...
> > 	if (!S_ISREG(child->i_mode) && !S_ISDIR(child->i_mode) &&
> > 	    !S_ISLNK(child->i_mode))
> > 		return 1;
> > 	...
> > }
> > 
> > Every other in-tree caller passes a known inode, while this one can
> > pass NULL. An open of a name that does not exist in an encrypted
> > directory reaches this through lookup_open() -> atomic_open() ->
> > ceph_atomic_open().
> > 
> > Could the fscrypt check stay behind the positive-dentry test, as
> > before?
> 
> Alternately: can we simply remove the fscrypt check?  I don't know a
> whole lot about fscrypt, but it seems that ceph_open() does all
> necessary fscrypt checks, and we already didn't need this one before
> calling ceph_open().
> 
> You added this in
>   Commit: 94af0470924c ("ceph: add some fscrypt guardrails")
> but the commit doesn't explain why ceph_atomic_open() needed more that
> ceph_open() needed.
> 

Yes, let's just drop it. I had an LLM verify that too, and here's it's
rationales:

Three reasons to remove rather than re-nest it:

1) For regular files it's redundant. finish_open() calls ceph_open(),
   which calls fscrypt_file_open(). That does the same
   fscrypt_has_permitted_context() against the parent (d_parent's inode
   is dir on this path), plus fscrypt_require_key(), which the
   open-coded version doesn't.

2) It's broken as written. err is 0 at that point, so tripping it
   returns 0 from ->atomic_open with FMODE_OPENED clear and
   f_path.dentry still DENTRY_NOT_SET. That hits the WARN_ON() in
   atomic_open() in fs/namei.c and gives back -EIO instead of -EPERM.
   Clearly it has never fired in testing.

3) The only thing it covers that ceph_open() doesn't is a directory
   child (symlinks and spliced/negative dentries all go to
   finish_no_open). But that coverup()
   has no context check at all, unlike ext4/f2fs/ubifs which check
   S_ISDIR||S_ISLNK children in -> via
   the normal lookup path -- including everything finish_no_open()
   bounces back to the VFS -- are already unchecked.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 06/14] vfs: add some allowed open flags to vfs_lookup_open()

[Jori Koolstra]

> Op 19-09-2026 04:06 CEST schreef NeilBrown <neilb@ownmail.net>:
> 
>  
> 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.
> 
> Also O_NOFOLLOW as we don't/can't handle symlinks.  In fact we
> should enforce O_NOFOLLOW for the same reason we enforce __O_REGULAR.

I don't know all the NFS background, but: you can handle intermediate symlinks
but not trailing ones? Why is that?

> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/namei.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 0f69abb3743b..a08f37aca3e1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4632,11 +4632,12 @@ 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|O_NOFOLLOW),
>  		  "open_flag has unsupported flags");
>  
>  	mode |= S_IFREG;
> -	open_flag |= __O_REGULAR;
> +	open_flag |= __O_REGULAR | O_NOFOLLOW;

What does __O_REGULAR | O_NOFOLLOW do? Does __O_REGULAR only block opening
if the final resolved thing is not a regular file, or does is also block
trailing symlinks? Afaict, the former.

>  
>  	error = lookup_noperm_common(last, parent->dentry);
>  	if (error)
> -- 
> 2.50.0.107.gf914562f5916.dirty

Re: [PATCH v2 06/14] vfs: add some allowed open flags to vfs_lookup_open()

[NeilBrown]

On Wed, 30 Sep 2026, Jori Koolstra wrote:
> > Op 19-09-2026 04:06 CEST schreef NeilBrown <neilb@ownmail.net>:
> > 
> >  
> > 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.
> > 
> > Also O_NOFOLLOW as we don't/can't handle symlinks.  In fact we
> > should enforce O_NOFOLLOW for the same reason we enforce __O_REGULAR.
> 
> I don't know all the NFS background, but: you can handle intermediate symlinks
> but not trailing ones? Why is that?

NFS performs a lookup one component at a time.  Each component might be
a symlink and the client decides what to do it if is.
When opening a file where the client hasn't previously performed a
lookup of the basename, it can use the NFS OPEN request which combines
lookup and open (and create), much like how ->atomic_open does.
So the OPEN NFS request only needs to care about trailing symlinks as
those are the only ones that it has any chance of seeing.

> 
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> >  fs/namei.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 0f69abb3743b..a08f37aca3e1 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4632,11 +4632,12 @@ 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|O_NOFOLLOW),
> >  		  "open_flag has unsupported flags");
> >  
> >  	mode |= S_IFREG;
> > -	open_flag |= __O_REGULAR;
> > +	open_flag |= __O_REGULAR | O_NOFOLLOW;
> 
> What does __O_REGULAR | O_NOFOLLOW do? Does __O_REGULAR only block opening
> if the final resolved thing is not a regular file, or does is also block
> trailing symlinks? Afaict, the former.

__O_REGULAR is not documented it detail, but I understand that it
prevents the filesystem from opening anything that isn't a regular file.
As symlinks can never be opened anyway (O_PATH is not really an "open"),
__O_REGULAR should have no effect if a symlink is found.  But O_NOFOLLOW
might have an effect.  (If the symlink is followed and ultimately points
to a non-regular file, then __O_REGULAR will have an effect).

For ->atomic_open, if a symlink is found, then the dentry should be
instantiated to that symlink and it should be provided via
finish_no_open().
If, however, O_NOFOLLOW is set, then if a symlink is found then it is
perfectly acceptable to return -ELOOP instead.

This makes a difference for the NFS client.  If it sends an OPEN request
to the server and gets the error NFS4ERR_SYMLINK, then it will normally
send a LOOKUP to the server to get the details of the symlink.  However
if O_NOFOLLOW is set it can avoid the LOOKUP and simply return -ELOOP.


NeilBrown

> 
> >  
> >  	error = lookup_noperm_common(last, parent->dentry);
> >  	if (error)
> > -- 
> > 2.50.0.107.gf914562f5916.dirty
>