]> git.ipfire.org Git - thirdparty/kernel/stable.git/commitdiff
ksmbd: centralize ksmbd_conn final release to plug transport leak
authorDaeMyung Kang <charsyam@gmail.com>
Tue, 28 Apr 2026 14:08:54 +0000 (23:08 +0900)
committerSteve French <stfrench@microsoft.com>
Sat, 2 May 2026 02:49:35 +0000 (21:49 -0500)
ksmbd_conn_free() is one of four sites that can observe the last
refcount drop of a struct ksmbd_conn.  The other three

    fs/smb/server/connection.c    ksmbd_conn_r_count_dec()
    fs/smb/server/oplock.c        __free_opinfo()
    fs/smb/server/vfs_cache.c     session_fd_check()

end the conn with a bare kfree(), skipping
ida_destroy(&conn->async_ida) and
conn->transport->ops->free_transport(conn->transport).  Whenever one
of them is the last putter, the embedded async_ida and the entire
transport struct leak -- for TCP, that is also the struct socket and
the kvec iov.

__free_opinfo() being a final putter is not theoretical.  opinfo_put()
queues the callback via call_rcu(&opinfo->rcu, free_opinfo_rcu), so
ksmbd_server_terminate_conn() can deposit N opinfo releases in RCU and
have ksmbd_conn_free() run in the handler thread before any of them
fire.  ksmbd_conn_free() then observes refcnt > 0 and short-circuits;
the last RCU-delivered __free_opinfo() falls onto its bare kfree(conn)
branch and the transport is lost.

A/B validation in a QEMU/virtme guest, mounting //127.0.0.1/testshare:
each iteration holds 8 files open via sleep processes, force-closes
TCP with "ss -K sport = :445", kills the holders, lazy-umounts;
repeated 10 times, then ksmbd shutdown and kmemleak scan.

    state         conn_alloc  conn_free  tcp_free  opi_rcu  kmemleak
    ----------    ----------  ---------  --------  -------  --------
    pre-patch         20          20        10       160        7
    with patch        20          20        20       160        0

Pre-patch conn_free=20 with tcp_free=10 directly demonstrates the
bare-kfree paths skipping transport cleanup; kmemleak backtraces point
into struct tcp_transport / iov.  With this patch tcp_free matches
conn_free at 20/20 and kmemleak is clean.

Move the per-struct final release into __ksmbd_conn_release_work() and
route the three bare-kfree final-put sites through a new
ksmbd_conn_put().  Those sites now pair ida_destroy() and
free_transport() with kfree(conn) regardless of which holder happens
to release the last reference.  stop_sessions() only triggers the
transport shutdown and does not itself drop the last conn reference,
so it is unaffected.

The centralized release reaches sock_release() -> tcp_close() ->
lock_sock_nested() (might_sleep) from every final putter, including
__free_opinfo() invoked from an RCU softirq callback, which trips
CONFIG_DEBUG_ATOMIC_SLEEP.  Defer the release to a dedicated
ksmbd_conn_wq workqueue so ksmbd_conn_put() is safe from any
non-sleeping context.

Make ksmbd_file own a strong connection reference while fp->conn is
non-NULL so durable-preserve and final-close paths cannot dereference
a stale connection.  ksmbd_open_fd() and ksmbd_reopen_durable_fd()
take the reference via ksmbd_conn_get() (the latter also reorders the
fp->conn / fp->tcon assignments before __open_id() so the published fp
is never observed with fp->conn == NULL); session_fd_check() and
__ksmbd_close_fd() drop it via ksmbd_conn_put().  With that invariant,
session_fd_check() can take a local conn pointer once and use it
across the m_op_list and lock_list iterations even though op->conn
puts may otherwise drop the last reference.

At module exit the workqueue is flushed and destroyed after
rcu_barrier(), so any release queued by a trailing RCU callback is
drained before the inode hash and module text go away.

