Re: [PATCH 2/2] ocfs2: o2hb: quiesce negotiate handlers and timeout work
Joseph Qi <[email protected]> Fri, 24 Jul 2026 09:41:29 +0800
| Newsgroups | dev.linux.lists.ocfs2-devel,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On 6/24/26 5:53 PM, Cen Zhang wrote:
> Heartbeat regions publish struct o2hb_region as the private data for the
> NEGO_TIMEOUT and NEGO_APPROVE o2net handlers as soon as make_item()
> creates the configfs region. The approve handler can call
> o2hb_arm_timeout(), so a peer can touch the region timeout work before
> dev_store() has finished building the heartbeat runtime, or after
> teardown has started to shut that runtime back down.
>
> The final configfs put also has to keep reg alive until the last
> in-flight o2net callback drops its handler reference.
> o2net_unregister_handler_list() blocks future handler lookups, but it
> does not wait for sc_rx_work that already passed o2net_handler_get().
> That drain needs to cover local listener teardown as well, where the
> o2net ordered workqueue may already be inside destroy_workqueue().
>
> Fix the lifetime rule in both directions. Initialize the region delayed
> works before publishing reg through the o2net handler table, keep new or
> stopping regions non-armable with hr_stopping, and quiesce both delayed
> works on failed-start and teardown paths even when no heartbeat thread is
> left to call o2hb_disarm_timeout(). Then unregister handlers before
> tearing down handler-visible region state and make the drain wait for the
> active or destroying o2net ordered workqueue before release frees reg.
>
> The buggy scenario involves two paths, with each column showing the order
> within that path:
>
> region lifecycle: late negotiate callback:
> 1. make_item() registers the 1. o2net_process_message() gets a
> region handlers before heartbeat handler for reg.
> dev_store() has built a 2. The callback runs after the lookup
> runnable heartbeat context. lock is dropped and dereferences reg.
> 2. A failed start or rmdir 3. An approve or timeout path tries to
> stops the heartbeat thread, queue reg's delayed work, or release
> quiesces existing work, and races the callback body after handler
> drops the final configfs ref. unregister.
> 3. region_release() must drain 4. The callback or delayed work can
> handler-visible o2net rx work outlive reg unless lifecycle code
> before freeing reg. keeps the region non-armable and
> drains the active-or-destroying
> o2net workqueue.
>
> Validation reproduced this kernel report:
> KASAN slab-use-after-free in __run_timers+0x22c/0x5b0
> Write of size 8
> Call trace:
> dump_stack_lvl+0x66/0xa0
> print_report+0xce/0x630
> __run_timers+0x22c/0x5b0
> kasan_report+0xe0/0x110
> _raw_spin_unlock_irqrestore+0x27/0x60
> try_to_wake_up+0x191/0xf70
> timer_expire_remote+0xae/0xf0
> run_timer_softirq+0x19b/0x1a0
> handle_softirqs+0x156/0x660
> __irq_exit_rcu+0xc4/0x160
> irq_exit_rcu+0xe/0x20
> sysvec_apic_timer_interrupt+0x6c/0x80
> asm_sysvec_apic_timer_interrupt+0x1a/0x20
>
> Allocated by task stack:
> kasan_save_stack+0x33/0x60
> kasan_save_track+0x14/0x30
> __kasan_kmalloc+0xaa/0xb0
> o2hb_heartbeat_group_make_item+0x3c/0x600
>
> Fixes: 34069b886f95 ("ocfs2: o2hb: add NEGO_TIMEOUT message")
> Fixes: e76f8237a2f7 ("ocfs2: o2hb: add NEGOTIATE_APPROVE message")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <[email protected]>
Looks fine.
Reviewed-by: Joseph Qi <[email protected]>
> ---
> fs/ocfs2/cluster/heartbeat.c | 56 ++++++++++++++++---
> fs/ocfs2/cluster/tcp.c | 103 ++++++++++++++++++++++++++++++-----
> fs/ocfs2/cluster/tcp.h | 1 +
> 3 files changed, 138 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index 76e0c687bcbd..428b8e52f7ff 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -15,6 +15,7 @@
> #include <linux/file.h>
> #include <linux/kthread.h>
> #include <linux/configfs.h>
> +#include <linux/mutex.h>
> #include <linux/random.h>
> #include <linux/crc32.h>
> #include <linux/time.h>
> @@ -258,6 +259,9 @@ struct o2hb_region {
> /* Message key for negotiate timeout message. */
> unsigned int hr_key;
> struct list_head hr_handler_list;
> + /* Serializes timeout arming against failed-start and teardown. */
> + struct mutex hr_arming_mutex;
> + bool hr_stopping;
>
> /* last hb status, 0 for success, other value for error. */
> int hr_last_hb_status;
> @@ -322,9 +326,14 @@ static void o2hb_write_timeout(struct work_struct *work)
>
> static void o2hb_arm_timeout(struct o2hb_region *reg)
> {
> + mutex_lock(®->hr_arming_mutex);
> +
> + if (reg->hr_stopping)
> + goto out_unlock;
> +
> /* Arm writeout only after thread reaches steady state */
> if (atomic_read(®->hr_steady_iterations) != 0)
> - return;
> + goto out_unlock;
>
> mlog(ML_HEARTBEAT, "Queue write timeout for %u ms\n",
> O2HB_MAX_WRITE_TIMEOUT_MS);
> @@ -343,6 +352,18 @@ static void o2hb_arm_timeout(struct o2hb_region *reg)
> schedule_delayed_work(®->hr_nego_timeout_work,
> msecs_to_jiffies(O2HB_NEGO_TIMEOUT_MS));
> bitmap_zero(reg->hr_nego_node_bitmap, O2NM_MAX_NODES);
> +
> +out_unlock:
> + mutex_unlock(®->hr_arming_mutex);
> +}
> +
> +static void o2hb_queue_nego_timeout(struct o2hb_region *reg,
> + unsigned long delay)
> +{
> + mutex_lock(®->hr_arming_mutex);
> + if (!reg->hr_stopping)
> + schedule_delayed_work(®->hr_nego_timeout_work, delay);
> + mutex_unlock(®->hr_arming_mutex);
> }
>
> static void o2hb_disarm_timeout(struct o2hb_region *reg)
> @@ -351,6 +372,19 @@ static void o2hb_disarm_timeout(struct o2hb_region *reg)
> cancel_delayed_work_sync(®->hr_nego_timeout_work);
> }
>
> +static void o2hb_set_region_stopping(struct o2hb_region *reg, bool stopping)
> +{
> + mutex_lock(®->hr_arming_mutex);
> + reg->hr_stopping = stopping;
> + mutex_unlock(®->hr_arming_mutex);
> +}
> +
> +static void o2hb_quiesce_timeout(struct o2hb_region *reg)
> +{
> + o2hb_set_region_stopping(reg, true);
> + o2hb_disarm_timeout(reg);
> +}
> +
> static int o2hb_send_nego_msg(int key, int type, u8 target, u8 node_num)
> {
> struct o2hb_nego_msg msg;
> @@ -400,8 +434,7 @@ static void o2hb_nego_timeout(struct work_struct *work)
> /* check negotiate bitmap every second to do timeout
> * approve decision.
> */
> - schedule_delayed_work(®->hr_nego_timeout_work,
> - msecs_to_jiffies(1000));
> + o2hb_queue_nego_timeout(reg, msecs_to_jiffies(1000));
>
> return;
> }
> @@ -1558,6 +1591,8 @@ static void o2hb_region_release(struct config_item *item)
>
> mlog(ML_HEARTBEAT, "hb region release (%pg)\n", reg_bdev(reg));
>
> + o2hb_quiesce_timeout(reg);
> + o2net_unregister_and_flush_handler_list(®->hr_handler_list);
> o2hb_unmap_slot_data(reg);
>
> if (reg->hr_bdev_file)
> @@ -1573,7 +1608,6 @@ static void o2hb_region_release(struct config_item *item)
> list_del(®->hr_all_item);
> spin_unlock(&o2hb_live_lock);
>
> - o2net_unregister_handler_list(®->hr_handler_list);
> kfree(reg);
> }
>
> @@ -1888,9 +1922,6 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
> goto out;
> }
>
> - INIT_DELAYED_WORK(®->hr_write_timeout_work, o2hb_write_timeout);
> - INIT_DELAYED_WORK(®->hr_nego_timeout_work, o2hb_nego_timeout);
> -
> /*
> * A node is considered live after it has beat LIVE_THRESHOLD
> * times. We're not steady until we've given them a chance
> @@ -1910,6 +1941,7 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
> atomic_set(®->hr_steady_iterations, live_threshold);
> /* unsteady_iterations is triple the steady_iterations */
> atomic_set(®->hr_unsteady_iterations, (live_threshold * 3));
> + o2hb_set_region_stopping(reg, false);
>
> hb_task = kthread_run(o2hb_thread, reg, "o2hb-%s",
> reg->hr_item.ci_name);
> @@ -1959,6 +1991,8 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>
> out:
> if (ret < 0) {
> + o2hb_quiesce_timeout(reg);
> +
> spin_lock(&o2hb_live_lock);
> hb_task = reg->hr_task;
> reg->hr_task = NULL;
> @@ -2098,6 +2132,10 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
> */
> reg->hr_key = crc32_le(reg->hr_region_num + O2NM_MAX_REGIONS,
> name, strlen(name));
> + mutex_init(®->hr_arming_mutex);
> + reg->hr_stopping = true;
> + INIT_DELAYED_WORK(®->hr_write_timeout_work, o2hb_write_timeout);
> + INIT_DELAYED_WORK(®->hr_nego_timeout_work, o2hb_nego_timeout);
> INIT_LIST_HEAD(®->hr_handler_list);
> ret = o2net_register_handler(O2HB_NEGO_TIMEOUT_MSG, reg->hr_key,
> sizeof(struct o2hb_nego_msg),
> @@ -2118,7 +2156,7 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
> return ®->hr_item;
>
> unregister_handler:
> - o2net_unregister_handler_list(®->hr_handler_list);
> + o2net_unregister_and_flush_handler_list(®->hr_handler_list);
> remove_item:
> spin_lock(&o2hb_live_lock);
> list_del(®->hr_all_item);
> @@ -2137,6 +2175,8 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
> struct o2hb_region *reg = to_o2hb_region(item);
> int quorum_region = 0;
>
> + o2hb_quiesce_timeout(reg);
> +
> /* stop the thread when the user removes the region dir */
> spin_lock(&o2hb_live_lock);
> hb_task = reg->hr_task;
> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
> index e62c1ef8223b..474fe1414cee 100644
> --- a/fs/ocfs2/cluster/tcp.c
> +++ b/fs/ocfs2/cluster/tcp.c
> @@ -38,6 +38,8 @@
> */
>
> #include <linux/kernel.h>
> +#include <linux/completion.h>
> +#include <linux/mutex.h>
> #include <linux/sched/mm.h>
> #include <linux/jiffies.h>
> #include <linux/slab.h>
> @@ -102,9 +104,14 @@ static struct socket *o2net_listen_sock;
> * quorum work is queued as sock containers are shutdown.. stop_listening
> * tears down all the node's sock containers, preventing future shutdowns
> * and queued quorum work, before canceling delayed quorum work and
> - * destroying the work queue.
> + * destroying the work queue. Handler teardown can also race local listener
> + * shutdown, so keep a waitable destroying pointer until the old ordered
> + * queue has finished draining.
> */
> static struct workqueue_struct *o2net_wq;
> +static struct workqueue_struct *o2net_wq_destroying;
> +static DEFINE_MUTEX(o2net_wq_mutex);
> +static DECLARE_COMPLETION(o2net_wq_destroyed);
> /* Heartbeat callbacks stay registered across local-node off/on. */
> static bool o2net_listening;
> static struct work_struct o2net_listen_work;
> @@ -886,6 +893,27 @@ void o2net_unregister_handler_list(struct list_head *list)
> }
> EXPORT_SYMBOL_GPL(o2net_unregister_handler_list);
>
> +static void o2net_flush_wq(void)
> +{
> + mutex_lock(&o2net_wq_mutex);
> + if (o2net_wq_destroying) {
> + mutex_unlock(&o2net_wq_mutex);
> + wait_for_completion(&o2net_wq_destroyed);
> + return;
> + }
> +
> + if (o2net_wq)
> + flush_workqueue(o2net_wq);
> + mutex_unlock(&o2net_wq_mutex);
> +}
> +
> +void o2net_unregister_and_flush_handler_list(struct list_head *list)
> +{
> + o2net_unregister_handler_list(list);
> + o2net_flush_wq();
> +}
> +EXPORT_SYMBOL_GPL(o2net_unregister_and_flush_handler_list);
> +
> static struct o2net_msg_handler *o2net_handler_get(u32 msg_type, u32 key)
> {
> struct o2net_msg_handler *nmh;
> @@ -1717,12 +1745,10 @@ void o2net_disconnect_node(struct o2nm_node *node)
> o2net_set_nn_state(nn, NULL, 0, -ENOTCONN);
> spin_unlock(&nn->nn_lock);
>
> - if (o2net_wq) {
> - cancel_delayed_work(&nn->nn_connect_expired);
> - cancel_delayed_work(&nn->nn_connect_work);
> - cancel_delayed_work(&nn->nn_still_up);
> - flush_workqueue(o2net_wq);
> - }
> + cancel_delayed_work(&nn->nn_connect_expired);
> + cancel_delayed_work(&nn->nn_connect_work);
> + cancel_delayed_work(&nn->nn_still_up);
> + o2net_flush_wq();
> }
>
> static void o2net_hb_node_down_cb(struct o2nm_node *node, int node_num,
> @@ -2067,6 +2093,36 @@ static int o2net_open_listening_sock(__be32 addr, __be16 port)
> return ret;
> }
>
> +static void o2net_destroy_wq(void)
> +{
> + struct workqueue_struct *wq;
> +
> + mutex_lock(&o2net_wq_mutex);
> + if (o2net_wq_destroying) {
> + mutex_unlock(&o2net_wq_mutex);
> + wait_for_completion(&o2net_wq_destroyed);
> + return;
> + }
> +
> + wq = o2net_wq;
> + if (!wq) {
> + mutex_unlock(&o2net_wq_mutex);
> + return;
> + }
> +
> + reinit_completion(&o2net_wq_destroyed);
> + o2net_wq_destroying = wq;
> + mutex_unlock(&o2net_wq_mutex);
> +
> + destroy_workqueue(wq);
> +
> + mutex_lock(&o2net_wq_mutex);
> + o2net_wq = NULL;
> + o2net_wq_destroying = NULL;
> + complete_all(&o2net_wq_destroyed);
> + mutex_unlock(&o2net_wq_mutex);
> +}
> +
> /*
> * called from node manager when we should bring up our network listening
> * socket. node manager handles all the serialization to only call this
> @@ -2077,24 +2133,44 @@ static int o2net_open_listening_sock(__be32 addr, __be16 port)
> int o2net_start_listening(struct o2nm_node *node)
> {
> int ret = 0;
> + struct workqueue_struct *wq;
>
> if (WARN_ON_ONCE(READ_ONCE(o2net_listening)))
> return -EBUSY;
> - BUG_ON(o2net_wq != NULL);
> +
> + mutex_lock(&o2net_wq_mutex);
> + if (o2net_wq_destroying) {
> + mutex_unlock(&o2net_wq_mutex);
> + return -EBUSY;
> + }
> + if (WARN_ON_ONCE(o2net_wq)) {
> + mutex_unlock(&o2net_wq_mutex);
> + return -EBUSY;
> + }
> + mutex_unlock(&o2net_wq_mutex);
> +
> BUG_ON(o2net_listen_sock != NULL);
>
> mlog(ML_KTHREAD, "starting o2net thread...\n");
> - o2net_wq = alloc_ordered_workqueue("o2net", WQ_MEM_RECLAIM);
> - if (o2net_wq == NULL) {
> + wq = alloc_ordered_workqueue("o2net", WQ_MEM_RECLAIM);
> + if (!wq) {
> mlog(ML_ERROR, "unable to launch o2net thread\n");
> return -ENOMEM; /* ? */
> }
>
> + mutex_lock(&o2net_wq_mutex);
> + if (unlikely(o2net_wq_destroying || o2net_wq)) {
> + mutex_unlock(&o2net_wq_mutex);
> + destroy_workqueue(wq);
> + return -EBUSY;
> + }
> + o2net_wq = wq;
> + mutex_unlock(&o2net_wq_mutex);
> +
> ret = o2net_open_listening_sock(node->nd_ipv4_address,
> node->nd_ipv4_port);
> if (ret) {
> - destroy_workqueue(o2net_wq);
> - o2net_wq = NULL;
> + o2net_destroy_wq();
> } else
> o2quo_conn_up(node->nd_num);
>
> @@ -2130,8 +2206,7 @@ void o2net_stop_listening(struct o2nm_node *node)
>
> /* finish all work and tear down the work queue */
> mlog(ML_KTHREAD, "waiting for o2net thread to exit....\n");
> - destroy_workqueue(o2net_wq);
> - o2net_wq = NULL;
> + o2net_destroy_wq();
>
> sock_release(o2net_listen_sock);
> o2net_listen_sock = NULL;
> diff --git a/fs/ocfs2/cluster/tcp.h b/fs/ocfs2/cluster/tcp.h
> index 2e86d42b5faf..a11bcee28947 100644
> --- a/fs/ocfs2/cluster/tcp.h
> +++ b/fs/ocfs2/cluster/tcp.h
> @@ -89,6 +89,7 @@ int o2net_register_handler(u32 msg_type, u32 key, u32 max_len,
> o2net_post_msg_handler_func *post_func,
> struct list_head *unreg_list);
> void o2net_unregister_handler_list(struct list_head *list);
> +void o2net_unregister_and_flush_handler_list(struct list_head *list);
>
> void o2net_fill_node_map(unsigned long *map, unsigned bytes);
>