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

On Mon, 13 Jul 2026, Chuck Lever wrote:
> 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).

Thanks for the review! I'll develop some fixes in a day or 3.
I wouldn't bother back poring.  This is not a regression and is not
exploitable.

This has *never* worked correctly.  CREATE has *always* ignored mounts.
unchecked-create is unique in that it doesn't fail with -EEXIST, but
succeeds without having gone through nfsd_lookup().
Maybe I should try to refactor nfsd_lookup() and use the same code...

Thanks,
NeilBrown