Fixes: ee426bfb9d09 ("ksmbd: add refcnt to ksmbd_conn struct")
Signed-off-by: DaeMyung Kang <charsyam@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
fs/smb/server/connection.c
fs/smb/server/connection.h
fs/smb/server/oplock.c
fs/smb/server/server.c
fs/smb/server/vfs_cache.c

index c5aac4946cbe52691d68345e8965e15e7211a01e..95654a8081628607eec0c6897f539ff456bd9f87 100644 (file)
@@ -79,6 +79,81 @@ static int create_proc_clients(void) { return 0; }
 static void delete_proc_clients(void) {}
 #endif
 
+static struct workqueue_struct *ksmbd_conn_wq;
+
+int ksmbd_conn_wq_init(void)
+{
+       ksmbd_conn_wq = alloc_workqueue("ksmbd-conn-release",
+                                       WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
+       if (!ksmbd_conn_wq)
+               return -ENOMEM;
+       return 0;
+}
+
+void ksmbd_conn_wq_destroy(void)
+{
+       if (ksmbd_conn_wq) {
+               destroy_workqueue(ksmbd_conn_wq);
+               ksmbd_conn_wq = NULL;
+       }
+}
+
+/*
+ * __ksmbd_conn_release_work() - perform the final, once-per-struct cleanup
+ * of a ksmbd_conn whose refcount has just dropped to zero.
+ *
+ * This is the common release path used by ksmbd_conn_put() for the embedded
+ * state that outlives the connection thread: async_ida and the attached
+ * transport (which owns the socket and iov for TCP).  Called from a workqueue
+ * so that sleep-allowed teardown (sock_release -> tcp_close ->
+ * lock_sock_nested) never runs from an RCU softirq callback (free_opinfo_rcu)
+ * or any other non-sleeping putter context.
+ */
+static void __ksmbd_conn_release_work(struct work_struct *work)
+{
+       struct ksmbd_conn *conn =
+               container_of(work, struct ksmbd_conn, release_work);
+
+       ida_destroy(&conn->async_ida);
+       conn->transport->ops->free_transport(conn->transport);
+       kfree(conn);
+}
+
+/**
+ * ksmbd_conn_get() - take a reference on @conn and return it.
+ *
+ * Returns @conn unchanged so callers can write
+ * "fp->conn = ksmbd_conn_get(work->conn);" in one expression.  Returns NULL
+ * if @conn is NULL.
+ */
+struct ksmbd_conn *ksmbd_conn_get(struct ksmbd_conn *conn)
+{
+       if (!conn)
+               return NULL;
+
+       atomic_inc(&conn->refcnt);
+       return conn;
+}
+
+/**
+ * ksmbd_conn_put() - drop a reference and, if it was the last, queue the
+ * release onto ksmbd_conn_wq so it runs from process context.
+ *
+ * Callable from any context including RCU softirq callbacks and non-sleeping
+ * locks; the actual release is deferred to the workqueue.  ksmbd_conn_wq is
+ * created in ksmbd_server_init() before any conn can be allocated and is
+ * destroyed in ksmbd_server_exit() after rcu_barrier(), so it is always
+ * non-NULL while a conn reference is held.
+ */
+void ksmbd_conn_put(struct ksmbd_conn *conn)
+{
+       if (!conn)
+               return;
+
+       if (atomic_dec_and_test(&conn->refcnt))
+               queue_work(ksmbd_conn_wq, &conn->release_work);
+}
+
 /**
  * ksmbd_conn_free() - free resources of the connection instance
  *
@@ -93,23 +168,19 @@ void ksmbd_conn_free(struct ksmbd_conn *conn)
        hash_del(&conn->hlist);
        up_write(&conn_list_lock);
 
+       /*
+        * request_buf / preauth_info / mechToken are only ever accessed by the
+        * connection handler thread that owns @conn.  ksmbd_conn_free() is
+        * called from the transport free_transport() path when that thread is
+        * exiting, so it is safe to release them unconditionally even when
+        * ksmbd_conn_put() below is not the final putter (oplock / ksmbd_file
+        * holders only retain the conn pointer, not these per-thread buffers).
+        */
        xa_destroy(&conn->sessions);
        kvfree(conn->request_buf);
        kfree(conn->preauth_info);
        kfree(conn->mechToken);
-       if (atomic_dec_and_test(&conn->refcnt)) {
-               /*
-                * async_ida is embedded in struct ksmbd_conn, so pair
-                * ida_destroy() with the final kfree() rather than with
-                * the unconditional field teardown above.  This keeps
-                * the IDA valid for the entire lifetime of the struct,
-                * even while other refcount holders (oplock / vfs
-                * durable handles) still reference the connection.
-                */
-               ida_destroy(&conn->async_ida);
-               conn->transport->ops->free_transport(conn->transport);
-               kfree(conn);
-       }
+       ksmbd_conn_put(conn);
 }
 
 /**
@@ -136,6 +207,7 @@ struct ksmbd_conn *ksmbd_conn_alloc(void)
                conn->um = ERR_PTR(-EOPNOTSUPP);
        if (IS_ERR(conn->um))
                conn->um = NULL;
+       INIT_WORK(&conn->release_work, __ksmbd_conn_release_work);
        atomic_set(&conn->req_running, 0);
        atomic_set(&conn->r_count, 0);
        atomic_set(&conn->refcnt, 1);
@@ -512,8 +584,7 @@ void ksmbd_conn_r_count_dec(struct ksmbd_conn *conn)
        if (!atomic_dec_return(&conn->r_count) && waitqueue_active(&conn->r_count_q))
                wake_up(&conn->r_count_q);
 
-       if (atomic_dec_and_test(&conn->refcnt))
-               kfree(conn);
+       ksmbd_conn_put(conn);
 }
 
 int ksmbd_conn_transport_init(void)
index de2d46941c930b393a202b377bd86d684374f210..e074be9425823b70eeceb78f1410ba1a5bc24d62 100644 (file)
@@ -16,6 +16,7 @@
 #include <linux/kthread.h>
 #include <linux/nls.h>
 #include <linux/unicode.h>
+#include <linux/workqueue.h>
 
 #include "smb_common.h"
 #include "ksmbd_work.h"
@@ -120,6 +121,7 @@ struct ksmbd_conn {
        bool                            binding;
        atomic_t                        refcnt;
        bool                            is_aapl;
+       struct work_struct              release_work;
 };
 
 struct ksmbd_conn_ops {
@@ -164,6 +166,10 @@ void ksmbd_conn_wait_idle(struct ksmbd_conn *conn);
 int ksmbd_conn_wait_idle_sess_id(struct ksmbd_conn *curr_conn, u64 sess_id);
 struct ksmbd_conn *ksmbd_conn_alloc(void);
 void ksmbd_conn_free(struct ksmbd_conn *conn);
+struct ksmbd_conn *ksmbd_conn_get(struct ksmbd_conn *conn);
+void ksmbd_conn_put(struct ksmbd_conn *conn);
+int ksmbd_conn_wq_init(void);
+void ksmbd_conn_wq_destroy(void);
 bool ksmbd_conn_lookup_dialect(struct ksmbd_conn *c);
 int ksmbd_conn_write(struct ksmbd_work *work);
 int ksmbd_conn_rdma_read(struct ksmbd_conn *conn,
index cd3f28b0e7cb24923e737c017b41d00569d784fd..8feca02ddbf2595d24de0a7911a9c8f64a41cfb7 100644 (file)
@@ -30,7 +30,6 @@ static DEFINE_RWLOCK(lease_list_lock);
 static struct oplock_info *alloc_opinfo(struct ksmbd_work *work,
                                        u64 id, __u16 Tid)
 {
-       struct ksmbd_conn *conn = work->conn;
        struct ksmbd_session *sess = work->sess;
        struct oplock_info *opinfo;
 
@@ -39,7 +38,7 @@ static struct oplock_info *alloc_opinfo(struct ksmbd_work *work,
                return NULL;
 
        opinfo->sess = sess;
-       opinfo->conn = conn;
+       opinfo->conn = ksmbd_conn_get(work->conn);
        opinfo->level = SMB2_OPLOCK_LEVEL_NONE;
        opinfo->op_state = OPLOCK_STATE_NONE;
        opinfo->pending_break = 0;
@@ -50,7 +49,6 @@ static struct oplock_info *alloc_opinfo(struct ksmbd_work *work,
        init_waitqueue_head(&opinfo->oplock_brk);
        atomic_set(&opinfo->refcount, 1);
        atomic_set(&opinfo->breaking_cnt, 0);
-       atomic_inc(&opinfo->conn->refcnt);
 
        return opinfo;
 }
@@ -132,8 +130,7 @@ static void __free_opinfo(struct oplock_info *opinfo)
 {
        if (opinfo->is_lease)
                free_lease(opinfo);
-       if (opinfo->conn && atomic_dec_and_test(&opinfo->conn->refcnt))
-               kfree(opinfo->conn);
+       ksmbd_conn_put(opinfo->conn);
        kfree(opinfo);
 }
 
index 58ef02c423fcef78d1b8813a6b852b9864aa317a..5d799b2d4c62f4c36dd025e162e1984fe606775c 100644 (file)
@@ -596,8 +596,14 @@ static int __init ksmbd_server_init(void)
        if (ret)
                goto err_crypto_destroy;
 
+       ret = ksmbd_conn_wq_init();
+       if (ret)
+               goto err_workqueue_destroy;
+
        return 0;
 
+err_workqueue_destroy:
+       ksmbd_workqueue_destroy();
 err_crypto_destroy:
        ksmbd_crypto_destroy();
 err_release_inode_hash:
@@ -623,6 +629,12 @@ static void __exit ksmbd_server_exit(void)
 {
        ksmbd_server_shutdown();
        rcu_barrier();
+       /*
+        * ksmbd_conn_put() defers the final release onto ksmbd_conn_wq,
+        * so drain it after rcu_barrier() has fired any pending RCU
+        * callbacks that may have queued a release.
+        */
+       ksmbd_conn_wq_destroy();
        ksmbd_release_inode_hash();
 }
 
