Re: [PATCH 1/3] smb/client: don't unhash and rehash to prevent new opens.

[NeilBrown]

On Thu, 22 Oct 2026, Tom Talpey wrote:
> One nit below...
> 
> On 9/27/2026 9:25 PM, NeilBrown wrote:
> > From: NeilBrown <neil@xxxxxxxxxx>
> > 
> > smb/client needs to block new opens of the target of unlink and rename
> > while the operation is progressing.  This stablises d_count() and allows
> > a determination of whether a "silly-rename" is required.
> > 
> > It currently unhashes the dentry which will cause lookup to block on
> > the parent directory i_rwsem.  Proposed changes to locking will cause
> > this approach to stop working as the exclusivity will be provided for
> > the dentry only, and only while it is hashed.
> > 
> > So we introduce a new machanism similar to that used by nfs.
> > DCACHE_PRIVATE (given the name DCACHE_BLOCKED) is set when lookups need
> > to be blocked.  ->d_revalidate checks for this and blocks.  This might
> > still allow d_count() to increment, but once it has been tested as 1,
> > there can be no new opens completed.
> > 
> > Unlike unhash which does not need to be reverted on error, and which
> > must not be reverted on a successful d_move, blocking of opens must
> > always be reverted.  So we don't block the open until after the last
> > early "return", and we always unblock on the final "return".
> > 
> > Strictly speaking it is only necessary to block opens before the value
> > of d_count() is tested under ->d_lock. But we block as early as possible
> > to discourage new opens from starting.  As open is path-based in cifs a
> > concurrent rename can be confused a rename of one other the paths.  Once
> > cifs_open() has generated the full_path it doesn't hold any locks to
> > prevent a rename from making that path invalid.
> > 
> > The important details of the interlock between __cifs_unlink (which both
> > unlink and rename use) and "open" are that __cifs_unlink() sets
> > DCACHE_BLOCKED  *before* testing d_count() which is
> > in a d_lock locked region,  and cifs_d_revalidate() tests the bit *after*
> > ->d_lock is taken by e.g. __d_lookup() to increment d_count().
> > Thus ->d_lock provide serialization between the set and the test
> > even though the test isn't in a locked region.
> > (The fact that cifs_d_revalidate() always returned -ECHILD when
> >   LOOKUP_RCU is important for this sequencing to work as it ensures
> >   d_revalidate() is called *after* d_count() is incremented).
> > 
> > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> > ---
> >   fs/smb/client/cifsfs.h |  8 +++++
> >   fs/smb/client/dir.c    |  3 ++
> >   fs/smb/client/inode.c  | 68 ++++++++++++++++++++++++------------------
> >   3 files changed, 50 insertions(+), 29 deletions(-)
> > 
> > diff --git a/fs/smb/client/cifsfs.h b/fs/smb/client/cifsfs.h
> > index 0c85daa8386e..bcc0fb5c2c9b 100644
> > --- a/fs/smb/client/cifsfs.h
> > +++ b/fs/smb/client/cifsfs.h
> > @@ -42,6 +42,14 @@ static inline unsigned long cifs_get_time(struct dentry *dentry)
> >   	return (unsigned long) dentry->d_fsdata;
> >   }
> >   
> > +/*
> > + * This is set to block d_revalidate on a dentry that is being removed -
> > + * the target of unlink or rename.  This causes any open attempt to
> > + * block.  There may be existing opens but they can be detected by
> > + * checking d_count() under ->d_lock.
> > + */
> > +#define DCACHE_BLOCKED DCACHE_PRIVATE
> 
> Why the alias? The blocking behavior is reasonable but #defining a
> flag value which is purely local to this file seems unnecessary and
> confusing. What if another file in fs/smb/client also overloads the
> DCACHE_PRIVATE flag, for example. Consider removing this hunk.


I thought that giving the flag a meaningful name would make the code
that uses it easier to read. But I'm happy to drop it.
I'll resend with the IS_ERR_OR_NULL change too.

Thanks,
NeilBrown




