Re: [PATCH v2] ocfs2/heartbeat: Unlink live slots when their node is missing

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 10/9/26 1:21 PM, Cen Zhang wrote:
> o2hb_shutdown_slot() returns immediately when its node is no longer
> in the configfs registry. A slot can still be linked in o2hb_live_slots
> when that lookup fails. Region teardown or startup-error cleanup then
> frees the slot array without removing its embedded list member.
> 
> With the same peer live in another region, the next list update can
> follow a predecessor into the freed array. Reusing the node number can
> also make a later insertion encounter that stale member.
> 
> Always remove the slot under o2hb_live_lock, even when node lookup
> fails. Preserve aggregate membership accounting and the last-region
> DOWN callback: the existing callback interface explicitly permits a
> NULL node for DOWN events. Only drop the node reference when lookup
> returned one. This closes both normal release and startup-abort paths
> without changing the publication or callback locking protocol.
> 
> Also require a configured node before admitting a new live slot in
> o2hb_check_slot(). The early live-bitmap check is not held through
> admission: the last live slot in another region can be removed in
> between. Without a node, admission can then queue an UP event with
> a NULL argument and hit the event helper's BUG_ON. Existing live
> slots can still leave and generate NULL-node DOWN events.
> 
> A controlled probe-instrumented test reproduced this while stopping
> one of two regions after removing the peer from configfs. Its KASAN
> report includes:
> 
>   BUG: KASAN: slab-use-after-free in __list_del_entry_valid_or_report+0x1e0/0x210
>   Read of size 8 at addr ffff888106a4ac60 by task o2hb-pmbd0058_b/497
>   Call Trace:
>    <TASK>
>    dump_stack_lvl+0x93/0xd0
>    print_report+0xce/0x630
>    kasan_report+0xe0/0x110
>    __list_del_entry_valid_or_report+0x1e0/0x210
>    o2hb_do_disk_heartbeat+0x1591/0x2c50
>    o2hb_thread+0x1ed/0xe70
>    kthread+0x351/0x460
>    ret_from_fork+0x659/0x940
>    ret_from_fork_asm+0x1a/0x30
>    </TASK>
>   Allocated by task 491:
>    kasan_save_stack+0x33/0x60
>    kasan_save_track+0x14/0x30
>    __kasan_kmalloc+0xaa/0xb0
>    __kmalloc_noprof+0x2de/0x780
>    o2hb_region_dev_store+0x7c2/0x1b60
>    configfs_write_iter+0x2f9/0x4f0
>    vfs_write+0x639/0x1010
>    ksys_write+0x111/0x200
>    do_syscall_64+0x114/0x620
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
>   Freed by task 491:
>    kasan_save_stack+0x33/0x60
>    kasan_save_track+0x14/0x30
>    kasan_save_free_info+0x3b/0x60
>    __kasan_slab_free+0x5f/0x80
>    kfree+0x308/0x580
>    o2hb_unmap_slot_data+0x1db/0x330
>    o2hb_region_release+0x14b/0x4f0
>    config_item_cleanup+0x14b/0x230
>    config_item_put+0x79/0x90
>    configfs_rmdir+0x58a/0x910
>    vfs_rmdir+0x2e8/0x820
>    filename_rmdir+0x3b1/0x520
>    __x64_sys_rmdir+0x4b/0x70
>    do_syscall_64+0x114/0x620
>    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Fixes: a7f6a5fb4bde ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>

Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

> ---
> Changes in v2:
> - Require a configured node before admitting a live slot in
>   o2hb_check_slot().
> 
> Link to v1: https://lore.kernel.org/r/pm-ocfs2-objects-candidate-0058-v1-177573785904d0ea43c7@gmail.com
> 
>  fs/ocfs2/cluster/heartbeat.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index 1c3def99bb0765deb828ad6415fde31ed9316002..3f3f6fc6b1316e7c996681fc50fde84d183d117d 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -880,8 +880,11 @@ static void o2hb_shutdown_slot(struct o2hb_disk_slot *slot)
>  	int queued = 0;
>  
>  	node = o2nm_get_node_by_num(slot->ds_node_num);
> -	if (!node)
> -		return;
> +	/*
> +	 * The node may have been removed from the node table while this
> +	 * region's slot is still linked.  The list member must still be
> +	 * removed before the region releases its slot array.
> +	 */
>  
>  	spin_lock(&o2hb_live_lock);
>  	if (!list_empty(&slot->ds_live_item)) {
> @@ -903,7 +906,8 @@ static void o2hb_shutdown_slot(struct o2hb_disk_slot *slot)
>  	if (queued)
>  		o2hb_run_event_list(&event);
>  
> -	o2nm_node_put(node);
> +	if (node)
> +		o2nm_node_put(node);
>  }
>  
>  static void o2hb_set_quorum_device(struct o2hb_region *reg)
> @@ -1038,7 +1042,7 @@ static int o2hb_check_slot(struct o2hb_region *reg,
>  fire_callbacks:
>  	/* dead nodes only come to life after some number of
>  	 * changes at any time during their dead time */
> -	if (list_empty(&slot->ds_live_item) &&
> +	if (node && list_empty(&slot->ds_live_item) &&
>  	    slot->ds_changed_samples >= O2HB_LIVE_THRESHOLD) {
>  		mlog(ML_HEARTBEAT, "Node %d (id 0x%llx) joined my region\n",
>  		     slot->ds_node_num, (long long)slot->ds_last_generation);