Re: [PATCH v3 resend 3/3] smb/client: fix nlink of an overwritten open file

[hehuiwen <huiwen.he@linux.dev>]

The AI review of the overwrite rename patch raised two possible races.

1. A handle opened through a surviving hard link may be marked deleted:

   target_fh -- target --+
                         +--> inode X (nlink = 2)
   alias_fh  -- alias  --+

   CPU A                                   CPU B
   -----                                   -----
   rename(source, target)
     server nlink: 2 -> 1

                                           statx(alias,
                                                 AT_STATX_FORCE_SYNC)
                                             cached i_nlink: 2 -> 1

   cifs_mark_open_handles_for_deleted_file()
     sees i_nlink == 1
     marks every handle:
       target_fh: deleted                   correct
       alias_fh:  deleted                   wrong

2. A concurrently refreshed link count may be decremented again:

   CPU A                                   CPU B
   -----                                   -----
   rename(source, target)
     server nlink: 2 -> 1

                                           statx(alias,
                                                 AT_STATX_FORCE_SYNC)
                                             cached i_nlink: 2 -> 1

   cifs_drop_nlink()
     actual:   cached i_nlink: 1 -> 0       wrong
     expected: cached i_nlink remains 1

I tried addressing these problems by taking an i_nlink snapshot before
the unlink/rename request, using it to decide which handles to mark,
and decrementing the cached link count only if it still matched the
snapshot.

Unfortunately, an nlink snapshot does not show whether revalidation
happened before or after unlink/rename. Therefore, it cannot fully
address these races or reject stale attribute responses that arrive late.

NFS addresses these issues with per-dentry silly rename, attribute
generation counters, and cache invalidation. A similar CIFS solution
may be challenging, as it must coordinate namespace operations,
open-handle state, and asynchronous attribute updates.

Do you have any ideas?

Thanks,
Huiwen



在 2026/8/5 13:11, ChenXiaoSong 写道:
> From: ChenXiaoSong <chenxiaosong@kylinos.cn>
> 
> Reproducer:
> 
>    1. server: systemctl start ksmbd
>    2. client: mount with `posix` option
>       mount -t cifs -o posix //${server_ip}/export /mnt
>    3. client: touch /mnt/file1 /mnt/file2
>    4. client: C program: int fd = open("/mnt/file2", O_RDONLY);
>    5. client: C program: rename("/mnt/file1", "/mnt/file2");
>    6. client: C program: struct stat stbuf; fstat(fd, &stbuf);
>                          stbuf.st_nlink is 1, should be 0
> 
> This patch fixes xfstests generic/035 when mounted with `posix` option.
> 
> Signed-off-by: ChenXiaoSong <chenxiaosong@kylinos.cn>
> ---
>   fs/smb/client/inode.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index 0afff761aab9..0b6273ce01a8 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -2648,11 +2648,8 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
>   	if (d_really_is_positive(target_dentry)) {
>   		if (!rc) {
>   			struct inode *inode = d_inode(target_dentry);
> -			/*
> -			 * Samba and ksmbd servers allow renaming a target
> -			 * directory that is open, so make sure to update
> -			 * ->i_nlink and then mark it as delete pending.
> -			 */
> +
> +			/* Update the target link count after rename. */
>   			if (S_ISDIR(inode->i_mode)) {
>   				drop_cached_dir_by_name(xid, tcon, to_name, cifs_sb);
>   				spin_lock(&inode->i_lock);
> @@ -2663,6 +2660,10 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
>   				CIFS_I(inode)->time = 0; /* force reval */
>   				inode_set_ctime_current(inode);
>   				inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
> +			} else {
> +				cifs_mark_open_handles_for_deleted_file(inode, to_name);
> +				cifs_drop_nlink(inode);
> +				inode_set_ctime_current(inode);
>   			}
>   		} else if (rc == -EACCES || rc == -EEXIST) {
>   			/*