Re: [PATCH v3 02/17] nfsd: correctly handle CREATE of mounted-on files

Hi Neil-

On Mon, Jul 13, 2026, at 2:15 AM, NeilBrown wrote:
> From: NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>
>
> Linux allows a file (non-directory) to be mounted on a file.  nfsd
> mostly supports this if the crossmnt option is in effect.  However if
> CREATE is used on an existing mounted-on file, the filehandle for the
> underlying file is returns.  The client will then continue to use that
> filehandle.
>
> So
>   cat /mnt/file
> will show the contents of the mounted file as expected, but if
> the dcache is flushed with "drop_caches" or similar, then
>   >> /mnt/file
>   cat /mnt/file
> will show the mounted-on file.
>
> For exclusive or checked creates this is not a problem as the creation
> will fail no matter which file is seen. For unchecked creates we need to
> see if the name is in the dcache, and if it is mounted.  If so, we
> simply provide that filehandle, possibly truncating.
>
> Signed-off-by: NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>

I didn't see issues in the other patches in this series, but this
new one does have some correctness issues. This one doesn't build
here with CONFIG_NFSD_V2=y, and the NFSv2 and NFSv3 create paths
have some refcount and behavior problems. The NFSv4 path looks
good.

Big picture: the three create paths now handle an existing
mounted-on file three different ways. v4 sets op_truncate for
size-zero truncation only, v3 applies the full client iattr, and
v2 applies nothing.  The v4 behavior is the one I prefer, so
bring v2 and v3 into line with it.

Specifics below.


> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index bbaef884f893..20eaf56fa9e7 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -303,6 +303,34 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct 
> svc_fh *fhp,
>  	parent = fhp->fh_dentry;
>  	inode = d_inode(parent);
> 
> +	if (argp->createmode == NFS3_CREATE_UNCHECKED) {
> +		/*
> +		 * If name is already in dcache we need to check for mountpoints
> +		 */
> +		child = try_lookup_noperm(&QSTR_LEN(argp->name,
> +						    argp->len),
> +					  parent);
> +		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);
> +			if (nfsd_cross_mnt(rqstp, &child, &exp) == 0) {
> +				status = check_nfsd_access(exp, rqstp, false);
> +				if (status == nfs_ok)
> +					status = fh_compose(resfhp, exp,
> +							    child, fhp);
> +				if (status == nfs_ok)
> +					status = nfsd_create_setattr(
> +						rqstp, fhp, resfhp, &attrs);
> +				dput(child);
> +				exp_put(exp);
> +				return status;
> +			}
> +			exp_put(exp);
> +		}
> +		if (!IS_ERR(child))
> +			dput(child);
> +	}
> +
>  	host_err = fh_want_write(fhp);
>  	if (host_err)
>  		return nfserrno(host_err);

The ordinary UNCHECKED path masks iap->ia_valid to ATTR_SIZE before
calling nfsd_create_setattr(). This branch passes the full client
iattr, so it applies atime/mtime to the existing mounted-on file
that the ordinary create path drops. Mask to ATTR_SIZE here too.


> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index f60043632575..549eed8f2c19 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -302,11 +302,34 @@ nfsd_proc_create(struct svc_rqst *rqstp)
>  	if (resp->status != nfs_ok)
>  		goto done; /* must fh_put dirfhp even on error */
> 
> +	fh_init(newfhp, NFS_FHSIZE);
> +
>  	/* Check for NFSD_MAY_WRITE in nfsd_create if necessary */
> 
>  	resp->status = nfserr_exist;
>  	if (name_is_dot_dotdot(argp->name, argp->len))
>  		goto done;
> +
> +	/*
> +	 * If name is already in dcache we need to check for mountpoints
> +	 */
> +	dchild = try_lookup_noperm(&QSTR_LEN(argp->name, argp->len),
> +				   dirfhp->fh_export);
> +	if (dchild && !IS_ERR(dchild) && d_is_reg(child) &&
> +	    unlikely(nfsd_mountpoint(dchild, fhp->fh_export))) {

This hunk does not compile with CONFIG_NFSD_V2=y.


> +		struct svc_export *exp = fhp->fh_export;
> +		if (nfsd_cross_mnt(rqstp, &dchild, &exp) == 0 &&
> +		    d_isreg(dchild)) {

nfsd_cross_mnt() drops a reference on the export it is given and
returns referenced replacements in dchild and exp. This branch
hands it the filehandle's borrowed fh_export with no exp_get(),
so a successful crossing underflows the export refcount. It then
jumps to done without releasing either replacement, leaking dchild
and exp. The v3 and v4 hunks get this right: exp_get() first,
dput(child) and exp_put(exp) after.


> +			resp->status = check_nfsd_access(exp, rqstp, false);
> +			if (resp->status == nfs_ok)
> +				resp->status = fh_compose(newfhp, dirfhp->fh_export,
> +							  dchild, dirfhp);

After the crossing, dchild is on the mounted filesystem, which exp
describes, not dirfhp->fh_export. Thus fh_compose() must use exp
here.


> +			goto done;

The normal v2 path truncates an existing regular file: it masks to
ATTR_SIZE and calls nfsd_setattr(). This branch returns without
truncating, so an UNCHECKED create with size zero leaves the
mounted-on file's contents intact.


Lastly, should we consider this patch for backporting to LTS? If
so, I'm guessing the issues it fixes were introduced at different
points in the commit history, so this patch would have to be split
accordingly. (If no backporting is necessary, then it can remain
as a single patch).


-- 
Chuck Lever