[PATCH v2 0/3] ocfs2: cluster: o2hb_region_pin() fixes

[PATCH v2 0/3] ocfs2: cluster: o2hb_region_pin() fixes

[Joseph Qi]

This series fixes three related issues in o2hb_region_pin(), all are
from the original implementation in commit:
58a3158a5d17 ("ocfs2/cluster: Pin/unpin o2hb regions"):

1) It is called with o2hb_live_lock (a spinlock) held, but the
   underlying configfs_depend_item() sleeps (takes inode rwsem and
   pins the filesystem).  This triggers BUG under
   CONFIG_DEBUG_ATOMIC_SLEEP.

2) When called from the configfs drop_item callback, it creates a
   lock order inversion: parent inode_lock -> configfs root
   inode_lock, which can deadlock against subsystem unregistration
   paths taking root -> parent.

3) If pinning fails partway through o2hb_region_inc_user(), the
   o2hb_dependent_users counter is leaked and partially-pinned
   regions are never released, leaving heartbeat regions
   unprotected on subsequent mounts.

Patch 1 reworks o2hb_region_pin() to drop o2hb_live_lock across
each sleeping configfs_depend_item() call, using a config_item
reference to keep the region alive while unlocked.

Patch 2 adds a from_callback parameter to select
configfs_depend_item_unlocked() when called from configfs context,
avoiding the inode_lock nesting.

Patch 3 fixes the error path in o2hb_region_inc_user() to unpin
and decrement the counter on failure.

Joseph Qi (3):
  ocfs2: cluster: don't sleep while holding o2hb_live_lock in
    o2hb_region_pin()
  ocfs2: cluster: avoid lock order inversion in o2hb_region_pin() from
    drop_item
  ocfs2: cluster: fix o2hb_dependent_users leak on pin failure

 fs/ocfs2/cluster/heartbeat.c   | 146 ++++++++++++++++++++++++++-------
 fs/ocfs2/cluster/nodemanager.c |   6 ++
 fs/ocfs2/cluster/nodemanager.h |   1 +
 3 files changed, 122 insertions(+), 31 deletions(-)

-- 
2.39.3

[PATCH v2 1/3] ocfs2: cluster: don't sleep while holding o2hb_live_lock in o2hb_region_pin()

[Joseph Qi]

o2hb_region_pin() is always called with the o2hb_live_lock spinlock held
(from o2hb_region_inc_user() and o2hb_heartbeat_group_drop_item()), but it
calls o2nm_depend_item() -> configfs_depend_item(), which sleeps: it pins
the configfs filesystem and takes the configfs root inode rwsem. Under
CONFIG_DEBUG_ATOMIC_SLEEP this triggers:

  BUG: sleeping function called from invalid context at kernel/locking/rwsem.c
  in_atomic(): 1, ... name: mount.ocfs2
    down_write
    configfs_depend_item
    o2hb_region_pin
    o2hb_region_inc_user
    o2hb_register_callback
    dlm_register_domain_handlers
    ...
    ocfs2_dlm_init
    ocfs2_mount_volume
    ocfs2_fill_super

Rework o2hb_region_pin() to pin one region at a time with the lock
dropped across the sleeping call: under o2hb_live_lock find the next
eligible region and take a config_item reference to keep it alive, drop
the lock, call o2nm_depend_item(), then retake the lock and record the
pin. The config_item_put() is done with the lock released as well, since
o2hb_region_release() also acquires o2hb_live_lock and can sleep. The
region list may change while unlocked, so the scan restarts from the
top after each pin. Local heartbeat still pins only the matching region;
global heartbeat pins all eligible regions.

The unpin path is unaffected: configfs_undepend_item() only takes a
spinlock and does not sleep.

Fixes: 58a3158a5d17 ("ocfs2/cluster: Pin/unpin o2hb regions")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/cluster/heartbeat.c | 126 ++++++++++++++++++++++++++++-------
 1 file changed, 101 insertions(+), 25 deletions(-)

diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
index 29542edbc992c..5ca1d9c0c6575 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -43,6 +43,14 @@ static DECLARE_RWSEM(o2hb_callback_sem);
  * whenever any of the threads sees activity from the node in its region.
  */
 static DEFINE_SPINLOCK(o2hb_live_lock);
+/*
+ * Serializes region pin/unpin dependency management (o2hb_dependent_users
+ * and the o2nm_depend_item()/o2nm_undepend_item() calls). o2hb_region_pin()
+ * has to drop o2hb_live_lock across the sleeping o2nm_depend_item(), so the
+ * spinlock alone can no longer keep pin and unpin mutually exclusive; this
+ * mutex, taken outside o2hb_live_lock, does.
+ */
+static DEFINE_MUTEX(o2hb_dependency_mutex);
 static struct list_head o2hb_live_slots[O2NM_MAX_NODES];
 static unsigned long o2hb_live_node_bitmap[BITS_TO_LONGS(O2NM_MAX_NODES)];
 static LIST_HEAD(o2hb_node_events);
@@ -2172,6 +2180,7 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
 	 * If global heartbeat active and there are dependent users,
 	 * pin all regions if quorum region count <= CUT_OFF
 	 */
+	mutex_lock(&o2hb_dependency_mutex);
 	spin_lock(&o2hb_live_lock);
 
 	if (!o2hb_dependent_users)
@@ -2183,6 +2192,7 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
 
 unlock:
 	spin_unlock(&o2hb_live_lock);