> 
> Tom.
> 
> 
> > +
> >   extern struct file_system_type cifs_fs_type, smb3_fs_type;
> >   extern const struct address_space_operations cifs_addr_ops;
> >   extern const struct address_space_operations cifs_addr_ops_smallbuf;
> > diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
> > index 6fa6d48fdfd3..88fff65320f5 100644
> > --- a/fs/smb/client/dir.c
> > +++ b/fs/smb/client/dir.c
> > @@ -872,6 +872,9 @@ cifs_d_revalidate(struct inode *dir, const struct qstr *name,
> >   	if (flags & LOOKUP_RCU)
> >   		return -ECHILD;
> >   
> > +	/* Wait for pending rename/unlink */
> > +	wait_var_event(&direntry->d_flags, !(direntry->d_flags & DCACHE_BLOCKED));
> > +
> >   	if (d_really_is_positive(direntry)) {
> >   		int rc;
> >   		struct inode *inode = d_inode(direntry);
> > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> > index 12ed8db10e00..4d34c5622cd4 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -1967,24 +1967,31 @@ static int __cifs_unlink(struct inode *dir, struct dentry *dentry, bool sillyren
> >   	__u32 dosattr = 0, origattr = 0;
> >   	struct TCP_Server_Info *server;
> >   	struct iattr *attrs = NULL;
> > -	bool rehash = false;
> > +	bool unblock = false;
> >   
> >   	cifs_dbg(FYI, "cifs_unlink, dir=0x%p, dentry=0x%p\n", dir, dentry);
> >   
> >   	if (unlikely(cifs_forced_shutdown(cifs_sb)))
> >   		return smb_EIO(smb_eio_trace_forced_shutdown);
> >   
> > -	/* Unhash dentry in advance to prevent any concurrent opens */
> > -	spin_lock(&dentry->d_lock);
> > -	if (!d_unhashed(dentry)) {
> > -		__d_drop(dentry);
> > -		rehash = true;
> > -	}
> > -	spin_unlock(&dentry->d_lock);
> > -
> >   	tlink = cifs_sb_tlink(cifs_sb);
> >   	if (IS_ERR(tlink))
> >   		return PTR_ERR(tlink);
> > +
> > +	/* opens might already be blocked by rename */
> > +	if (!(dentry->d_flags & DCACHE_BLOCKED)) {
> > +		/*
> > +		 * Block opens - and all lookups that involve d_revalidate.
> > +		 * This guarantees that if another thread tries to open(), it
> > +		 * will either block, or will increment d_count()
> > +		 * before we test it below.
> > +		 */
> > +		spin_lock(&dentry->d_lock);
> > +		dentry->d_flags |= DCACHE_BLOCKED;
> > +		spin_unlock(&dentry->d_lock);
> > +		unblock = true;
> > +	}
> > +
> >   	tcon = tlink_tcon(tlink);
> >   	server = tcon->ses->server;
> >   
> > @@ -2107,8 +2114,13 @@ static int __cifs_unlink(struct inode *dir, struct dentry *dentry, bool sillyren
> >   	kfree(attrs);
> >   	free_xid(xid);
> >   	cifs_put_tlink(tlink);
> > -	if (rehash)
> > -		d_rehash(dentry);
> > +	/* Allow lookups/opens */
> > +	if (unblock) {
> > +		spin_lock(&dentry->d_lock);
> > +		store_release_wake_up(&dentry->d_flags,
> > +				      dentry->d_flags &~ DCACHE_BLOCKED);
> > +		spin_unlock(&dentry->d_lock);
> > +	}
> >   	return rc;
> >   }
> >   
> > @@ -2536,7 +2548,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
> >   	struct cifs_sb_info *cifs_sb;
> >   	struct tcon_link *tlink;
> >   	struct cifs_tcon *tcon;
> > -	bool rehash = false;
> >   	unsigned int xid;
> >   	int rc, tmprc;
> >   	int retry_count = 0;
> > @@ -2552,20 +2563,20 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
> >   	if (unlikely(cifs_forced_shutdown(cifs_sb)))
> >   		return smb_EIO(smb_eio_trace_forced_shutdown);
> >   
> > -	/*
> > -	 * Prevent any concurrent opens on the target by unhashing the dentry.
> > -	 * VFS already unhashes the target when renaming directories.
> > -	 */
> > -	if (d_is_positive(target_dentry) && !d_is_dir(target_dentry)) {
> > -		if (!d_unhashed(target_dentry)) {
> > -			d_drop(target_dentry);
> > -			rehash = true;
> > -		}
> > -	}
> > -
> >   	tlink = cifs_sb_tlink(cifs_sb);
> >   	if (IS_ERR(tlink))
> >   		return PTR_ERR(tlink);
> > +
> > +	/*
> > +	 * Block opens - and all lookups that involve d_revalidate.
> > +	 * This guarantees that if another thread tries to open(), it
> > +	 * will either block, or will increment d_count()
> > +	 * before we test it in __cifs_unlink().
> > +	 */
> > +	spin_lock(&target_dentry->d_lock);
> > +	target_dentry->d_flags |= DCACHE_BLOCKED;
> > +	spin_unlock(&target_dentry->d_lock);
> > +
> >   	tcon = tlink_tcon(tlink);
> >   	server = tcon->ses->server;
> >   
> > @@ -2605,8 +2616,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
> >   		}
> >   	}
> >   
> > -	if (!rc)
> > -		rehash = false;
> >   	/*
> >   	 * No-replace is the natural behavior for CIFS, so skip unlink hacks.
> >   	 */
> > @@ -2698,8 +2707,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
> >   			}
> >   			rc = cifs_do_rename(xid, source_dentry, from_name,
> >   					    target_dentry, to_name);
> > -			if (!rc)
> > -				rehash = false;
> >   		}
> >   	}
> >   
> > @@ -2713,8 +2720,11 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
> >   	CIFS_I(source_dir)->time = CIFS_I(target_dir)->time = 0;
> >   
> >   cifs_rename_exit:
> > -	if (rehash)
> > -		d_rehash(target_dentry);
> > +	/* Allow lookups/opens */
> > +	spin_lock(&target_dentry->d_lock);
> > +	store_release_wake_up(&target_dentry->d_flags,
> > +			      target_dentry->d_flags &~ DCACHE_BLOCKED);
> > +	spin_unlock(&target_dentry->d_lock);
> >   	kfree(info_buf_source);
> >   	free_dentry_path(page2);
> >   	free_dentry_path(page1);
> > 
> > base-commit: 3879f51857325da9bf3cfb073280257cd16ae067
> 
>