Re: [PATCH v3 14/17] nfsd: reduce want-write range in nfsd4_create_file()

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2026-07-13 at 16:15 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd4_create_file() needs write access to the mount for two purposes:
> 
> 1/ to create/open the file.
> 2/ to set attributes on the newly created (or pre-existing) file.
> 
> Currently this is all handled by holding the write access across the
> open and the setattr.  A subsequent patch will necessarily change how
> write access is gained for the open.  So we reduce the range for the
> first want_write, and add another one to cover setattr.  If we failed to
> get write access, it is only fatal if there were attrs to set.
> 
> We call nfsd_create_setattr() if at all possible, even when no attrs, as
> it also calls commit_metadata and we need to be certain that the file
> creation has been synced.  If the mount became read-only since the
> creation happened, we can safely assume that the sync happened as part
> of that.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4proc.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index adfc1f5ccd98..0d1bcb12ecbc 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -344,6 +344,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  			       &QSTR_LEN(open->op_fname, open->op_fnamelen));
>  	if (IS_ERR(child)) {
>  		status = nfserrno(PTR_ERR(child));
> +		if (!want_write_err)
> +			fh_drop_write(fhp);
>  		goto out;
>  	}
>  	path.dentry = child;
> @@ -378,6 +380,8 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  		}
>  	}
>  	end_creating(child);
> +	if (!want_write_err)
> +		fh_drop_write(fhp);
>  	if (status != nfs_ok)
>  		goto out;
>  
> @@ -421,7 +425,16 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if ((iap->ia_valid & ATTR_SIZE) && (iap->ia_size == 0))
>  		iap->ia_valid &= ~ATTR_SIZE;
>  
> -	status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
> +	/* We will need write access to set the attrs */
> +	want_write_err = fh_want_write(fhp);
> +	if (!want_write_err) {
> +		status = nfsd_create_setattr(rqstp, fhp,
> +					     resfhp, &attrs);
> +		fh_drop_write(fhp);
> +	} else if (nfsd_attrs_valid(&attrs)) {
> +		/* Needed write access */
> +		status = nfserrno(want_write_err);
> +	}
>  
>  	if (attrs.na_labelerr)
>  		open->op_bmval[2] &= ~FATTR4_WORD2_SECURITY_LABEL;
> @@ -432,8 +445,6 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	if (attrs.na_paclerr)
>  		open->op_bmval[2] &= ~FATTR4_WORD2_POSIX_ACCESS_ACL;
>  out:
> -	if (!want_write_err)
> -		fh_drop_write(fhp);
>  	nfsd_attrs_free(&attrs);
>  	return status;
>  }

It sucks that file creation is so fraught with peril and places that
things can go wrong and leave stuff sitting out on the fs. I wonder if
we ought to be using O_TMPFILE where possible to do all of this setup
and only later link it into the namespace. That's a much bigger change
though of course.

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