Re: [PATCH v2] ocfs2/heartbeat: serialize concurrent writes to a region's dev attribute

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

On 10/19/26 3:29 PM, Eklavya Singh wrote:
> o2hb_region_dev_store() checks reg->hr_bdev_file to see if the region
> is already started, but nothing stops two writers from doing that at
> the same time. configfs only serializes writes through the same open
> file, so two processes that each open the dev attribute can both get
> past the check:
> 
>   A: hr_bdev_file is NULL, opens file A, sets hr_bdev_file = A
>   B: hr_bdev_file is NULL, opens file B, sets hr_bdev_file = B
>   A: start fails, fput(hr_bdev_file) drops B, sets it to NULL
>   B: start fails, fput(NULL)
> 
> Both calls also use the same hr_slots, hr_slot_data and hr_tmp_block,
> so the error paths can free the slot data twice.
> 
> syzbot hit the fput(NULL) with an unbound loop device, where every
> start fails with -EIO. With four threads doing the same thing on a
> KASAN kernel I get both crashes within a second:
> 
>   BUG: KASAN: double-free in o2hb_unmap_slot_data+0x14d/0x2f0
>   BUG: kernel NULL pointer dereference, address: 0000000000000158
> 
> Add a mutex to the region and hold it in dev_store(), in the
> block_bytes, start_block and blocks stores, and in dev_show(), since
> they all look at hr_bdev_file. dev_store() cleans up with goto, so it
> takes the mutex in a small wrapper rather than with guard(). It sleeps
> with the mutex held while it waits for the heartbeat thread. That is
> fine because neither the thread nor the teardown paths take it.
> 
> With the patch the same test ran for 10 minutes with lockdep on and
> nothing was reported.
> 
> Fixes: a7f6a5fb4bde ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
> Reported-by: syzbot+5d4e96acd0febccb5e87@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=5d4e96acd0febccb5e87
> Cc: stable@vger.kernel.org
> Signed-off-by: Eklavya Singh <eklavyasingh2106@gmail.com>
> ---
> v2: take the mutex in a wrapper around dev_store() instead of with
> guard(), since dev_store() uses goto for its cleanup. No other changes.
> 
> v1: https://lore.kernel.org/all/20261018163121.128911-1-eklavyasingh2106@gmail.com/
> 
>  fs/ocfs2/cluster/heartbeat.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index 1c3def99bb0..003c398366e 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -269,6 +269,8 @@ struct o2hb_region {
>  	struct list_head	hr_handler_list;
>  	/* Serializes timeout arming against failed-start and teardown. */
>  	struct mutex		hr_arming_mutex;
> +	/* Serializes the region's configfs attributes against dev_store(). */

Naming the covered 4 region's attributes would be clearer.

> +	struct mutex		hr_dev_mutex;
>  	bool			hr_stopping;
>  
>  	/* last hb status, 0 for success, other value for error. */
> @@ -1676,6 +1678,7 @@ static ssize_t o2hb_region_block_bytes_store(struct config_item *item,
>  	unsigned long block_bytes;
>  	unsigned int block_bits;
>  
> +	guard(mutex)(&reg->hr_dev_mutex);
>  	if (reg->hr_bdev_file)
>  		return -EINVAL;
>  
> @@ -1705,6 +1708,7 @@ static ssize_t o2hb_region_start_block_store(struct config_item *item,
>  	char *p = (char *)page;
>  	ssize_t ret;
>  
> +	guard(mutex)(&reg->hr_dev_mutex);
>  	if (reg->hr_bdev_file)
>  		return -EINVAL;
>  
> @@ -1731,6 +1735,7 @@ static ssize_t o2hb_region_blocks_store(struct config_item *item,
>  	char *p = (char *)page;
>  	int ret;
>  
> +	guard(mutex)(&reg->hr_dev_mutex);
>  	if (reg->hr_bdev_file)
>  		return -EINVAL;
>  
> @@ -1748,10 +1753,12 @@ static ssize_t o2hb_region_blocks_store(struct config_item *item,
>  
>  static ssize_t o2hb_region_dev_show(struct config_item *item, char *page)
>  {
> +	struct o2hb_region *reg = to_o2hb_region(item);
>  	unsigned int ret = 0;
>  
> -	if (to_o2hb_region(item)->hr_bdev_file)
> -		ret = sprintf(page, "%pg\n", reg_bdev(to_o2hb_region(item)));
> +	guard(mutex)(&reg->hr_dev_mutex);
> +	if (reg->hr_bdev_file)
> +		ret = sprintf(page, "%pg\n", reg_bdev(reg));
>  
>  	return ret;
>  }
> @@ -1867,11 +1874,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
>   */
> -static ssize_t o2hb_region_dev_store(struct config_item *item,
> -				     const char *page,
> -				     size_t count)
> +static ssize_t __o2hb_region_dev_store(struct o2hb_region *reg,
> +				       const char *page, size_t count)
>  {
> -	struct o2hb_region *reg = to_o2hb_region(item);

Would it be worth recording that hr_dev_mutex must be held now?

>  	struct task_struct *hb_task;
>  	long fd;
>  	int sectsize;
> @@ -2032,6 +2037,20 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  	return ret;
>  }
>  
> +static ssize_t o2hb_region_dev_store(struct config_item *item,
> +				     const char *page,
> +				     size_t count)
> +{
> +	struct o2hb_region *reg = to_o2hb_region(item);
> +	ssize_t ret;
> +
> +	mutex_lock(&reg->hr_dev_mutex);
> +	ret = __o2hb_region_dev_store(reg, page, count);
> +	mutex_unlock(&reg->hr_dev_mutex);
> +
> +	return ret;
> +}
> +
The wrapper keeps hr_dev_mutex held across the whole start, including
the wait for the thread:

        ret = wait_event_interruptible(o2hb_steady_queue,
                                atomic_read(&reg->hr_steady_iterations) == 0 ||
                                reg->hr_node_deleted);

hr_timeout_ms is O2HB_REGION_TIMEOUT_MS, 2000, and hr_steady_iterations
starts at O2HB_LIVE_THRESHOLD + 1, or one more doubling again when this
is the only region and global heartbeat is active.  So a read of the dev
attribute that races a start blocks for roughly six seconds, or ten for
that first region, where it used to return at once.

Thanks,
Joseph

>  static ssize_t o2hb_region_pid_show(struct config_item *item, char *page)
>  {
>  	struct o2hb_region *reg = to_o2hb_region(item);
> @@ -2156,6 +2175,7 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
>  	reg->hr_key = crc32_le(reg->hr_region_num + O2NM_MAX_REGIONS,
>  		name, strlen(name));
>  	mutex_init(&reg->hr_arming_mutex);
> +	mutex_init(&reg->hr_dev_mutex);
>  	reg->hr_stopping = true;
>  	INIT_DELAYED_WORK(&reg->hr_write_timeout_work, o2hb_write_timeout);
>  	INIT_DELAYED_WORK(&reg->hr_nego_timeout_work, o2hb_nego_timeout);