[PATCH] ocfs2/cluster: hold a reference on the heartbeat thread

[PATCH] ocfs2/cluster: hold a reference on the heartbeat thread

[Karl Mehltretter]

Since commit 688bc88e2046 ("ocfs2/cluster: keep heartbeat local node
stable"), o2hb_thread() leaves its loop and returns when the local node
changes, for example after "echo 0 > node/<name>/local". The thread
was started with kthread_run() and nothing holds a reference to its
task_struct, so the task is freed once it exits, while reg->hr_task
still points to it.

Reading the region's pid attribute then reads the freed task, and
removing the region calls kthread_stop() on it:

  BUG: KASAN: slab-use-after-free in o2hb_region_pid_show+0xb3/0xc0
  refcount_t: addition on 0; use-after-free.
  Oops: Oops: 0000 [#1] SMP KASAN NOPTI
  RIP: 0010:kthread_stop+0xb1/0x390

The thread could already return by itself before, when heartbeat start
was aborted or on an unclean stop, but the local node change makes it
reachable from userspace at any time.

Create the thread parked, take a reference on it and only then wake it,
so the reference cannot race with the thread exiting. Drop it with
kthread_stop_put().

Fixes: 688bc88e2046 ("ocfs2/cluster: keep heartbeat local node stable")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260616074931.3774929-1-zzzccc427%40gmail.com
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
Reproduced under QEMU x86_64 with KASAN, using one node over configfs
and a loop device as the heartbeat region:

  echo 0 > cluster/c1/node/n0/local     # heartbeat thread returns
  cat cluster/c1/heartbeat/<uuid>/pid   # reads the freed task_struct
  rmdir cluster/c1/heartbeat/<uuid>     # kthread_stop() on it

With the patch, that sequence, a normal region removal and a start
interrupted by a signal ran clean on two CPUs, five rounds each, and
every heartbeat task_struct was freed.

 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 1c3def99bb076..a4c8ea695f5cf 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -1966,18 +1966,22 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
 	atomic_set(&reg->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);
+	hb_task = kthread_create(o2hb_thread, reg, "o2hb-%s",
+				 reg->hr_item.ci_name);
 	if (IS_ERR(hb_task)) {
 		ret = PTR_ERR(hb_task);
 		mlog_errno(ret);
 		goto out;
 	}
+	/* The thread may exit on its own, so pin it before it can run. */
+	get_task_struct(hb_task);
 
 	spin_lock(&o2hb_live_lock);
 	reg->hr_task = hb_task;
 	spin_unlock(&o2hb_live_lock);
 
+	wake_up_process(hb_task);
+
 	ret = wait_event_interruptible(o2hb_steady_queue,
 				atomic_read(&reg->hr_steady_iterations) == 0 ||
 				reg->hr_node_deleted);
@@ -2022,7 +2026,7 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
 		spin_unlock(&o2hb_live_lock);
 
 		if (hb_task)
-			kthread_stop(hb_task);
+			kthread_stop_put(hb_task);
 
 		o2hb_unmap_slot_data(reg);
 
@@ -2208,7 +2212,7 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
 	spin_unlock(&o2hb_live_lock);
 
 	if (hb_task)
-		kthread_stop(hb_task);
+		kthread_stop_put(hb_task);
 
 	if (o2hb_global_heartbeat_active()) {
 		spin_lock(&o2hb_live_lock);
-- 
2.53.0

Re: [PATCH] ocfs2/cluster: hold a reference on the heartbeat thread

[Joseph Qi]

On 9/23/26 9:01 AM, Karl Mehltretter wrote:
> Since commit 688bc88e2046 ("ocfs2/cluster: keep heartbeat local node
> stable"), o2hb_thread() leaves its loop and returns when the local node
> changes, for example after "echo 0 > node/<name>/local". The thread
> was started with kthread_run() and nothing holds a reference to its
> task_struct, so the task is freed once it exits, while reg->hr_task
> still points to it.
> 
> Reading the region's pid attribute then reads the freed task, and
> removing the region calls kthread_stop() on it:
> 
>   BUG: KASAN: slab-use-after-free in o2hb_region_pid_show+0xb3/0xc0
>   refcount_t: addition on 0; use-after-free.
>   Oops: Oops: 0000 [#1] SMP KASAN NOPTI
>   RIP: 0010:kthread_stop+0xb1/0x390
> 
> The thread could already return by itself before, when heartbeat start
> was aborted or on an unclean stop, but the local node change makes it
> reachable from userspace at any time.
> 
> Create the thread parked, take a reference on it and only then wake it,

Just a wording nit. kthread_create() doesn't leave the thread parked in
the kthread_park() sense. __kthread_parkme() only runs after the first
wake_up_process(), so "parked" here may be a little confused.

> so the reference cannot race with the thread exiting. Drop it with
> kthread_stop_put().
> 
> Fixes: 688bc88e2046 ("ocfs2/cluster: keep heartbeat local node stable")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260616074931.3774929-1-zzzccc427%40gmail.com
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>

The change looks fine to me.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

> ---
> Reproduced under QEMU x86_64 with KASAN, using one node over configfs
> and a loop device as the heartbeat region:
> 
>   echo 0 > cluster/c1/node/n0/local     # heartbeat thread returns
>   cat cluster/c1/heartbeat/<uuid>/pid   # reads the freed task_struct
>   rmdir cluster/c1/heartbeat/<uuid>     # kthread_stop() on it
> 
> With the patch, that sequence, a normal region removal and a start
> interrupted by a signal ran clean on two CPUs, five rounds each, and
> every heartbeat task_struct was freed.
> 
>  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 1c3def99bb076..a4c8ea695f5cf 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -1966,18 +1966,22 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  	atomic_set(&reg->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);
> +	hb_task = kthread_create(o2hb_thread, reg, "o2hb-%s",
> +				 reg->hr_item.ci_name);
>  	if (IS_ERR(hb_task)) {
>  		ret = PTR_ERR(hb_task);
>  		mlog_errno(ret);
>  		goto out;
>  	}
> +	/* The thread may exit on its own, so pin it before it can run. */
> +	get_task_struct(hb_task);
>  
>  	spin_lock(&o2hb_live_lock);
>  	reg->hr_task = hb_task;
>  	spin_unlock(&o2hb_live_lock);
>  
> +	wake_up_process(hb_task);
> +
>  	ret = wait_event_interruptible(o2hb_steady_queue,
>  				atomic_read(&reg->hr_steady_iterations) == 0 ||
>  				reg->hr_node_deleted);
> @@ -2022,7 +2026,7 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  		spin_unlock(&o2hb_live_lock);
>  
>  		if (hb_task)
> -			kthread_stop(hb_task);
> +			kthread_stop_put(hb_task);
>  
>  		o2hb_unmap_slot_data(reg);
>  
> @@ -2208,7 +2212,7 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
>  	spin_unlock(&o2hb_live_lock);
>  
>  	if (hb_task)
> -		kthread_stop(hb_task);
> +		kthread_stop_put(hb_task);
>  
>  	if (o2hb_global_heartbeat_active()) {
>  		spin_lock(&o2hb_live_lock);