index 3551f01a3fa0351d9fd801ccee87c23f3a7db198..4a18107937cc94c27b41f8de353f70285b9b5682 100644 (file)
@@ -475,6 +475,17 @@ static void __ksmbd_close_fd(struct ksmbd_file_table *ft, struct ksmbd_file *fp)
                kfree(smb_lock);
        }
 
+       /*
+        * Drop fp's strong reference on conn (taken in ksmbd_open_fd() /
+        * ksmbd_reopen_durable_fd()).  Durable fps that reached the
+        * scavenger have already had fp->conn cleared by session_fd_check(),
+        * in which case there is nothing to drop here.
+        */
+       if (fp->conn) {
+               ksmbd_conn_put(fp->conn);
+               fp->conn = NULL;
+       }
+
        if (ksmbd_stream_fd(fp))
                kfree(fp->stream.name);
        kfree(fp->owner.name);
@@ -752,7 +763,14 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp)
        atomic_set(&fp->refcount, 1);
 
        fp->filp                = filp;
-       fp->conn                = work->conn;
+       /*
+        * fp owns a strong reference on fp->conn for as long as fp->conn is
+        * non-NULL, so session_fd_check() and __ksmbd_close_fd() never
+        * dereference a dangling pointer.  Paired with ksmbd_conn_put() in
+        * session_fd_check() (durable preserve), in __ksmbd_close_fd()
+        * (final close), and on the error paths below.
+        */
+       fp->conn                = ksmbd_conn_get(work->conn);
        fp->tcon                = work->tcon;
        fp->volatile_id         = KSMBD_NO_FID;
        fp->persistent_id       = KSMBD_NO_FID;
