Re: [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store

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

On 9/5/26 7:57 AM, Deepanshu Kartikey wrote:
> Concurrent writes to a region's "dev" attribute can race, letting two
> tasks both allocate/free reg->hr_slot_data for the same region and
> causing a use-after-free. Add a per-region mutex to serialize
> o2hb_region_dev_store() against itself and against
> o2hb_region_release().
> 

The description is too simple.
Please describe the issue in detail, better with a race flow.

> Fixes: 1d3aa0b97c55 ("ocfs2: port block device access to file")

Why blames commit 1d3aa0b97c55? It seems it just renames hr_bdev_handle
to hr_bdev_file and swaps bdev_release() for fput().

> Reported-by: syzbot+3025e3e8fc0b928af8f5@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=3025e3e8fc0b928af8f5
> Tested-by: syzbot+3025e3e8fc0b928af8f5@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  fs/ocfs2/cluster/heartbeat.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/heartbeat.c b/fs/ocfs2/cluster/heartbeat.c
> index 1c3def99bb07..6f03de1b6d4a 100644
> --- a/fs/ocfs2/cluster/heartbeat.c
> +++ b/fs/ocfs2/cluster/heartbeat.c
> @@ -273,6 +273,9 @@ struct o2hb_region {
>  
>  	/* last hb status, 0 for success, other value for error. */
>  	int			hr_last_hb_status;
> +	/* Serializes dev_store() against itself and region_release() */
> +	struct mutex            hr_dev_write_mutex;

Please use tab instead space.

> +
>  };
>  
>  static inline struct block_device *reg_bdev(struct o2hb_region *reg)
> @@ -1616,7 +1619,10 @@ static void o2hb_region_release(struct config_item *item)
>  
>  	o2hb_quiesce_timeout(reg);
>  	o2net_unregister_and_flush_handler_list(&reg->hr_handler_list);
> +
> +	mutex_lock(&reg->hr_dev_write_mutex);
>  	o2hb_unmap_slot_data(reg);
> +	mutex_unlock(&reg->hr_dev_write_mutex);
>  

Seems frag_sem in configfs serializes store and rmdir. So don't
understand the race.

>  	if (reg->hr_bdev_file)
>  		fput(reg->hr_bdev_file);
> @@ -1879,9 +1885,6 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  	ssize_t ret = -EINVAL;
>  	int live_threshold;
>  
> -	if (reg->hr_bdev_file)
> -		return -EINVAL;
> -
>  	/* We can't heartbeat without having had our node number
>  	 * configured yet. */
>  	reg->hr_node_num = o2nm_this_node();
> @@ -1906,12 +1909,20 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  	if (!S_ISBLK(fd_file(f)->f_mapping->host->i_mode))
>  		return -EINVAL;
>  
> +	if (mutex_lock_interruptible(&reg->hr_dev_write_mutex))
> +		return -ERESTARTSYS;
> +
> +	if (reg->hr_bdev_file) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}

This seems buggy.
We have to do this before the hr_node_num assignment. Otherwise it will
conflict with the logic in o2hb_region_dev_store().

> +
>  	reg->hr_bdev_file = bdev_file_open_by_dev(fd_file(f)->f_mapping->host->i_rdev,
>  			BLK_OPEN_WRITE | BLK_OPEN_READ, NULL, NULL);
>  	if (IS_ERR(reg->hr_bdev_file)) {
>  		ret = PTR_ERR(reg->hr_bdev_file);
>  		reg->hr_bdev_file = NULL;
> -		return ret;
> +		goto out_unlock;
>  	}
>  
>  	sectsize = bdev_logical_block_size(reg_bdev(reg));
> @@ -2029,6 +2040,8 @@ static ssize_t o2hb_region_dev_store(struct config_item *item,
>  		fput(reg->hr_bdev_file);
>  		reg->hr_bdev_file = NULL;
>  	}
> +out_unlock:
> +	mutex_unlock(&reg->hr_dev_write_mutex);
>  	return ret;
>  }
>  
> @@ -2149,6 +2162,8 @@ static struct config_item *o2hb_heartbeat_group_make_item(struct config_group *g
>  
>  	config_item_init_type_name(&reg->hr_item, name, &o2hb_region_type);
>  
> +	mutex_init(&reg->hr_dev_write_mutex);
> +
>  	/* this is the same way to generate msg key as dlm, for local heartbeat,
>  	 * name is also the same, so make initial crc value different to avoid
>  	 * message key conflict.