Re: [PATCH] ocfs2: validate la_size before ocfs2_clear_local_alloc()

[Heming Zhao <heming.zhao@suse.com>]

On Sun, Sep 27, 2026 at 02:04:05PM +0300, d.morgun@ispras.ru wrote:
> On 8/11/26 9:57 AM, Joseph Qi wrote:
> > I think we can validate localalloc inode during block read in
> > ocfs2_validate_inode_block(), which will drop the duplicate code in
> > callers.
> 
> Hi Joseph,
> 
> Thanks for the suggestion. I moved the la_size check there, and it
> does catch the current reproducer during mount. Before dropping the
> duplicate checks in ocfs2_begin_local_alloc_recovery() and
> ocfs2_load_local_alloc(), I'd like to raise two concerns:
> 
> 1. ocfs2_begin_local_alloc_recovery() reads the inode with
>    OCFS2_BH_IGNORE_CACHE, and ocfs2_read_blocks() skips validation
>    for dirty buffers. I traced through jbd2 replay: ocfs2_replay_journal()
>    calls jbd2_journal_flush() right after jbd2_journal_load(), and
>    jbd2_journal_recover() itself calls sync_blockdev(), which should
>    clear the dirty flag before ocfs2_begin_local_alloc_recovery() runs.
>    So this may not be reachable in practice, but I haven't been able
>    to confirm it directly. The current reproducer hits the
>    validator with a clean buffer during the initial mount, before
>    recovery is reached at all. Is it possible to confirm that the
>    buffer is guaranteed to be clean by that point?

In my view, OCFS2_BH_IGNORE_CACHE tells the lower layer to bypass the cache.
If the buffer is dirty on this code path, that would be a separate bug.

> 
> 2. The check is gated on OCFS2_LOCAL_ALLOC_FL, but
>    ocfs2_validate_inode_block() doesn't receive the inode's expected
>    type, so it can't verify that the on-disk flags actually match
>    what the caller expects. A corrupted local alloc inode with the
>    flag cleared would skip the check, even though
>    ocfs2_clear_local_alloc() still treats its id2 as i_lab. Is there
>    an existing way to check this correspondence? Otherwise, this
>    might call for a separate per-type validation helper that takes
>    the expected inode type, which could also help elsewhere.
> 
> My inclination is: v1 (the check directly in
> ocfs2_begin_local_alloc_recovery() and ocfs2_load_local_alloc())
> has held up in testing so far, with a small additional fix on my
> end. Both call sites operate in the LOCAL_ALLOC_SYSTEM_INODE context
> and check la_size before using id2 as i_lab, so they do not depend on
> OCFS2_LOCAL_ALLOC_FL for deciding whether to validate la_size.
> Returning this new error also made a separate, pre-existing bug in
> the recovery thread reachable, for which I have a small follow-up
> fix.
> 
> If the two concerns with ocfs2_validate_inode_block() can be
> addressed without too much churn, I agree it is the better place
> for this check, since it would cover every caller uniformly.
> Otherwise, I'd rather keep it in v1's approach.
> 
> Does this seem reasonable, or would you weigh it differently?
> 
> Thanks,
> Dmitry
> 

Hope I understand your concerns correctly.
You can reject further operations in ocfs2_begin_local_alloc_recovery() when
OCFS2_LOCAL_ALLOC_FL is not set, while keeping the la_size check in
ocfs2_validate_inode_block() guarded by OCFS2_LOCAL_ALLOC_FL.

Thanks,
Heming