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

[Tom Talpey <tom@talpey.com>]

One nit below...

On 9/27/2026 9:25 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> 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@brown.name>
> ---
>   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.

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