Re: [PATCH] ocfs2/cluster: Initialize heartbeat regions before publishing them
From: Joseph Qi
Date: Mon Oct 19 2026 - 23:31:59 EST
On 10/16/26 12:58 AM, Cen Zhang wrote:
> A region on o2hb_all_regions must have an initialized config item and
> name. o2hb_heartbeat_group_make_item() currently links the zeroed region
> under o2hb_live_lock, drops the lock, and only then initializes hr_item.
> The lock therefore protects the list without protecting initialization
> from readers of the published item.
>
> With global heartbeat enabled, a DLM_QUERY_REGION from the current
> joining node can pass the existing-domain and protocol checks while a
> configfs mkdir creates another region. The DLM locks are not taken by
> the constructor, and the network worker does not take the configfs
> subsystem mutex. The following ordering is possible:
>
> Configfs mkdir O2net receive worker
> -------------- --------------------
> Allocate reg
> Lock o2hb_live_lock
> Link into o2hb_all_regions
> Unlock o2hb_live_lock
> dlm_query_region_handler()
> dlm_match_regions()
> o2hb_get_all_regions()
> Lock o2hb_live_lock
> Copy 32 bytes from NULL name
> Unlock o2hb_live_lock
> config_item_init_type_name()
>
> The query thus dereferences NULL. Initialize hr_item before linking the
> region so the existing list lock also orders name initialization before
> any reader. Reject a failed name allocation, since the initialization
> helper does not propagate that error.
>
> Use config_item_put() on initialized constructor failures to release the
> name and respect references retained by o2hb_region_pin(). Initialize
> the list heads, mutex and delayed works needed by o2hb_region_release()
> before any such put, and leave the list node self-linked after rollback.
> Keep the existing handler drain before removing a failed published item.
>
> KASAN report as below:
>
> ==================================================================
> BUG: KASAN: null-ptr-deref in o2hb_get_all_regions+0xbb/0x1f0
> Read of size 32 at addr 0000000000000000 by task kworker/u16:1/14
>
> CPU: 2 UID: 0 PID: 14 Comm: kworker/u16:1 Not tainted 7.3.0-rc4-next-20260921-pmb-ocfs2-functional-v1+ #1 PREEMPT(lazy)
> Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> Workqueue: o2net o2net_rx_until_empty
> Call Trace:
> <TASK>
> dump_stack_lvl+0x93/0xd0
> kasan_report+0xe0/0x110
> ? o2hb_get_all_regions+0xbb/0x1f0
> kasan_check_range+0x105/0x1b0
> __asan_memcpy+0x23/0x60
> o2hb_get_all_regions+0xbb/0x1f0
> dlm_query_region_handler+0x4c2/0xc20
> ? __pfx_dlm_query_region_handler+0x10/0x10
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? trace_hardirqs_on+0x18/0x160
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? kvm_clock_get_cycles+0x31/0x60
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __pfx_dlm_query_region_handler+0x10/0x10
> o2net_rx_until_empty+0x1a50/0x3200
> ? reacquire_held_locks+0xdd/0x200
> ? __pfx_o2net_rx_until_empty+0x10/0x10
> ? lock_acquire+0x190/0x300
> ? process_one_work+0x935/0x1b40
> ? process_one_work+0x834/0x1b40
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? lock_release+0xc8/0x290
> ? srso_alias_return_thunk+0x5/0xfbef5
> process_one_work+0x9a8/0x1b40
> ? __pfx_process_one_work+0x10/0x10
> ? lock_acquire+0x190/0x300
> ? lock_is_held_type+0x8f/0x100
> ? srso_alias_return_thunk+0x5/0xfbef5
> worker_thread+0x65c/0xe40
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __kthread_parkme+0x177/0x220
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __pfx_worker_thread+0x10/0x10
> kthread+0x351/0x460
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __pfx_kthread+0x10/0x10
> ret_from_fork+0x659/0x940
> ? __pfx_ret_from_fork+0x10/0x10
> ? srso_alias_return_thunk+0x5/0xfbef5
> ? __switch_to+0x74f/0xf70
> ? __pfx_kthread+0x10/0x10
> ret_from_fork_asm+0x1a/0x30
> </TASK>
> ==================================================================
>
> Fixes: 536f0741f324 ("ocfs2/cluster: Track number of global heartbeat regions")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Cen Zhang <zzzccc427@xxxxxxxxx>
Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>
> Assisted-by: LLM
>
> ---
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index 8e6ead433228..9a4703bdc4e8 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -2140,6 +2140,18 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
> }
> reg->hr_node_num = O2NM_MAX_NODES;
>
> + INIT_LIST_HEAD(®->hr_all_item);
> + 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);
> + config_item_init_type_name(®->hr_item, name, &o2hb_region_type);
> + if (!config_item_name(®->hr_item)) {
> + ret = -ENOMEM;
> + goto put_item;
> + }
> +
> spin_lock(&o2hb_live_lock);
> reg->hr_region_num = 0;
> if (o2hb_global_heartbeat_active()) {
> @@ -2148,26 +2160,19 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
> if (reg->hr_region_num >= O2NM_MAX_REGIONS) {
> spin_unlock(&o2hb_live_lock);
> ret = -EFBIG;
> - goto free;
> + goto put_item;
> }
> set_bit(reg->hr_region_num, o2hb_region_bitmap);
> }
> list_add_tail(®->hr_all_item, &o2hb_all_regions);
> spin_unlock(&o2hb_live_lock);
>
> - config_item_init_type_name(®->hr_item, name, &o2hb_region_type);
> -
> /* this is the same way to generate msg key as dlm, for local heartbeat,
> * name is also the same, so make initial crc value different to avoid
> * message key conflict.
> */
> 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),
> o2hb_nego_timeout_handler,
> @@ -2190,10 +2195,13 @@ unregister_handler:
> o2net_unregister_and_flush_handler_list(®->hr_handler_list);
> remove_item:
> spin_lock(&o2hb_live_lock);
> - list_del(®->hr_all_item);
> + list_del_init(®->hr_all_item);
> if (o2hb_global_heartbeat_active())
> clear_bit(reg->hr_region_num, o2hb_region_bitmap);
> spin_unlock(&o2hb_live_lock);
> +put_item:
> + config_item_put(®->hr_item);
> + return ERR_PTR(ret);
> free:
> kfree(reg);
> return ERR_PTR(ret);