From 01cba263d1bdb4683224330ab030194b2150ef92 Mon Sep 17 00:00:00 2001 From: Qianchang Zhao Date: Mon, 24 Nov 2025 16:05:09 +0900 Subject: [PATCH] ksmbd: vfs: fix race on m_flags in vfs_cache ksmbd maintains delete-on-close and pending-delete state in ksmbd_inode->m_flags. In vfs_cache.c this field is accessed under inconsistent locking: some paths read and modify m_flags under ci->m_lock while others do so without taking the lock at all. Examples: - ksmbd_query_inode_status() and __ksmbd_inode_close() use ci->m_lock when checking or updating m_flags. - ksmbd_inode_pending_delete(), ksmbd_set_inode_pending_delete(), ksmbd_clear_inode_pending_delete() and ksmbd_fd_set_delete_on_close() used to read and modify m_flags without ci->m_lock. This creates a potential data race on m_flags when multiple threads open, close and delete the same file concurrently. In the worst case delete-on-close and pending-delete bits can be lost or observed in an inconsistent state, leading to confusing delete semantics (files that stay on disk after delete-on-close, or files that disappear while still in use). Fix it by: - Making ksmbd_query_inode_status() look at m_flags under ci->m_lock after dropping inode_hash_lock. - Adding ci->m_lock protection to all helpers that read or modify m_flags (ksmbd_inode_pending_delete(), ksmbd_set_inode_pending_delete(), ksmbd_clear_inode_pending_delete(), ksmbd_fd_set_delete_on_close()). - Keeping the existing ci->m_lock protection in __ksmbd_inode_close(), and moving the actual unlink/xattr removal outside the lock. This unifies the locking around m_flags and removes the data race while preserving the existing delete-on-close behaviour. Reported-by: Qianchang Zhao Reported-by: Zhitong Liu Signed-off-by: Qianchang Zhao Acked-by: Namjae Jeon Signed-off-by: Steve French --- fs/smb/server/vfs_cache.c | 88 +++++++++++++++++++++++++++------------ 1 file changed, 62 insertions(+), 26 deletions(-) diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c index dfed6fce8904..6ef116585af6 100644 --- a/fs/smb/server/vfs_cache.c +++ b/fs/smb/server/vfs_cache.c @@ -112,40 +112,62 @@ int ksmbd_query_inode_status(struct dentry *dentry) read_lock(&inode_hash_lock); ci = __ksmbd_inode_lookup(dentry); - if (ci) { - ret = KSMBD_INODE_STATUS_OK; - if (ci->m_flags & (S_DEL_PENDING | S_DEL_ON_CLS)) - ret = KSMBD_INODE_STATUS_PENDING_DELETE; - atomic_dec(&ci->m_count); - } read_unlock(&inode_hash_lock); + if (!ci) + return ret; + + down_read(&ci->m_lock); + if (ci->m_flags & (S_DEL_PENDING | S_DEL_ON_CLS)) + ret = KSMBD_INODE_STATUS_PENDING_DELETE; + else + ret = KSMBD_INODE_STATUS_OK; + up_read(&ci->m_lock); + + atomic_dec(&ci->m_count); return ret; } bool ksmbd_inode_pending_delete(struct ksmbd_file *fp) { - return (fp->f_ci->m_flags & (S_DEL_PENDING | S_DEL_ON_CLS)); + struct ksmbd_inode *ci = fp->f_ci; + int ret; + + down_read(&ci->m_lock); + ret = (ci->m_flags & (S_DEL_PENDING | S_DEL_ON_CLS)); + up_read(&ci->m_lock); + + return ret; } void ksmbd_set_inode_pending_delete(struct ksmbd_file *fp) { - fp->f_ci->m_flags |= S_DEL_PENDING; + struct ksmbd_inode *ci = fp->f_ci; + + down_write(&ci->m_lock); + ci->m_flags |= S_DEL_PENDING; + up_write(&ci->m_lock); } void ksmbd_clear_inode_pending_delete(struct ksmbd_file *fp) { - fp->f_ci->m_flags &= ~S_DEL_PENDING; + struct ksmbd_inode *ci = fp->f_ci; + + down_write(&ci->m_lock); + ci->m_flags &= ~S_DEL_PENDING; + up_write(&ci->m_lock); } void ksmbd_fd_set_delete_on_close(struct ksmbd_file *fp, int file_info) { - if (ksmbd_stream_fd(fp)) { - fp->f_ci->m_flags |= S_DEL_ON_CLS_STREAM; - return; - } + struct ksmbd_inode *ci = fp->f_ci; - fp->f_ci->m_flags |= S_DEL_ON_CLS; + down_write(&ci->m_lock); + if (ksmbd_stream_fd(fp)) + ci->m_flags |= S_DEL_ON_CLS_STREAM; + else + ci->m_flags |= S_DEL_ON_CLS; + up_write(&ci->m_lock); } static void ksmbd_inode_hash(struct ksmbd_inode *ci) @@ -257,27 +279,41 @@ static void __ksmbd_inode_close(struct ksmbd_file *fp) struct file *filp; filp = fp->filp; - if (ksmbd_stream_fd(fp) && (ci->m_flags & S_DEL_ON_CLS_STREAM)) { - ci->m_flags &= ~S_DEL_ON_CLS_STREAM; - err = ksmbd_vfs_remove_xattr(file_mnt_idmap(filp), - &filp->f_path, - fp->stream.name, - true); - if (err) - pr_err("remove xattr failed : %s\n", - fp->stream.name); + + if (ksmbd_stream_fd(fp)) { + bool remove_stream_xattr = false; + + down_write(&ci->m_lock); + if (ci->m_flags & S_DEL_ON_CLS_STREAM) { + ci->m_flags &= ~S_DEL_ON_CLS_STREAM; + remove_stream_xattr = true; + } + up_write(&ci->m_lock); + + if (remove_stream_xattr) { + err = ksmbd_vfs_remove_xattr(file_mnt_idmap(filp), + &filp->f_path, + fp->stream.name, + true); + if (err) + pr_err("remove xattr failed : %s\n", + fp->stream.name); + } } if (atomic_dec_and_test(&ci->m_count)) { + bool do_unlink = false; + down_write(&ci->m_lock); if (ci->m_flags & (S_DEL_ON_CLS | S_DEL_PENDING)) { ci->m_flags &= ~(S_DEL_ON_CLS | S_DEL_PENDING); - up_write(&ci->m_lock); - ksmbd_vfs_unlink(filp); - down_write(&ci->m_lock); + do_unlink = true; } up_write(&ci->m_lock); + if (do_unlink) + ksmbd_vfs_unlink(filp); + ksmbd_inode_free(ci); } } -- 2.25.1