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

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

Fixed.

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

Fixed.

> 
> > The non-create path now doesn't use nfsd_lookup().  As mount-point
> > crossing including nfsd_check_access() is already included for existing
> > names, this does not lose us anything.
> 
> The function is check_nfsd_access(), reached through nfsd_cross_mnt().
> 
> Is "already included for existing names" accurate? The only mount-point
> crossing left in nfsd4_open_file() is the try_lookup_noperm() block,
> and that consults only the dcache. More on that below.

It is now!

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

Thanks.

> 
>  * Implement NFSv4 OPEN semantics for regular files, both non-creating
>  * and creating (unchecked, guarded, and exclusive). Open state for the
>  * file is subsequently fabricated in nfsd4_process_open2().
>  *
>  * Upon return, caller must release @fhp and @resfhp.
> 
> 
> > @@ -285,6 +286,10 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (status != nfs_ok)
> >  		return status;
> >
> > +	status = fh_fill_pre_attrs_unlocked(fhp);
> > +	if (status)
> > +		return status;
> > +
> >  	if (open->op_createmode == NFS4_CREATE_UNCHECKED) {
> >  		/*
> >  		 * If name is already in dcache we need to check for mountpoints
> 
> First, the test reads open->op_createmode, which the commit message says
> is undefined when op_create is zero. It works because
> nfsd4_decode_open() zeroes the whole struct, so a non-creating OPEN
> sees NFS4_CREATE_UNCHECKED here. Could the op_create test that sets
> createmode be hoisted above this block, and the condition become

oops. Fixed.

> 
> 	if (!open->op_create || createmode == NFS4_CREATE_UNCHECKED) {
> 
> so the non-create path does not depend on that zero?
> 
> Second, nfsd_lookup_dentry() did a real lookup and then called
> nfsd_cross_mnt() whenever nfsd_mountpoint() was true. try_lookup_noperm()
> returns NULL when the name is not cached. In that case vfs_lookup_open()
> opens whatever it finds on the parent's mount and fh_compose() uses the
> parent's export.
> 
> For a referral junction (a regular file carrying the junction xattr),
> nfsd_cross_mnt() would have swapped in the referral export. On a cold
> dcache the client instead gets an ordinary filehandle for the junction
> file, and a second OPEN of the same name, now cached, takes the
> crossing path. The same applies to a bind-mounted file whose mount
> target has its own export.
> 
> The Linux client sends OPEN by name without a preceding LOOKUP, so the
> first access after a server reboot reaches this. Could the block do a
> real lookup of the name, as nfsd_lookup_dentry() does, before deciding
> whether to cross?

I don't think we want a real lookup.  I think we want to stick with the
the vfs_open_lookup() call, but if it says it didn't create the file,
then we need to check for a mountpoint.  I think this might be a
long-standing buglet.  I've addressed for my next posting.

> 
> 
> > @@ -373,11 +382,11 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  		iap->ia_atime.tv_nsec = 0;
> >  	}
> >
> > -	create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> > -	if (create_status)
> > -		/* Might still succeed if no create is needed */
> > -		oflags &= ~O_CREAT;
> > -
> > +	if (oflags & O_CREAT) {
> 
> Nit: O_CREAT is set only from open->op_create a few lines up. Would
> testing open->op_create here read more directly?

As the guarded code clears O_CREAT if there is an error, I think it
works well to test for O_CREAT.  So I'd prefer to leave this one as-is.

> 
> 
> > +		create_status = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE);
> > +		if (create_status)
> > +			oflags &= ~O_CREAT;
> > +	}
> >  	dget(parent.dentry);
> >  	open->op_filp = vfs_lookup_open(&parent,
> >  					&QSTR_LEN(open->op_fname,
> 
> This changes when a delegation is recalled relative to the access
> check. oflags carries O_WRONLY or O_RDWR from op_share_access, and
> vfs_lookup_open() opens the child with those flags. do_dentry_open()
> does no permission check of its own and calls break_lease() on the
> inode, so a conflicting delegation held by another client is recalled
> here. Only afterwards does do_open_lookup() call do_open_permission()
> and return NFS4ERR_ACCESS.
> 
> Before this patch a non-creating OPEN went through nfsd_lookup() and
> do_open_permission() first, and the open that breaks the lease
> happened later in nfsd_file_do_acquire(), where fh_verify() precedes
> nfsd_open_break_lease(). So a client that can search the directory
> but has no write permission on the file can now recall other clients'
> write delegations on it by sending OPEN with SHARE_ACCESS_WRITE.
> 
> The create path has had this ordering since nfsd4_create_file() started
> opening the file itself, but it was limited to opens that pass
> NFSD_MAY_CREATE on the directory. Extending it to every OPEN makes it
> reachable from any client with lookup access.
> 
> Could the access check move ahead of the open for the non-creating
> case, or could the open be done O_RDONLY for the type check and the
> share-access open deferred to the filecache as before?

Yes.  vfs_lookup_open() really should be doing an access check before
calling vfs_open() - calling do_open() there has the desired effect.
I've added a patch.

> 
> 
> > @@ -387,7 +396,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >  	if (IS_ERR(open->op_filp)) {
> >  		status = nfserrno(PTR_ERR(open->op_filp));
> >  		if (status == nfserr_wrong_type) {
> > -			if (nfsd4_create_is_exclusive(open->op_createmode))
> > +			if (nfsd4_create_is_exclusive(createmode))
> >  				status = nfserr_exist;
> >  			else
> >  				status = nfsd_check_obj_isreg(parent.dentry);
> 
> On a re-export this arm changes the status a non-creating OPEN of a
> directory returns. nfsd_check_obj_isreg(NULL) returns
> nfserr_wrong_type, where nfsd_lookup() used to return nfserr_isdir.

I've changed this code a lot - there is no longer a chance of this
problem occurring.

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

I've decide to fix the various atomic_open implementations so they
consistently do what I want them too :-) I've even added
NFS4ERR_WRONG_TYPE handling for nfs.

Thanks for the awesome review!

NeilBrown


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