* Re: [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store
2026-09-04 23:57 [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store Deepanshu Kartikey
@ 2026-09-23 3:26 ` Deepanshu Kartikey
2026-09-24 10:37 ` Heming Zhao
2026-09-24 12:07 ` Joseph Qi
2 siblings, 0 replies; 4+ messages in thread
From: Deepanshu Kartikey @ 2026-09-23 3:26 UTC (permalink / raw)
To: mark, jlbec, joseph.qi, akpm, brauner, jack
Cc: zzzccc427, ericterminal, ocfs2-devel, christophe.jaillet,
linux-kernel, syzbot+3025e3e8fc0b928af8f5
On Sat, Sep 5, 2026 at 5:27 AM Deepanshu Kartikey <kartikey406@gmail.com> 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().
>
> Fixes: 1d3aa0b97c55 ("ocfs2: port block device access to file")
> 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;
> +
> };
>
> 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(®->hr_handler_list);
> +
> + mutex_lock(®->hr_dev_write_mutex);
> o2hb_unmap_slot_data(reg);
> + mutex_unlock(®->hr_dev_write_mutex);
>
> 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(®->hr_dev_write_mutex))
> + return -ERESTARTSYS;
> +
> + if (reg->hr_bdev_file) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> 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(®->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(®->hr_item, name, &o2hb_region_type);
>
> + mutex_init(®->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.
> --
> 2.34.1
>
Gentle Reminder. Please let me know the status of this patch.
Thanks
Deepanshu
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store
2026-09-04 23:57 [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store Deepanshu Kartikey
2026-09-23 3:26 ` Deepanshu Kartikey
@ 2026-09-24 10:37 ` Heming Zhao
2026-09-24 12:07 ` Joseph Qi
2 siblings, 0 replies; 4+ messages in thread
From: Heming Zhao @ 2026-09-24 10:37 UTC (permalink / raw)
To: Deepanshu Kartikey, joseph.qi
Cc: mark, jlbec, akpm, brauner, jack, zzzccc427, ericterminal,
ocfs2-devel, christophe.jaillet, linux-kernel,
syzbot+3025e3e8fc0b928af8f5
On Sat, Sep 05, 2026 at 05:27:10AM +0530, 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().
Hi,
Did AI generate this patch? The commit log says "reg->hr_slot_data" should be
protected, but the code protects ->hr_bdev_file in o2hb_region_dev_store(), not
o2hb_map_slot_data().
Just code logic, I prefer changing struct mutex hr_dev_write_mutex to hr_mutex.
@Joseph,
I'm not very familiar with the heartbeat, based on the syzbot report, it seems
hr_slot_data needs protection. Does it make sense to fix this in cluster world?
Thanks,
Heming
>
> Fixes: 1d3aa0b97c55 ("ocfs2: port block device access to file")
> 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;
> +
> };
>
> 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(®->hr_handler_list);
> +
> + mutex_lock(®->hr_dev_write_mutex);
> o2hb_unmap_slot_data(reg);
> + mutex_unlock(®->hr_dev_write_mutex);
>
> 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(®->hr_dev_write_mutex))
> + return -ERESTARTSYS;
> +
> + if (reg->hr_bdev_file) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> 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(®->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(®->hr_item, name, &o2hb_region_type);
>
> + mutex_init(®->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.
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store
2026-09-04 23:57 [PATCH] ocfs2: fix use-after-free in o2hb_region_dev_store Deepanshu Kartikey
2026-09-23 3:26 ` Deepanshu Kartikey
2026-09-24 10:37 ` Heming Zhao
@ 2026-09-24 12:07 ` Joseph Qi
2 siblings, 0 replies; 4+ messages in thread
From: Joseph Qi @ 2026-09-24 12:07 UTC (permalink / raw)
To: Deepanshu Kartikey
Cc: mark, jlbec, akpm, Heming Zhao, brauner, jack, zzzccc427,
ericterminal, ocfs2-devel, christophe.jaillet, linux-kernel,
syzbot+3025e3e8fc0b928af8f5
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(®->hr_handler_list);
> +
> + mutex_lock(®->hr_dev_write_mutex);
> o2hb_unmap_slot_data(reg);
> + mutex_unlock(®->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(®->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(®->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(®->hr_item, name, &o2hb_region_type);
>
> + mutex_init(®->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.
^ permalink raw reply [flat|nested] 4+ messages in thread