+	mutex_unlock(&o2hb_dependency_mutex);
 }
 
 static ssize_t o2hb_heartbeat_group_dead_threshold_show(struct config_item *item,
@@ -2322,46 +2332,108 @@ EXPORT_SYMBOL_GPL(o2hb_setup_callback);
  */
 static int o2hb_region_pin(const char *region_uuid)
 {
-	int ret = 0, found = 0;
-	struct o2hb_region *reg;
+	int ret = 0, found;
+	struct o2hb_region *reg, *pinned;
 	char *uuid;
 
 	assert_spin_locked(&o2hb_live_lock);
 
-	list_for_each_entry(reg, &o2hb_all_regions, hr_all_item) {
-		if (reg->hr_item_dropped)
-			continue;
+	do {
+		found = 0;
+		pinned = NULL;
 
-		uuid = config_item_name(&reg->hr_item);
+		list_for_each_entry(reg, &o2hb_all_regions, hr_all_item) {
+			if (reg->hr_item_dropped)
+				continue;
 
-		/* local heartbeat */
-		if (region_uuid) {
-			if (strcmp(region_uuid, uuid))
+			uuid = config_item_name(&reg->hr_item);
+
+			/* local heartbeat */
+			if (region_uuid) {
+				if (strcmp(region_uuid, uuid))
+					continue;
+				found = 1;
+			}
+
+			if (reg->hr_item_pinned || reg->hr_item_dropped) {
+				if (found)
+					break;
 				continue;
-			found = 1;
+			}
+
+			/*
+			 * Found a region that needs pinning. Take a reference
+			 * so it stays alive while we drop the lock below.
+			 */
+			pinned = reg;
+			config_item_get(&reg->hr_item);
+			break;
 		}
 
-		if (reg->hr_item_pinned || reg->hr_item_dropped)
-			goto skip_pin;
+		if (!pinned)
+			break;
+
+		uuid = config_item_name(&pinned->hr_item);
+
+		/*
+		 * o2nm_depend_item() -> configfs_depend_item() can sleep (it
+		 * takes the configfs root inode rwsem), so it must not run
+		 * under o2hb_live_lock. Drop the lock across it; @pinned is
+		 * kept alive by the reference taken above. The region list may
+		 * change while unlocked, so we rescan from the top afterwards.
+		 */
+		spin_unlock(&o2hb_live_lock);
 
 		/* Ignore ENOENT only for local hb (userdlm domain) */
-		ret = o2nm_depend_item(&reg->hr_item);
+		ret = o2nm_depend_item(&pinned->hr_item);
+
+		spin_lock(&o2hb_live_lock);
 		if (!ret) {
-			mlog(ML_CLUSTER, "Pin region %s\n", uuid);
-			reg->hr_item_pinned = 1;
-		} else {
-			if (ret == -ENOENT && found)
-				ret = 0;
-			else {
-				mlog(ML_ERROR, "Pin region %s fails with %d\n",
-				     uuid, ret);
+			/*
+			 * o2hb_live_lock was dropped across o2nm_depend_item().
+			 * o2hb_set_quorum_device() runs in the heartbeat thread
+			 * without o2hb_dependency_mutex, so for global heartbeat
+			 * it may have crossed O2HB_PIN_CUT_OFF and unpinned the
+			 * regions while we slept. If that happened this pin is
+			 * no longer wanted; undo it and stop rather than
+			 * resurrecting it on the rescan below.
+			 */
+			if (!region_uuid &&
+			    bitmap_weight(o2hb_quorum_region_bitmap,
+					  O2NM_MAX_REGIONS) > O2HB_PIN_CUT_OFF) {
+				o2nm_undepend_item(&pinned->hr_item);
+				spin_unlock(&o2hb_live_lock);
+				config_item_put(&pinned->hr_item);
+				spin_lock(&o2hb_live_lock);
 				break;
 			}
+			mlog(ML_CLUSTER, "Pin region %s\n", uuid);
+			pinned->hr_item_pinned = 1;
+		} else if (ret == -ENOENT && (found || !region_uuid)) {
+			/*
+			 * For local hb (found): ignore ENOENT from userdlm
+			 * domains as before.  For global hb (!region_uuid):
+			 * the region may have been detached from configfs
+			 * while the lock was dropped — skip it and continue
+			 * pinning the remaining regions.
+			 */
+			ret = 0;
+		} else {
+			mlog(ML_ERROR, "Pin region %s fails with %d\n",
+			     uuid, ret);
 		}
-skip_pin:
-		if (found)
-			break;
-	}
+
+		/*
+		 * config_item_put() may drop the last reference and run
+		 * o2hb_region_release(), which also grabs o2hb_live_lock and
+		 * can sleep, so it must happen with the lock released.
+		 */
+		spin_unlock(&o2hb_live_lock);
+		config_item_put(&pinned->hr_item);
+		spin_lock(&o2hb_live_lock);
+
+		/* local hb pins a single matching region */
+	} while (!ret && !region_uuid);
 
 	return ret;
 }
@@ -2406,6 +2478,7 @@ static int o2hb_region_inc_user(const char *region_uuid)
 {
 	int ret = 0;
 
+	mutex_lock(&o2hb_dependency_mutex);
 	spin_lock(&o2hb_live_lock);
 
 	/* local heartbeat */
@@ -2428,11 +2501,13 @@ static int o2hb_region_inc_user(const char *region_uuid)
 
 unlock:
 	spin_unlock(&o2hb_live_lock);
+	mutex_unlock(&o2hb_dependency_mutex);
 	return ret;
 }
 
 static void o2hb_region_dec_user(const char *region_uuid)
 {
+	mutex_lock(&o2hb_dependency_mutex);
 	spin_lock(&o2hb_live_lock);
 
 	/* local heartbeat */
@@ -2451,6 +2526,7 @@ static void o2hb_region_dec_user(const char *region_uuid)
 
 unlock:
 	spin_unlock(&o2hb_live_lock);
+	mutex_unlock(&o2hb_dependency_mutex);
 }
 
 int o2hb_register_callback(const char *region_uuid,
-- 
2.39.3

[PATCH v2 2/3] ocfs2: cluster: avoid lock order inversion in o2hb_region_pin() from drop_item

[Joseph Qi]

o2hb_heartbeat_group_drop_item() is called from configfs rmdir with the
parent directory's inode_lock held.  It calls o2hb_region_pin() ->
o2nm_depend_item() -> configfs_depend_item(), which acquires the configfs
root inode_lock.  This creates a parent -> root inode_lock nesting that
could deadlock against paths taking root -> parent (e.g. subsystem
unregistration).

Fix this by using configfs_depend_item_unlocked() when o2hb_region_pin()
is called from a configfs callback context.  This variant skips the root
inode_lock when caller and target are in the same subsystem, which is
safe because VFS already holds a lock preventing unregistration.

Add o2nm_depend_item_unlocked() wrapper and a from_callback parameter to
o2hb_region_pin() to select the appropriate variant.

Fixes: 58a3158a5d17 ("ocfs2/cluster: Pin/unpin o2hb regions")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/cluster/heartbeat.c   | 17 ++++++++++-------
 fs/ocfs2/cluster/nodemanager.c |  6 ++++++
 fs/ocfs2/cluster/nodemanager.h |  1 +
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
index 5ca1d9c0c6575..b9395835523e2 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -146,7 +146,7 @@ static unsigned int o2hb_dependent_users;
  * In global heartbeat mode, we pin/unpin all o2hb regions. This solution
  * works for both file system and userdlm domains.
  */
-static int o2hb_region_pin(const char *region_uuid);
+static int o2hb_region_pin(const char *region_uuid, bool from_callback);
 static void o2hb_region_unpin(const char *region_uuid);
 
 /* Only sets a new threshold if there are no active regions.
@@ -2188,7 +2188,7 @@ static void o2hb_heartbeat_group_drop_item(struct config_group *group,
 
 	if (bitmap_weight(o2hb_quorum_region_bitmap,
 			   O2NM_MAX_REGIONS) <= O2HB_PIN_CUT_OFF)
-		o2hb_region_pin(NULL);
+		o2hb_region_pin(NULL, true);
 
 unlock:
 	spin_unlock(&o2hb_live_lock);
@@ -2330,7 +2330,7 @@ EXPORT_SYMBOL_GPL(o2hb_setup_callback);
  * In local, we only pin the matching region. In global we pin all the active
  * regions.
  */
-static int o2hb_region_pin(const char *region_uuid)
+static int o2hb_region_pin(const char *region_uuid, bool from_callback)
 {
 	int ret = 0, found;
 	struct o2hb_region *reg, *pinned;
@@ -2385,7 +2385,10 @@ static int o2hb_region_pin(const char *region_uuid)
 		spin_unlock(&o2hb_live_lock);
 
 		/* Ignore ENOENT only for local hb (userdlm domain) */
-		ret = o2nm_depend_item(&pinned->hr_item);
+		if (from_callback)
+			ret = o2nm_depend_item_unlocked(&pinned->hr_item);
+		else
+			ret = o2nm_depend_item(&pinned->hr_item);
 
 		spin_lock(&o2hb_live_lock);
 		if (!ret) {
@@ -2483,8 +2486,8 @@ static int o2hb_region_inc_user(const char *region_uuid)
 
 	/* local heartbeat */
 	if (!o2hb_global_heartbeat_active()) {
-	    ret = o2hb_region_pin(region_uuid);
-	    goto unlock;
+		ret = o2hb_region_pin(region_uuid, false);
+		goto unlock;
 	}
 
 	/*
@@ -2497,7 +2500,7 @@ static int o2hb_region_inc_user(const char *region_uuid)
 
 	if (bitmap_weight(o2hb_quorum_region_bitmap,
 			   O2NM_MAX_REGIONS) <= O2HB_PIN_CUT_OFF)
-		ret = o2hb_region_pin(NULL);
+		ret = o2hb_region_pin(NULL, false);
 
 unlock:
 	spin_unlock(&o2hb_live_lock);
diff --git a/fs/ocfs2/cluster/nodemanager.c b/fs/ocfs2/cluster/nodemanager.c
index e1f8f577ce5d6..ebdf0bdbb8c69 100644
--- a/fs/ocfs2/cluster/nodemanager.c
+++ b/fs/ocfs2/cluster/nodemanager.c
@@ -778,6 +778,12 @@ int o2nm_depend_item(struct config_item *item)
 	return configfs_depend_item(&o2nm_cluster_group.cs_subsys, item);
 }
 
+int o2nm_depend_item_unlocked(struct config_item *item)
+{
+	return configfs_depend_item_unlocked(&o2nm_cluster_group.cs_subsys,
+					     item);
+}
+
 void o2nm_undepend_item(struct config_item *item)
 {
 	configfs_undepend_item(item);
diff --git a/fs/ocfs2/cluster/nodemanager.h b/fs/ocfs2/cluster/nodemanager.h
index 39006005427b6..ca3483fb54504 100644
--- a/fs/ocfs2/cluster/nodemanager.h
+++ b/fs/ocfs2/cluster/nodemanager.h
@@ -64,6 +64,7 @@ void o2nm_node_get(struct o2nm_node *node);
 void o2nm_node_put(struct o2nm_node *node);
 
 int o2nm_depend_item(struct config_item *item);
+int o2nm_depend_item_unlocked(struct config_item *item);
 void o2nm_undepend_item(struct config_item *item);
 int o2nm_depend_node(u8 node_num);
 void o2nm_undepend_node(u8 node_num);
-- 
2.39.3

[PATCH v2 3/3] ocfs2: cluster: fix o2hb_dependent_users leak on pin failure

[Joseph Qi]

In o2hb_region_inc_user(), o2hb_dependent_users is incremented
unconditionally before calling o2hb_region_pin().  If the pin fails,
the counter is never decremented and any partially-pinned regions are
never unpinned, since the caller does not call o2hb_region_dec_user()
on error.

The leaked counter causes subsequent o2hb_region_inc_user() calls to
skip pinning entirely (the > 1 check), leaving heartbeat regions
unprotected.

Fix by rolling back on failure: call o2hb_region_unpin(NULL) to release
any partially-pinned regions and decrement o2hb_dependent_users to
restore the pre-increment state.

Fixes: 58a3158a5d17 ("ocfs2/cluster: Pin/unpin o2hb regions")
Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
---
 fs/ocfs2/cluster/heartbeat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
index b9395835523e2..c25aa2ff3ebc2 100644
--- a/fs/ocfs2/cluster/heartbeat.c
+++ b/fs/ocfs2/cluster/heartbeat.c
@@ -2499,8 +2499,13 @@ static int o2hb_region_inc_user(const char *region_uuid)
 		goto unlock;
 
 	if (bitmap_weight(o2hb_quorum_region_bitmap,
-			   O2NM_MAX_REGIONS) <= O2HB_PIN_CUT_OFF)
+			  O2NM_MAX_REGIONS) <= O2HB_PIN_CUT_OFF) {
 		ret = o2hb_region_pin(NULL, false);
+		if (ret) {
+			o2hb_region_unpin(NULL);
+			o2hb_dependent_users--;
+		}
+	}
 
 unlock:
 	spin_unlock(&o2hb_live_lock);
-- 
2.39.3

Re: [PATCH v2 0/3] ocfs2: cluster: o2hb_region_pin() fixes

[Andrew Morton]

On Wed, 22 Jul 2026 20:49:30 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote:

> This series fixes three related issues in o2hb_region_pin(), all are
> from the original implementation in commit:
> 58a3158a5d17 ("ocfs2/cluster: Pin/unpin o2hb regions"):

Thanks.  I assume we want cc:stable on all three?

(If there's no known way in which userspace can hit any of these issues
then cc:stable can be omitted, but I expect the stablebots will
backport the patches anyway, based on their Fixes:)

Re: [PATCH v2 0/3] ocfs2: cluster: o2hb_region_pin() fixes

[Joseph Qi]

On 7/23/26 7:43 AM, Andrew Morton wrote:
> On Wed, 22 Jul 2026 20:49:30 +0800 Joseph Qi <joseph.qi@linux.alibaba.com> wrote:
> 
>> This series fixes three related issues in o2hb_region_pin(), all are
>> from the original implementation in commit:
>> 58a3158a5d17 ("ocfs2/cluster: Pin/unpin o2hb regions"):
> 
> Thanks.  I assume we want cc:stable on all three?
> 
> (If there's no known way in which userspace can hit any of these issues
> then cc:stable can be omitted, but I expect the stablebots will
> backport the patches anyway, based on their Fixes:)
> 

These bugs pre-existing for a long time. And it seems can't be easily
reproduced.

Cc stable is fine. But I'm afraid some stable trees may not be applied
cleanly.

Thanks,
Joseph