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

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

[Ginger Li]

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@gmail.com>
---
 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);
 
 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

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

[Markus Elfring]

…
> Take osb->osb_lock when updating both fields.

May lock guards be applied in affected function implementations?

Regards,
Markus

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

[Ginger]

Hi Markus,

Thanks for checking. Do the following changes match your expectation? Thanks.

Sincerely,
Ginger
---
 fs/ocfs2/localalloc.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 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;
  }

- osb->local_alloc_bh = alloc_bh;
- osb->local_alloc_state = OCFS2_LA_ENABLED;
+ scoped_guard(spinlock, &osb->osb_lock) {
+ osb->local_alloc_bh = alloc_bh;
+ osb->local_alloc_state = OCFS2_LA_ENABLED;
+ }

 bail:
  if (status < 0)
@@ -392,7 +394,9 @@ void ocfs2_shutdown_local_alloc(struct ocfs2_super *os
  goto out;
  }

- osb->local_alloc_state = OCFS2_LA_DISABLED;
+ scoped_guard(spinlock, &osb->osb_lock) {
+ osb->local_alloc_state = OCFS2_LA_DISABLED;
+ }

  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);
- osb->local_alloc_bh = NULL;
- osb->local_alloc_state = OCFS2_LA_UNUSED;
+ scoped_guard(spinlock, &osb->osb_lock) {
+ osb->local_alloc_bh = NULL;
+ osb->local_alloc_state = OCFS2_LA_UNUSED;
+ }

  status = ocfs2_sync_local_to_main(osb, handle, alloc_copy,
   main_bm_inode, main_bm_bh);
-- 
2.43.0

On Thu, Sep 24, 2026 at 6:55 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> …
> > Take osb->osb_lock when updating both fields.
>
> May lock guards be applied in affected function implementations?
>
> Regards,
> Markus

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

[Markus Elfring]

> Thanks for checking. Do the following changes match your expectation?

I guess that source code indentation should be adjusted according to the proposed
usage of scoped lock guards.
Will further patch versions follow for improved applications of scope-based resource management?

Regards,
Markus

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

[Heming Zhao]

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@gmail.com>
> ---
>  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