Re: [PATCH 3/3] NFS: Initialise verifiers for visible dentries in _nfs4_open_and_get_state

[Trond Myklebust <trondmy@kernel.org>]

On Wed, 2025-11-19 at 11:14 -0500, Anna Schumaker wrote:
> Hi Trond,
> 
> On 11/19/25 8:58 AM, Trond Myklebust wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Ensure that the verifiers are initialised before calling
> > d_splice_alias() in _nfs4_open_and_get_state().
> > 
> > Reported-by: Michael Stoler <michael.stoler@vastdata.com>
> > Fixes: cf5b4059ba71 ("NFSv4: Fix races between open and dentry
> > revalidation")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/nfs4proc.c | 27 ++++++++++++++-------------
> >  1 file changed, 14 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 93c6ce04332b..54595983525d 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -3174,18 +3174,6 @@ static int _nfs4_open_and_get_state(struct
> > nfs4_opendata *opendata,
> >  	if (opendata->o_res.rflags &
> > NFS4_OPEN_RESULT_PRESERVE_UNLINKED)
> >  		set_bit(NFS_INO_PRESERVE_UNLINKED, &NFS_I(state-
> > >inode)->flags);
> >  
> > -	dentry = opendata->dentry;
> > -	if (d_really_is_negative(dentry)) {
> > -		struct dentry *alias;
> > -		d_drop(dentry);
> > -		alias = d_splice_alias(igrab(state->inode),
> > dentry);
> > -		/* d_splice_alias() can't fail here - it's a non-
> > directory */
> > -		if (alias) {
> > -			dput(ctx->dentry);
> > -			ctx->dentry = dentry = alias;
> > -		}
> > -	}
> > -
> >  	switch(opendata->o_arg.claim) {
> >  	default:
> >  		break;
> > @@ -3196,7 +3184,20 @@ static int _nfs4_open_and_get_state(struct
> > nfs4_opendata *opendata,
> >  			break;
> >  		if (opendata->o_res.delegation.type != 0)
> >  			dir_verifier =
> > nfs_save_change_attribute(dir);
> > -		nfs_set_verifier(dentry, dir_verifier);
> > +	}
> > +	nfs_set_verifier(dentry, dir_verifier);
> 
> Isn't 'dentry' uninitialized here? As far as I can tell, it gets set
> for the first time
> in the very next statement.

D'oh! I wonder why the compiler let that pass?
Anyhow, yes, the nfs_set_verifier() call needs to be moved one line
down.

> 
> Thanks,
> Anna
> 
> > +
> > +	dentry = opendata->dentry;
> > +	if (d_really_is_negative(dentry)) {
> > +		struct dentry *alias;
> > +		d_drop(dentry);
> > +		alias = d_splice_alias(igrab(state->inode),
> > dentry);
> > +		/* d_splice_alias() can't fail here - it's a non-
> > directory */
> > +		if (alias) {
> > +			dput(ctx->dentry);
> > +			nfs_set_verifier(alias, dir_verifier);
> > +			ctx->dentry = dentry = alias;
> > +		}
> >  	}
> >  
> >  	/* Parse layoutget results before we check for access */

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com