Re: [PATCH] ocfs2: Protect local_alloc_state updates in load/shutdown

[Ginger]

Hi Heming and Markus,

Thanks for checking this patch. According to Heming's response, I
think this patch is not necessary.

Sincerely,
Ginger

On Tue, Sep 29, 2026 at 4:59 AM Heming Zhao <heming.zhao@xxxxxxxx> wrote:
>
> On Tue, Sep 22, 2026 at 02:00:48PM +0800, Ginger Li wrote:
> > My static analyzer identified a potential issue in 'fs/ocfs2/localalloc.c':
> > osb->local_alloc_state is documented as protected by osb->osb_lock in
> > struct ocfs2_super, and most of the code follows that rule:
> > ocfs2_local_alloc_seen_free_bits(), ocfs2_la_enable_worker() and
> > ocfs2_recalc_la_window() all update the field with the lock held.
> >
> > ocfs2_load_local_alloc() and ocfs2_shutdown_local_alloc() update
> > local_alloc_state, and local_alloc_bh next to it, without taking osb_lock, so
> > those stores can race with the reads and writes done by the local alloc
> > reserve path.  Those two writers predate the locking convention and were
> > never converted.
> >
> > Take osb->osb_lock when updating both fields.
> >
> > Fixes: ccd979bdbce9 ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
> > Signed-off-by: Ginger Li <ginger.jzllee@xxxxxxxxx>
> > ---
> >  fs/ocfs2/localalloc.c | 6 ++++++
> >  1 file changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
> > --- a/fs/ocfs2/localalloc.c
> > +++ b/fs/ocfs2/localalloc.c
> > @@ -342,8 +342,10 @@ int ocfs2_load_local_alloc(struct ocfs2_super *osb)
> >               goto bail;
> >       }
> >
> > +     spin_lock(&osb->osb_lock);
> >       osb->local_alloc_bh = alloc_bh;
> >       osb->local_alloc_state = OCFS2_LA_ENABLED;
> > +     spin_unlock(&osb->osb_lock);
> >
>
> This function is triggered during the mount phase. The la (localalloc) is only
> active after this point, and the fs is still in the initialization state, so no
> inodes can be created. Therefore, we don't need to worry about any race conditions.
>
> >  bail:
> >       if (status < 0)
> > @@ -392,7 +394,9 @@ void ocfs2_shutdown_local_alloc(struct ocfs2_super *os
> >               goto out;
> >       }
> >
> > +     spin_lock(&osb->osb_lock);
> >       osb->local_alloc_state = OCFS2_LA_DISABLED;
> > +     spin_unlock(&osb->osb_lock);
> >
> >       ocfs2_resmap_uninit(&osb->osb_la_resmap);
> >
> > @@ -441,8 +445,10 @@ void ocfs2_shutdown_local_alloc(struct ocfs2_super *os
> >       ocfs2_journal_dirty(handle, bh);
> >
> >       brelse(bh);
> > +     spin_lock(&osb->osb_lock);
> >       osb->local_alloc_bh = NULL;
> >       osb->local_alloc_state = OCFS2_LA_UNUSED;
> > +     spin_unlock(&osb->osb_lock);
> >
> >       status = ocfs2_sync_local_to_main(osb, handle, alloc_copy,
> >                                         main_bm_inode, main_bm_bh);
> > --
> > 2.43.0
> >
>
> When the code enters the umount phase, the VFS layer ensures that there are
> no other references to the fs, so no race can occur at this time.
>
> At last, if you have reproducible steps, please provide them to help us
> understand the issue better.
>
> Thanks,
> Heming