@@ -774,6 +792,8 @@ struct ksmbd_file *ksmbd_open_fd(struct ksmbd_work *work, struct file *filp)
        return fp;
 
 err_out:
+       /* fp->conn was set and refcounted before every branch here. */
+       ksmbd_conn_put(fp->conn);
        kmem_cache_free(filp_cache, fp);
        return ERR_PTR(ret);
 }
@@ -1062,25 +1082,32 @@ static bool session_fd_check(struct ksmbd_tree_connect *tcon,
        if (!is_reconnectable(fp))
                return false;
 
+       if (WARN_ON_ONCE(!fp->conn))
+               return false;
+
        if (ksmbd_vfs_copy_durable_owner(fp, user))
                return false;
 
+       /*
+        * fp owns a strong reference on fp->conn (taken in ksmbd_open_fd()
+        * / ksmbd_reopen_durable_fd()), so conn stays valid for the whole
+        * body of this function regardless of any op->conn puts below.
+        */
        conn = fp->conn;
        ci = fp->f_ci;
        down_write(&ci->m_lock);
        list_for_each_entry_rcu(op, &ci->m_op_list, op_entry) {
                if (op->conn != conn)
                        continue;
-               if (op->conn && atomic_dec_and_test(&op->conn->refcnt))
-                       kfree(op->conn);
+               ksmbd_conn_put(op->conn);
                op->conn = NULL;
        }
        up_write(&ci->m_lock);
 
        list_for_each_entry_safe(smb_lock, tmp_lock, &fp->lock_list, flist) {
-               spin_lock(&fp->conn->llist_lock);
+               spin_lock(&conn->llist_lock);
                list_del_init(&smb_lock->clist);
-               spin_unlock(&fp->conn->llist_lock);
+               spin_unlock(&conn->llist_lock);
        }
 
        fp->conn = NULL;
@@ -1091,6 +1118,8 @@ static bool session_fd_check(struct ksmbd_tree_connect *tcon,
                fp->durable_scavenger_timeout =
                        jiffies_to_msecs(jiffies) + fp->durable_timeout;
 
+       /* Drop fp's own reference on conn. */
+       ksmbd_conn_put(conn);
        return true;
 }
 
@@ -1178,15 +1207,27 @@ int ksmbd_reopen_durable_fd(struct ksmbd_work *work, struct ksmbd_file *fp)
 
        old_f_state = fp->f_state;
        fp->f_state = FP_NEW;
+
+       /*
+        * Initialize fp's connection binding before publishing fp into the
+        * session's file table.  If __open_id() is ordered first, a
+        * concurrent teardown that iterates the table can observe a valid
+        * volatile_id with fp->conn == NULL and preserve a
+        * partially-initialized fp.  fp owns a strong reference on the new
+        * conn (see ksmbd_open_fd()); undo it on __open_id() failure.
+        */
+       fp->conn = ksmbd_conn_get(conn);
+       fp->tcon = work->tcon;
+
        __open_id(&work->sess->file_table, fp, OPEN_ID_TYPE_VOLATILE_ID);
        if (!has_file_id(fp->volatile_id)) {
+               fp->conn = NULL;
+               fp->tcon = NULL;
+               ksmbd_conn_put(conn);
                fp->f_state = old_f_state;
                return -EBADF;
        }
 
-       fp->conn = conn;
-       fp->tcon = work->tcon;
-
        list_for_each_entry(smb_lock, &fp->lock_list, flist) {
                spin_lock(&conn->llist_lock);
                list_add_tail(&smb_lock->clist, &conn->lock_list);
@@ -1198,8 +1239,7 @@ int ksmbd_reopen_durable_fd(struct ksmbd_work *work, struct ksmbd_file *fp)
        list_for_each_entry_rcu(op, &ci->m_op_list, op_entry) {
                if (op->conn)
                        continue;
-               op->conn = fp->conn;
-               atomic_inc(&op->conn->refcnt);
+               op->conn = ksmbd_conn_get(fp->conn);
        }
        up_write(&ci->m_lock);