* Re: [PATCH] ksmbd: vfs: fix race on m_flags in vfs_cache
2025-11-23 2:54 ` [PATCH] ksmbd: vfs: fix race on m_flags in vfs_cache Qianchang Zhao
@ 2025-11-24 7:33 ` Namjae Jeon
0 siblings, 0 replies; 2+ messages in thread
From: Namjae Jeon @ 2025-11-24 7:33 UTC (permalink / raw)
To: Qianchang Zhao
Cc: Steve French, gregkh, linux-cifs, linux-kernel, Zhitong Liu, stable
[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]
On Sun, Nov 23, 2025 at 11:54 AM Qianchang Zhao <pioooooooooip@gmail.com> wrote:
>
> 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 <pioooooooooip@gmail.com>
> Reported-by: Zhitong Liu <liuzhitong1993@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
I have directly updated your patch and applied it to #ksmbd-for-next-next.
Please check the attached patch and let me know if you find any issues.
Thanks.
[-- Attachment #2: 0001-ksmbd-vfs-fix-race-on-m_flags-in-vfs_cache.patch --]
[-- Type: text/x-patch, Size: 5515 bytes --]
From 01cba263d1bdb4683224330ab030194b2150ef92 Mon Sep 17 00:00:00 2001
From: Qianchang Zhao <pioooooooooip@gmail.com>
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 <pioooooooooip@gmail.com>
Reported-by: Zhitong Liu <liuzhitong1993@gmail.com>
Signed-off-by: Qianchang Zhao <pioooooooooip@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
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
^ permalink raw reply [flat|nested] 2+ messages in thread