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

[Deepanshu Kartikey]

On Thu, Sep 24, 2026 at 5:37 PM Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> 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.
>

Two tasks can concurrently write to the same region's "dev" configfs
attribute, each through its own open fd.

The only guard against a second writer is an unlocked check near the
top of o2hb_region_dev_store():

    if (reg->hr_bdev_file)
            return -EINVAL;

This check happens well before hr_bdev_file is actually assigned --
several validation steps and an fd lookup sit in between. If a second
task runs this same check before the first task has gotten around to
assigning hr_bdev_file, it also reads NULL and also proceeds, even
though the first task has already committed to setting up this
region.

Both tasks then go on to call o2hb_map_slot_data(), which allocates
reg->hr_slot_data, and later, on any failure, o2hb_unmap_slot_data(),
which frees it. Since hr_slot_data is also unprotected, one task can
free it via its own failure path while the other task is still
populating or using the array it allocated, producing the
use-after-free seen in the syzbot report.



> > 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().
>

You're right, that commit is just the hr_bdev_handle -> hr_bdev_file
rename and the bdev_release() -> fput() swap, not the source of the
race. I'll update Fixes: in v2.


> > Reported-by: syzbot+3025e3e8fc0b928af8f5@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=3025e3e8fc0b928af8f5
> > Tested-by: syzbot+3025e3e8fc0b928af8f5@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> > ---
> >  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.
>

I will this in v2

> > +
> >  };
> >
> >  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.
>

I'll drop the locking in
o2hb_region_release() in v2,  the mutex is only needed in
o2hb_region_dev_store() to guard against two concurrent stores.


> >       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().


You're right, I'll move the lock and hr_bdev_file check back to the
top, before hr_node_num is assigned, in v2.

>
> > +
> >       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.
>